Regression wrt mounting /proc in user namespace in 3.13

[Eric W. Biederman]

"Serge E. Hallyn" <serge at hallyn.com> writes:

> Quoting Serge E. Hallyn (serge at hallyn.com):
>> Quoting Gao feng (gaofeng at cn.fujitsu.com):
>> > On 11/18/2013 11:19 AM, Serge E. Hallyn wrote:
>> > > Quoting Serge E. Hallyn (serge at hallyn.com):
>> > >> Low on power and no charger, but a quick test printing out if a mount is
>> > >> !S_ISDIR or has nlink !=2 in fs_fully_visible() gives me:
>> > >>
>> > >> [   92.939650] nlink is 1 for ino 8733 (0:3)
>> > >>
>> > >> (that's major 0 minor 3)
>> > > 
>> > > Ok, so that is for binfmt_misc on /proc/sys/fs/binfmt_misc.  The
>> > > underlying directory is empty, and nlink is showing up as 1.
>> > >  
>> > > Can we just get the nlink check changed to check for < 3 instead
>> > > of ==2 ?
>> > > 
>> > 
>> > I already reported this problem to Eric,hi is working on fix this problem.
>> > 
>> > nlink is not the right thing to check if a directory is null. since
>> > in all of filesystems, parent dir's nlink is increase only when we
>> > create sub-dir.
>> 
>> This whole thing feels very brittle.  May I also point out that simply
>> setting perms appears to work just fine instead of overmounting.  If I
>> chmod 700 /proc/swaps, unshare my pid and mount namespaces and remount
>> /proc, then /proc/swaps is 700 in the new mount.  Since our concern is
>> with a new user namespace, which will be limited to world perms, this
>> should suffice and allow us to skip all this nonsense.
>> 
>> Eric?
>> 
>> -serge
>
> So yeah, I think this patch should be reverted, rather than "fixed".

I will get back to this shortly.   The trivial fix is to kill the nlink
test or make the nlink test look for nlink <= 2.

The point of the original patch was to remove the horrible special case
that implemented the test to see if proc was moutned, so that we could
trivial prevent mount of proc in chroots or other legacy environments
where root did not want proc to be mounted.

The refinement I attempted and failed was to verify that people had only
mounted on empty proc directories.  Just to verify that nothing becomes
possible when mounting a new copy of proc that wasn't possible before.

My test is totally for that last case is totally braindead.  Permissions
are nice but the actual concern is setups that did silly things that are
not namespace aware.

I am totally against reverting the cleanup.  But I have no problem
killing the nlink test.  Because we did not have protection against
overmounted directories earlier.

Eric