Re: [PATCH v2 1/2] fs: Improve and simplify copy_mount_options

[Willy Tarreau <w@1wt.eu>]

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