Date: Tue, 19 Nov 2013 09:49:22 +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: <20131119074922.GY59496@kib.kiev.ua> In-Reply-To: <20131118190142.GA28210@ambrisko.com> References: <51B3B59B.8050903@erdgeist.org> <CAMBSHm8GMWffuuEcSpuNu26Mv4N2yAa2iEdw5koiXx0w30zPRQ@mail.gmail.com> <201306101152.17966.jhb@freebsd.org> <52854161.6080104@FreeBSD.org> <20131115010854.GA76106@ambrisko.com> <20131116183129.GD59496@kib.kiev.ua> <20131118190142.GA28210@ambrisko.com>
next in thread | previous in thread | raw e-mail | index | archive | help
--rmCyz0CRE2AtwE2l Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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. >=20 > 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 ? > =20 > | 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. >=20 > 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: >=20 > + mount 192.168.38.1:/data/home/ambrisko/netboot /data/jail/test > + jail -i -c name=3Dtest path=3D/data/jail/test host.hostname=3Dtest.ambr= isko.com persist enforce_statfs=3D0 allow.mount=3D1 allow.mount.devfs=3D1 a= llow.mount.nullfs=3D1 allow.mount.tmpfs=3D1 allow.mount.procfs=3D1 > 14 > + jexec test mkdir -p /12345678901234567890123456789012345678901234567890= 12345678901234567890123456789012345678901234567890/proc > + jexec test mkdir -p /12345678901234567890123456789012345678901234567890= 12345678901234567890123456789012345678901234567890/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 //1234567890123456789012345678901234567= 890123456789012345678901234567890123456789012345678901234567890/proc > + jexec test mount -t devfs null //12345678901234567890123456789012345678= 90123456789012345678901234567890123456789012345678901234567890/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/1234567890123456789012345678901234567890123456789= 0123456789012345678901 > devfs 2 2 0 = 100% /data/jail/test/1234567890123456789012345678901234567890123456789= 0123456789012345678901 > + 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/12345678901234567890123456789012345678901234567= 890123456789012345678901 (procfs, local, fsid 26ff000202000000) > devfs on /data/jail/test/123456789012345678901234567890123456789012345678= 90123456789012345678901 (devfs, local, multilabel, fsid 27ff007171000000) > + jexec test umount /1234567890123456789012345678901234567890123456789012= 345678901234567890123456789012345678901234567890/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/1234567890123456789012345678901234567890123456789= 0123456789012345678901 > + jexec test umount /1234567890123456789012345678901234567890123456789012= 345678901234567890123456789012345678901234567890/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. >=20 > | 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. >=20 > 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. >=20 > 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. --rmCyz0CRE2AtwE2l Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.22 (FreeBSD) iQIcBAEBAgAGBQJSixgBAAoJEJDCuSvBvK1B1S4P/ilstNB84CaFfE1r+III3xkU 1X7eekSDOOz2A98mvW2pr4cqusf6Xx2J/feSIoKTqWs9yBcfGZjZt9l+i5d0C5t3 8JayjS/1sEYnbns1w4C34LoiVOHBRN1VAwS9XKQDcAvEcIpHFrYHxChHlkpsxRe8 +cz5hl2U9gRS6RjKHQJpC5OyskhMwXqjbbvJsvo37YEk0mYPAS9HvjBGilNgTph4 e9/a/ophP0AOF72KSMgaat5WT+37+x/ja6wBz+I3GWXjz0QgueuK/TIj/f3NFQpI pJwYwwtkvZ4pcxv1ELv4ZwShHGRpI5HiUbRw9M1dTJgy3vTQ3pOWTxKqkH8XGkSq N+vyww2toBBSCjL++UOaIaq7YPamR7jbeu2cNyQAbm8xh8fxwzZRmjOo1EevHR3r NfRkjLhlQYszbR7LPmEyfZBYkJUAUivkXlYqhszQ0H5usUk+lBa9PzblMvrO3XU+ o2oA+aqGioRZmm9JlwKsqIIYgA8aQyZzAxRDOrgDurDxtD4fUyTNks78mIocwse6 n9hvLTXCED9Oc7OPW8rBnyetGLX0YpCBsoN/E+1TCOOkuZHvmG78LY1Ofp8yB8zK EE66xfKpw6K7sQNSH6fsnbC/U9i1t9QVvmPG0UHKllR8yYAE/yJu86NUdytYWJRS JvTzu0zVvmwXjvlggau5 =OSsC -----END PGP SIGNATURE----- --rmCyz0CRE2AtwE2l--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20131119074922.GY59496>