Fix MNAMELEN or reimplement struct statfs

[Doug Ambrisko]

On Fri, Nov 22, 2013 at 09:42:28AM +0200, Konstantin Belousov wrote:
| On Thu, Nov 21, 2013 at 09:40:28AM -0800, Doug Ambrisko wrote:
| > On Wed, Nov 20, 2013 at 09:55:31AM +0200, Konstantin Belousov wrote:
| > | On Tue, Nov 19, 2013 at 09:42:16AM -0800, Doug Ambrisko wrote:
| > | > I was talking about the more general case since the system tries to keep
| > | > the path in the stat structure.  My prior approach which had more issues
| > | > was to modify the stat structure of which I was pointed to NetBSD and their
| > | > change to statvfs which doesn't really solve the problem.  They don't
| > | > have the check to see if the mount is longer then VFS_MNAMELEN (in their case)
| > | > and just truncate things.
| > | > 
| > | > If we are just talking about adding it to the mount structure that
| > | > would be okay since it isn't exposed to user land.  I can add that.
| > |
| > | Yes, this is exactly what I mean.  Add a struct mount field, and use
| > | it for kernel only.  In fact, it only matters for sys_unmount() and
| > | kern_jail.c, other locations in kernel use the path for warnings, and
| > | this could be postponed if you prefer to minimize the patch.
| > 
| > Okay, I went through all of the occurances and compile tested (except
| > for #DEBUG).  I united a few things but should do more once I get
| > consensus on the approach.  I found a few spots that should be updated as
| > well and made the length check more consistant.  Some were doing >= and others
| > >.  So this should be better, however, a lot larger.  On the plus side
| > when we figure out how to return the longer path length to user land
| > that can be more flexible since the kernel is tracking the longer length.
| > Probably things to note are changes in:
| > 	ZFS to mount snapshot
| > 	cd9660 for symlinks
| > 	fuse to return full path
| > 	jail to check statfs and mount
| > 	mount/umount to save and check full path
| > 	mountroot to save new field for full path
| > 	
| > Just in case it doesn't make it in email the full patch is at:
| > 	http://people.freebsd.org/~ambrisko/mount_bigger.patch
| > 
| Yes, this is closer to the patch I can agree with :).
| 
| Two notes, one was already made, about off by one.

The off by one, I want to revisit so that it is consistant.  We have
places in which there was checks
	if (strlen(fstype) >= MFSNAMELEN || strlen(fspath) >= MNAMELEN)
and
	if (strlen(fstype) >= MFSNAMELEN - 1 || strlen(fspath) >= MNAMELEN - 1)
both with the same comment of "Be ultra-paranoid".  Unless something is
special they should have been the same and whatever is right should be
carried forward.  If there is a special case then it should be clearly
commented.  Since this check has moved into other code we need to get
it hashed out once and for all IMHO.  I mainly did this current change
to make sure attention is drawn to this for now until it is resolved.
 
| Second is, I suggest to make the mnt_path member a char *. Usually, the
| mount point path length is quite short, so 1024 bytes for the buffer is
| excessive. You can allocate exact needed buffer, which would save in
| around 10KB of kernel memory even for relatively modest amount of 10
| mounts.

Okay, I thought you wanted it a const char to potential guard against
some mis use of the field in that this should be a read only value.
I had actually planned to do the malloc since I was concerned about
if this structure got allocated on the stack then it could explode
the kernel's stack.  It seems most of the consumers access the mount
structure as a pointer so then I wasn't as concerned.
 
| For additional cleverness, mnt_path could point to f_mntonname when
| the length is less than MNAMELEN.  Since mp deallocation is centralized,
| code for the trick should be not too hard.

I'll look to see if I can change the other places that update mnt_path
to use the vfs_mount_alloc type function.  Since then we could get more
sophisticated about the mnt_path allocater/reference as you mention.
In nfs_mounroot.c it probably doesn't matter much since it should be a
short path but it could be more of an issue with zfs snapshots.

It looks like we are converging.  I'll make some more changes to make
sure we are getting on a good path port another patch.  Once that looks
okay in concept then I'll start looking into testing the various file
systems since unfortuanately it touches a lot of code even though it is
mostly mechanical.  I don't have a lot of time to work on this so I
want to optimize various things as once.  If someone can help unit test
corner cases that would be great with the various file systems.  Atleast
I have VirtualBox netbooting so I can test things quicker.  However,
that required some debugging and changes to pxeboot to send the Client ID
so isc-dhcpd didn't get upset with it.  I need to check that doesn't
break the non-ipxe boot stuff that doesn't require the Client ID field to
be set.  I've only run into this issue with ipxe in VirtualBox and qemu.
I also have some pxe boot robustness and caching fixes that I should
get in as well.

Thanks,

Doug A.