Re: [PATCH] vfs: move_mount: reject moving kernel internal mounts
On Tue, Jul 09, 2019 at 12:40:01PM -0700, Eric Biggers wrote: > On Tue, Jul 02, 2019 at 11:22:59AM -0700, Eric Biggers wrote: > > > > Sure, but the new mount syscalls still need tests. Where are the tests? > > > > Still waiting for an answer to this question. In samples/vfs/fsmount.c, IIRC, and that's not much of a test.
Re: [PATCH] vfs: move_mount: reject moving kernel internal mounts
On Tue, Jul 02, 2019 at 11:22:59AM -0700, Eric Biggers wrote: > > Sure, but the new mount syscalls still need tests. Where are the tests? > Still waiting for an answer to this question. Did we just release 6 new syscalls with no tests? I don't understand how that is even remotely acceptable. - Eric
Re: [PATCH] vfs: move_mount: reject moving kernel internal mounts
On Mon, Jul 01, 2019 at 07:22:39PM +0100, Al Viro wrote:
> On Mon, Jul 01, 2019 at 09:45:37AM -0700, Eric Biggers wrote:
> > On Sat, Jun 29, 2019 at 01:27:44PM -0700, Eric Biggers wrote:
> > >
> > > Reproducer:
> > >
> > > #include
> > >
> > > #define __NR_move_mount 429
> > > #define MOVE_MOUNT_F_EMPTY_PATH 0x0004
> > >
> > > int main()
> > > {
> > > int fds[2];
> > >
> > > pipe(fds);
> > > syscall(__NR_move_mount, fds[0], "", -1, "/",
> > > MOVE_MOUNT_F_EMPTY_PATH);
> > > }
> >
> > David, I'd like to add this as a regression test somewhere.
> >
> > Can you point me to the tests for the new mount syscalls?
> >
> > I checked LTP, kselftests, and xfstests, but nothing to be found.
>
> FWIW, it's not just move_mount(2) - I'd expect
>
> int fds[2];
> char s[80];
>
> pipe(fds);
> sprintf(s, "/dev/fd/%d", fds[0]);
> mount(s, "/dev/null", NULL, MS_MOVE, 0);
>
> to step into exactly the same thing. mount(2) does follow symlinks -
> always had...
Sure, but the new mount syscalls still need tests. Where are the tests?
Also, since the case of a fd with an internal mount was overlooked, probably the
man page needs to be updated clarify that move_mount(2) fails with EINVAL in
this case. Where is the man page?
- Eric
Re: [PATCH] vfs: move_mount: reject moving kernel internal mounts
On Mon, Jul 01, 2019 at 07:22:39PM +0100, Al Viro wrote:
> FWIW, it's not just move_mount(2) - I'd expect
>
> int fds[2];
> char s[80];
>
> pipe(fds);
> sprintf(s, "/dev/fd/%d", fds[0]);
> mount(s, "/dev/null", NULL, MS_MOVE, 0);
>
> to step into exactly the same thing. mount(2) does follow symlinks -
> always had...
The same goes for e.g.
#define _GNU_SOURCE
#include
#include
#include
#include
main()
{
char s[80];
unshare(CLONE_NEWNS); // so nobody else gets confused
sprintf(s, "/dev/fd/%d", epoll_create1(0));
mount(s, "/dev/null", NULL, MS_MOVE, 0);// see if it oopses
}
modulo error-checking, etc.
Re: [PATCH] vfs: move_mount: reject moving kernel internal mounts
On Mon, Jul 01, 2019 at 09:45:37AM -0700, Eric Biggers wrote:
> On Sat, Jun 29, 2019 at 01:27:44PM -0700, Eric Biggers wrote:
> >
> > Reproducer:
> >
> > #include
> >
> > #define __NR_move_mount 429
> > #define MOVE_MOUNT_F_EMPTY_PATH 0x0004
> >
> > int main()
> > {
> > int fds[2];
> >
> > pipe(fds);
> > syscall(__NR_move_mount, fds[0], "", -1, "/",
> > MOVE_MOUNT_F_EMPTY_PATH);
> > }
>
> David, I'd like to add this as a regression test somewhere.
>
> Can you point me to the tests for the new mount syscalls?
>
> I checked LTP, kselftests, and xfstests, but nothing to be found.
FWIW, it's not just move_mount(2) - I'd expect
int fds[2];
char s[80];
pipe(fds);
sprintf(s, "/dev/fd/%d", fds[0]);
mount(s, "/dev/null", NULL, MS_MOVE, 0);
to step into exactly the same thing. mount(2) does follow symlinks -
always had...
Re: [PATCH] vfs: move_mount: reject moving kernel internal mounts
On Sat, Jun 29, 2019 at 01:27:44PM -0700, Eric Biggers wrote:
>
> Reproducer:
>
> #include
>
> #define __NR_move_mount 429
> #define MOVE_MOUNT_F_EMPTY_PATH 0x0004
>
> int main()
> {
> int fds[2];
>
> pipe(fds);
> syscall(__NR_move_mount, fds[0], "", -1, "/",
> MOVE_MOUNT_F_EMPTY_PATH);
> }
David, I'd like to add this as a regression test somewhere.
Can you point me to the tests for the new mount syscalls?
I checked LTP, kselftests, and xfstests, but nothing to be found.
- Eric
Re: [PATCH] vfs: move_mount: reject moving kernel internal mounts
On Mon, Jul 01, 2019 at 02:08:48AM +0100, Al Viro wrote: > > Let's reorder that a bit: > /* The mountpoint must be in our namespace. */ > if (!check_mnt(p)) > goto out; > > /* The thing moved must be mounted... */ > if (!is_mounted(old_path->mnt)) > goto out; > > /* ... and either ours or the root of anon namespace */ > if (!(attached ? check_mnt(old) : is_anon_ns(ns))) > goto out; > > IMO that looks saner and all it costs us is a redundant check > in attached case. Objections? Looks good to me. - Eric
Re: [PATCH] vfs: move_mount: reject moving kernel internal mounts
On Mon, Jul 01, 2019 at 08:38:10AM +0100, David Howells wrote: > Al Viro wrote: > > > /* The thing moved must be mounted... */ > > if (!is_mounted(old_path->mnt)) > > goto out; > > Um... Doesn't that stuff up fsmount()? Nope - check is_mounted() definition. Stuff in anon namespace *is* mounted there, so that's not a problem. FWIW, is_mounted() would've been better off spelled as ns != NULL && ns != MNT_NS_INTERNAL; the use of IS_ERR_OR_NULL in there works, but is unidiomatic and I don't think it yields better code...
Re: [PATCH] vfs: move_mount: reject moving kernel internal mounts
Al Viro wrote: > /* The thing moved must be mounted... */ > if (!is_mounted(old_path->mnt)) > goto out; Um... Doesn't that stuff up fsmount()? David
Re: [PATCH] vfs: move_mount: reject moving kernel internal mounts
On Sat, Jun 29, 2019 at 09:39:16PM +0100, Al Viro wrote: > On Sat, Jun 29, 2019 at 01:27:44PM -0700, Eric Biggers wrote: > > > @@ -2600,7 +2600,7 @@ static int do_move_mount(struct path *old_path, > > struct path *new_path) > > if (attached && !check_mnt(old)) > > goto out; > > > > - if (!attached && !(ns && is_anon_ns(ns))) > > + if (!attached && !(ns && ns != MNT_NS_INTERNAL && is_anon_ns(ns))) > > goto out; > > > > if (old->mnt.mnt_flags & MNT_LOCKED) > > *UGH* > > Applied, but that code is getting really ugly ;-/ FWIW, it's too ugly and confusing. Look: /* The mountpoint must be in our namespace. */ if (!check_mnt(p)) goto out; /* The thing moved should be either ours or completely unattached. */ if (attached && !check_mnt(old)) goto out; if (!attached && !(ns && ns != MNT_NS_INTERNAL && is_anon_ns(ns))) goto out; OK, the first check is sane and understandable. But let's look at what's coming after it. We have two cases: 1) attached. IOW, old->mnt_parent != old. In that case we require old->mnt_ns == current mnt_ns. Anything else is rejected. 2) !attached. old->mnt_parent == old. In that case we require old->mnt_ns to be an anon namespace. Let's reorder that a bit: /* The mountpoint must be in our namespace. */ if (!check_mnt(p)) goto out; /* The thing moved must be mounted... */ if (!is_mounted(old_path->mnt)) goto out; /* ... and either ours or the root of anon namespace */ if (!(attached ? check_mnt(old) : is_anon_ns(ns))) goto out; IMO that looks saner and all it costs us is a redundant check in attached case. Objections?
Re: [PATCH] vfs: move_mount: reject moving kernel internal mounts
On Sat, Jun 29, 2019 at 01:27:44PM -0700, Eric Biggers wrote: > @@ -2600,7 +2600,7 @@ static int do_move_mount(struct path *old_path, struct > path *new_path) > if (attached && !check_mnt(old)) > goto out; > > - if (!attached && !(ns && is_anon_ns(ns))) > + if (!attached && !(ns && ns != MNT_NS_INTERNAL && is_anon_ns(ns))) > goto out; > > if (old->mnt.mnt_flags & MNT_LOCKED) *UGH* Applied, but that code is getting really ugly ;-/
