Fix MNAMELEN or reimplement struct statfs

[Konstantin Belousov]

On Mon, Nov 18, 2013 at 11:01:42AM -0800, Doug Ambrisko wrote:
> On Sat, Nov 16, 2013 at 08:31:29PM +0200, Konstantin Belousov wrote:
> | Generally, I agree with the approach, but what is done seems to be too
> | simple to be usable.
> 
> I like the simplicity and I'd like to see examples of not being usable.
I did exactly this in the text following the introductionary sentence,
isn't it ?

>  
> | One obvious and important thing which is broken with the patch is
> | the unmounts from jails. In other words, now it is possible to mount
> | something from jail with appropriate privileges set up, and after that
> | corresponding mount cannot be unmounted, since vfs_mount_alloc() copies
> | trimmed path into f_mntonname, and sys_unmount() matches full path with
> | pathbuf.  Hmm, this should be broken in the same way for non-jailed
> | mounts with pathes which do not fit into f_mntonname.
> 
> They can be umounted since it will fall back to fsid as in the non-jail
> case.  I just tried it sorry for the bad line wrap:
> 
> + mount 192.168.38.1:/data/home/ambrisko/netboot /data/jail/test
> + jail -i -c name=test path=/data/jail/test host.hostname=test.ambrisko.com persist enforce_statfs=0 allow.mount=1 allow.mount.devfs=1 allow.mount.nullfs=1 allow.mount.tmpfs=1 allow.mount.procfs=1
> 14
> + jexec test mkdir -p /1234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890/proc
> + jexec test mkdir -p /1234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890/dev
> + jexec test df
> + egrep '^devfs|^procfs'
> devfs                                             2         2          0   100%    /dev
> procfs                                            8         8          0   100%    /proc
> + jexec test mount -t procfs null //1234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890/proc
> + jexec test mount -t devfs null //1234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890/dev
> + jexec test df
> + egrep '^devfs|^procfs'
> devfs                                             2         2          0   100%    /dev
> procfs                                            8         8          0   100%    /proc
> procfs                                            8         8          0   100%    /data/jail/test/12345678901234567890123456789012345678901234567890123456789012345678901
> devfs                                             2         2          0   100%    /data/jail/test/12345678901234567890123456789012345678901234567890123456789012345678901
> + jexec test mount -v
> + egrep '^devfs|^procfs'
> devfs on /dev (devfs, local, multilabel, fsid 00ff007171000000)
> procfs on /proc (procfs, local, fsid 02ff000202000000)
> procfs on /data/jail/test/12345678901234567890123456789012345678901234567890123456789012345678901 (procfs, local, fsid 26ff000202000000)
> devfs on /data/jail/test/12345678901234567890123456789012345678901234567890123456789012345678901 (devfs, local, multilabel, fsid 27ff007171000000)
> + jexec test umount /1234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890/proc
> + jexec test df
> + egrep '^devfs|^procfs'
> devfs                                             2         2          0   100%    /dev
> procfs                                            8         8          0   100%    /proc
> devfs                                             2         2          0   100%    /data/jail/test/12345678901234567890123456789012345678901234567890123456789012345678901
> + jexec test umount /1234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890/dev
> + jexec test df
> + egrep '^devfs|^procfs'
> devfs                                             2         2          0   100%    /dev
> procfs                                            8         8          0   100%    /proc

I.e. unmount gets EINVAL, right ? I do not like it, if going this route,
why do we need to store the path in the kernel at all ? At least, the
attempt to unmount by path should consistently return EINVAL always,
instead of failing randomly due to an implementation detail, where the
caller can reasonably expect the syscall to succeed.

> 
> | I think that struct mount should have a const char * field where the
> | non-trimmed path is stored and used for match at unmount. f_mntonname
> | truncation would be only unfortunate user interface glitch.
> 
> Note that we are not storing the path in mount structure so no structures
> have changed which is nice since then we haven't introduced any real
> ABI breakage.  So we could MFC this.  The match isn't critical since
> umount will fall back to fsid and work.  One thing that might be good to
> do is change umount to try to umount via fsid first and then do the
> match if the fsid failed versus the other way round that it does now.
I do not like somtimes not storing the full path of the mount point.
I do understand that the path can easily made invalid, but I still want
it there.

MFC is not the problem for struct mount, which is never directly allocated
by non-VFS.  The new member must be added to the end of the structure, which
preserves KBI. I did such surgery more than once.

> 
> The problem I see is if someone tries to do things based on the parsed
> output of mount/df then that will fail since the output is truncated.
Yes, this is understandable and IMO acceptable.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 834 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/freebsd-hackers/attachments/20131119/5ddadac5/attachment.sig>