[PATCH review 0/7] Bind mount escape fixes

Eric W. Biederman ebiederm at xmission.com
Sun Aug 16 11:33:21 UTC 2015

Al Viro <viro at ZenIV.linux.org.uk> writes:

> On Sun, Aug 16, 2015 at 01:22:41AM -0500, Eric W. Biederman wrote:
>> In my last round of patches that I sent out today.  I did put mount_lock
>> just outside of rename_lock, in d_splice_alias.  But apparently you
>> haven't noticed.
> I have.  The problem I have with that one is that you end up with duplicated
> logics rather than taking it to one place.

We are talking about a very small duplication of code.  But you are also
talking about combining logic that can get you in trouble pretty easily.

As I read the code sketch you posted it had the issue that if a
disconnected dentry was also a mount point it would mark that mount
point as escaped (because there was no common ancestor).

In the split out logic I don't even bother because you trivially can't
have escaped from anywhere when IS_ROOT(dentry).

>> Now at this point I have hit the limit of my time available for rewrites
>> before the merge window.  We can go with my 7 patch variant I posted
>> today (whose only sin appears not to be your implemenation), it's
>> trivial reduction that Linus likes because it is simple, someone else
>> can write one, or this can all wait until the next development cycle.
> ... or either of us can do merging those checks into a single place,
> be it as a followup to your 7-patch series, or folded with the
> fs/dcache.c-affecting patches in there.  If you have no time left, I can
> certainly do that followup myself - not a problem[1]

I don't have time.  Everytime I have worked with this it has take pretty
much full days of staring at the code, and I don't have any more full
days left before the merge window.

> And umount-related followups are just that - I'm not asking you to do those,
> especially since as I said this stuff is sensitive to fs_pin details (so far
> it appears to fold nicely with the __detach_mounts()/umount_tree() stuff,
> BTW).
> PS: it doesn't need namespace_sem taken inside __d_move() - actual
> detaching does, of course, but that part gets done via task_work_add(),
> in a reasonably sane locking environment.

The part that boggles my mind about that whole approach is that just
outside of rename_lock there already is a sane locking environment.

Admittedly it will take a touch of work in d_splice_alias to get it
sane, but that isn't particularly hard.

        if (!IS_ROOT(new))

        /* Parent's don't go away so !IS_ROOT(new) will stay valid */
        /* Hooray! The code can be normal and sleep if it needs to,
         * before calling into __d_move.

Similarly for duplicate logic removal.  Teaching d_splice_alias when
it is performing an ordinarly rename to be able to just call d_move
gets you far more from a maintenance perspective than cramming
everything into __d_move.

> [1] with credits for your patches preserved - normally I would assume that
> this goes without saying, but your reply seems to imply that I'm playing some
> kind of political BS games, so I'd rather spell that out.

My issue wasn't political games, but rather holding code to an
apparently BS standard.


More information about the Containers mailing list