From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Thu, 16 Jun 2016 10:20:38 +0200 From: Willy Tarreau To: Al Viro Cc: Linus Torvalds , Andy Lutomirski , Andy Lutomirski , Linux FS Devel , Stephen Rothwell , Andrew Morton , stable Subject: Re: [PATCH v2 1/2] fs: Improve and simplify copy_mount_options Message-ID: <20160616082038.GA7409@1wt.eu> References: <20160615235047.GA14480@ZenIV.linux.org.uk> <20160616054525.GA6803@1wt.eu> <20160616065738.GA7154@1wt.eu> <20160616070821.GB14480@ZenIV.linux.org.uk> <20160616072557.GA7336@1wt.eu> <20160616080229.GC14480@ZenIV.linux.org.uk> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160616080229.GC14480@ZenIV.linux.org.uk> Sender: stable-owner@vger.kernel.org List-ID: On Thu, Jun 16, 2016 at 09:02:29AM +0100, Al Viro wrote: > On Thu, Jun 16, 2016 at 09:25:57AM +0200, Willy Tarreau wrote: > > On Thu, Jun 16, 2016 at 08:08:22AM +0100, Al Viro wrote: > > > On Thu, Jun 16, 2016 at 08:57:38AM +0200, Willy Tarreau wrote: > > > > + type = get_fs_type(fstype); > > > > + if (!type) > > > > + return NULL; > > > > + > > > > copy = kmalloc(PAGE_SIZE, GFP_KERNEL); > > > > if (!copy) > > > > return ERR_PTR(-ENOMEM); > > > > > > > > + /* avoid reading a whole page if the FS only needs a string. */ > > > > + if (!(type->fs_flags & FS_BINARY_MOUNTDATA)) { > > > > + strlcpy(copy, data, PAGE_SIZE); > > > > + return copy; > > > > > > a) it leaks a file_system_type reference > > > > I was not sure about this one, thanks for confirming. > > > > > b) data is a userland pointer, for crying out loud! > > > > Yep I noticed it and fixed it after sending. I was focused on the > > data coming from kernel due to the discussion. > > > > I also think that since there are only two call places for > > copy_mount_options(), we may move the test there and switch > > to copy_mount_string() instead depending on the fs type. > > Another problem is that it will oops with NULL fstype, which is > absolutely normal both for mount --bind *and* mount -o remount. OK thanks for explaining, I didn't know. > And while mount --bind doesn't care about string options, > mount -o remount certainly does. IMO the latter makes that > approach hopeless - with remount you don't know the type > until well into do_mount() guts and I'd really hate to carry > the userland pointer all the way into it. Agreed. However if the initial point was to avoid reading extra pages most of the time, we could possibly consider that the string copy is an optimization for the case where we have the information available. Thus if !fstype || !(type->fs_flags & FS_BINARY_MOUNTDATA) then we use copy_mount_options() otherwise we use copy_mount_string(). It will only leave the full page copy for mount --bind or -o remount then. Willy