Re: [patch 00/13] vfs: add helpers to check r/o bind mounts

[Al Viro <viro@ZenIV.linux.org.uk>]

On Thu, Apr 24, 2008 at 03:05:21PM +0200, Miklos Szeredi wrote:
> Several calls to nfsd_setattr() for starters.  But I didn't do a full
> audit of all vfs_* callers, there might well be others.
> 
> > > I think it's totally pointless to continue trying to fix the symptoms
> > > instead of getting at the root of the problem.
> > > 
> > > I know that VFS interfaces are a sensitive question, but it would be
> > > nice it we could have some sanity back in this discussion.
> > 
> > Yes, it would.  How about that, for starters:
> > 
> > path_create() et.al. are *wrong* for nfsd;
> 
> Why are they wrong?  The performance impact is negligible, the code is
> not any more complicated.

Because you are mixing the "this sucker will be used for write access for
this interval" and "do what is needed to create a file".  The latter is
not guaranteed to coincide with the former and that in itself is enough.

> > if nothing else, I'm not at
> > all convinced that even apparmour wants export path + relative there
> > _and_ r/o vs. r/w is decision that doesn't clearly map to ex_mnt flags.
> 
> I don't care what apparmor wants.  What I care about is consistency of
> the thing.  If _anything_ calls into the filesystem, the same security
> hooks should be called and the same mount flags should be checked.

_IF_ they make sense for call in question.  At the level where they
are applied.

> > soon as vfs_...() is over in case of nfsd.  Some of the stuff done
> > immeidately afterwards might very well qualify for inclusion into
> > protected area; some of the stuff done immediately _prior_ very likely
> > needs that as well - look at fh_verify() and tell me why we don't want
> > that "I'll hold write access to vfsmount" to span the area including
> > that sucker.
> 
> I don't see anything in fh_verify() or after which modifies the
> filesystem.  The purpose of the r/o checks is to prevent modification,
> and nothing more. 

Bullshit.  It's not just "prevent modification".  It's "make sure that
no remount r/o happens while we do that".  fh_verify() doesn't modify.
It does check, though, and later we have that check duplicated by
will_write/wont_write pair bracketing a part of sequence.

Please, realize that spot checks like that are inherently racy and that's
the problem we had all along with r/o remounts et.al.

And that's why they got split in will/wont pairs and stretched to cover
relevant areas.  Areas that depend on specific callers.

And yes, we need the counterpart for superblock-level stuff, to deal with
remaining races (look at fs_may_remount_ro() and puke - it's still racy
as hell).  E.g. unlink should do sb-level "will write" when it drops
i_nlink to 0 and final removal of inode should do "won't write".

> > For ecryptfs it's also bogus - at the very least we need to decide what
> > should happen when underlying vfsmount is remounted.  Again, I'm less
> > than convinced that we want the same way to express r/o vs. r/w policy.
> 
> We can add whatever policy we want.  The path_ interface doesn't stop
> you from having sane r/o semantics on ecryptfs.  What it does is to
> make sure that the r/o rules are _always_ followed, regardless of any
> policy or lack thereof in the callers.

ecryptfs should not use the bloody vfsmount, for fuck sake!  You are confusing
access to fs with access to fs via specific vfsmount.  And pretending that
the latter is fundamental operation.  It isn't.  Flags on vfsmounts *do*
control it.  But the same operations make sense without any vfsmounts involved.
At all.  And "so let's invent some and express our access control rules by
tweaking its flag" is a kludge, not a sane answer.