Date:	Fri, 04 Oct 2013 17:03:23 -0700
From:	ebiederm@...ssion.com (Eric W. Biederman)
To:	Linus Torvalds <torvalds@...ux-foundation.org>
Cc:	Miklos Szeredi <miklos@...redi.hu>,
	"Serge E. Hallyn" <serge@...lyn.com>,
	Al Viro <viro@...iv.linux.org.uk>,
	Linux-Fsdevel <linux-fsdevel@...r.kernel.org>,
	Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Andy Lutomirski <luto@...capital.net>,
	Rob Landley <rob@...dley.net>
Subject: Re: [RFC][PATCH 0/3] vfs: Detach mounts on unlink.

Linus Torvalds <torvalds@...ux-foundation.org> writes:

> On Fri, Oct 4, 2013 at 3:41 PM, Eric W. Biederman <ebiederm@...ssion.com> wrote:
>>
>> After thinking about it removing the restrictions on mount points
>> appears safe, because it is just plain dumb to have a mount point
>> in a directory that is not restricted to root only modifications.
>>
>> This is a change in user visible semantics, so I want to be very careful
>> about this.  Are there any reasons to not make this change?
>
> At least one worry: people are very used to 'rmdir()' not removing
> empty directories, and I've written code myself that just does an
> 'rmdir()' without worrying about it. I think git has code like "remove
> file, then try to remove directory file is in, and recurse until it
> fails or you hit the top of tree". And it all depends on knowing that
> rmdir() is harmless, and returns the appropriate error when the
> directory isn't empty.
>
> And you're now changing that. If it's a mount-point, the rmdir just
> succeeds, afaik.
>
> Does anybody care? I dunno. But it looks like a _big_ semantic change.

Which is definitley why I am asking and being careful.

> That said, I like the _concept_ of being able to remove a mount-point
> and the mount just goes away. But I do think that for sanity sake, it
> should have something like "if one of the mounts is in the current
> namespace, return -EBUSY". IOW, the patch-series would make the VFS
> layer _able_ to remove mount-points, but a normal rmdir() when
> something is mounted in that namespace would fail in order to give
> legacy behavior.
>
> Hmm?

In principle I have no problems tweaking rmdir to check for that case.

At the same time the real reason that this is safe is that mount points
are almost always part of trusted paths to important files and you just
don't mess with those paths.

So tweaking rmdir to fail would be more about making stupid mistakes
like running "rm -rf /" fail than it would be about security or
correctness.

My intuition agrees with yours that it is less surprising if rmdir was
forbidden in mount namespaces where it has a mount.  Root would still be
able to do crazy things like:

# unshare --mount /bin/bash
# umount -l /sys
# mv /proc /sys
# exit
#
# ls /proc
ls: cannot access /proc No such file or directory
#
# ls /sys
1 2 3 4 acpi/ asound/ buddyinfo bus/ cmdline config.gz consoles cpuinfo
crypto devices diskstats dma dir/ driver/ execdomains fb filesystems
fs/ interrupts iomem ioports irq/ kallsyms kcore key-users kmsg
kpagecount kpageflags loadavg locks mdstat meminfo misc modules mounts/
mtrr pagetypeinfo partitions schedstat scsi/ self/ slabinfo softirqs
stat swaps sys/ sysrq-trigger sysvipc/ timer_list tty/ uptime version
vmallocinfo vmstat zoneinfo
#


I just noticed that Al's latest vfs changes posted yesterday mean I need
to rebase and possibly respin these patches, as all of the locking and
interesting bits of the dcache have changed.  I don't think the
conflicts would be fun to resolve during the merge window.

Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists