Date: Mon, 25 Nov 2013 10:29:19 +0200 From: Konstantin Belousov <kostikbel@gmail.com> To: Doug Ambrisko <ambrisko@ambrisko.com> Cc: freebsd-hackers@freebsd.org, Dirk Engling <erdgeist@erdgeist.org>, Jase Thew <jase@freebsd.org>, mdf@freebsd.org Subject: Re: Re: Fix MNAMELEN or reimplement struct statfs Message-ID: <20131125082919.GO59496@kib.kiev.ua> In-Reply-To: <20131122170419.GA60910@ambrisko.com> References: <52854161.6080104@FreeBSD.org> <20131115010854.GA76106@ambrisko.com> <20131116183129.GD59496@kib.kiev.ua> <20131118190142.GA28210@ambrisko.com> <20131119074922.GY59496@kib.kiev.ua> <20131119174216.GA80753@ambrisko.com> <20131120075531.GE59496@kib.kiev.ua> <20131121174028.GA80520@ambrisko.com> <20131122074228.GT59496@kib.kiev.ua> <20131122170419.GA60910@ambrisko.com>
next in thread | previous in thread | raw e-mail | index | archive | help
--HjlUprwJuNajQ6MQ Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Nov 22, 2013 at 09:04:19AM -0800, Doug Ambrisko wrote: > 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 d= on't > | > | > have the check to see if the mount is longer then VFS_MNAMELEN (i= n their case) > | > | > and just truncate things. > | > | >=20 > | > | > 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 tha= t. > | > | > | > | 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, a= nd > | > | this could be postponed if you prefer to minimize the patch. > | >=20 > | > 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 update= d as > | > well and made the length check more consistant. Some were doing >=3D= 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 len= gth. > | > 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 > | > =09 > | > Just in case it doesn't make it in email the full patch is at: > | > http://people.freebsd.org/~ambrisko/mount_bigger.patch > | >=20 > | Yes, this is closer to the patch I can agree with :). > |=20 > | Two notes, one was already made, about off by one. >=20 > The off by one, I want to revisit so that it is consistant. We have > places in which there was checks > if (strlen(fstype) >=3D MFSNAMELEN || strlen(fspath) >=3D MNAMELEN) > and > if (strlen(fstype) >=3D MFSNAMELEN - 1 || strlen(fspath) >=3D 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. > =20 > | 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. >=20 > 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. const char * is fine, but is usually quite hard to use. Kernel is hostile to constness, since lot of older functions, while accessing the arguments read-only, were written before const was invented. > 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. Are there any instance in the kernel where the structure is not allocated through vfs_mount_alloc() ? I am not aware of such case. > =20 > | 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. >=20 > 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. >=20 > 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. Well, the changes to most filesystems are trivial, so I would not gate the patch with testing of rarely-used and mostly rotten filesystems. --HjlUprwJuNajQ6MQ Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.22 (FreeBSD) iQIcBAEBAgAGBQJSkwpeAAoJEJDCuSvBvK1BtqkP/i+p6tKqhFng9uoOct1BvMM+ 5uobMKFKRb/0r52+J1SkjnQd62+Ck54jIiDRxTmtsymeiztwm01qvgrDjKr4Fe1K hXeBuc0OOqJyQ7Upr4XDkWN0RCtwuCz1Y/06n28x822uFmU6GKSlM6y+Gy/xQ0bN wJk8Zj9fDXGHlBmQKE+zCUOAAd6JUiiNhve90ooUHGAR108z4m5+57WpLCiH4Vvx FQgA+oJlRLwpMLGPHSKFMYX2dwXZzlxgoToetx8iCnPQrazd3Z42/gDfwZM7ndRj 06f9Y1DLx698ciNSxptQOlpbcggoracHNoi6gDjp9ix03OtheDTgIMZNU+NnRn02 6wihf2+sDPZ4NGenMKlG3OAd0Vmrptc/prKN4fy1sC5fiyxgZr1zjQK09ul6SVOC p1so6SBtIkCb5BUqv5vWfl7COpXrwNHjQgKcqMJBhZECheH5u/NhYTl2Rv1HlNqJ /OhOBbKxggf72pBIt9KXg0uYWqhfKs5/MpySOg659cAaplFijSurGyrQm8Tu5PYZ 7oHYePb2XzE5ESq66MgEYVBw3pJPUIUV0OqkCHxeuSIMQD0vPDHsdvxgTGm/bThc EyRY1MLXEGeWKyP+7cn7vvrubZa21v3mRLRC90wHt8bzl+p2AygOJ5Fm9qpj2YND 7CzHWz1U4JuiHF8+oUDd =NVf9 -----END PGP SIGNATURE----- --HjlUprwJuNajQ6MQ--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20131125082919.GO59496>