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

From: Miklos Szeredi
Date: Thu Apr 24 2008 - 10:10:27 EST


> > 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.

I lost you there, sorry. Can you please rephrase a bit less
abstractedly?

>
> > > 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.

They do. Specific counterexamples please.

> > > 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".

Sure.

> 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.

So what? All the other checks are also duplicated within
vfs_create()->may_create()->permission().

> 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.

I do very much realize that.

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

Specifics please. I don't see any reason why the brackets would need
to be stretched (except to make make multiple vfs calls atomic wrt r/o
remount).

> 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".

Sure.

> > > 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.

Umm, isn't it? Want to redo open() without a vfsmount?

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