Date:	Fri, 21 Mar 2008 21:23:39 +0100
From:	Miklos Szeredi <miklos@...redi.hu>
To:	viro@...IV.linux.org.uk
CC:	miklos@...redi.hu, haveblue@...ibm.com,
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
	neilb@...e.de, akpm@...ux-foundation.org, hch@...radead.org,
	linux-security-module@...r.kernel.org, jmorris@...ei.org
Subject: Re: r-o bind in nfsd

> > > > > And having the vfsmount available within vfs_...() functions means,
> > > > > that the mnt_want_write() check can be moved inside, which means that
> > > > > callers get simpler and less likely to be buggy.  Those are all
> > > > > advantages IMO, regardless of any security module issues.
> > > > 
> > > > Or we can introduce another set of exported functions (path_mkdir(),
> > > > ...), and leave vfs_...() alone.  And then the only question is if
> > > > LSM's can live with ordering change.
> > > 
> > > I really don't see the point of new helpers; especially since one doesn't
> > > have to _have_ vfsmount to use the old ones and since we don't have a lot
> > > of users of each of those to start with.
> > 
> > Traditionally we have syscalls, and nfsd.  Both of them want the
> > security checks, and I think nfsd wants the read-only mount checking
> > as well, but I'm not entirely sure.  Maybe we can handle that by just
> > making nfsd acquire a write-ref on the mount and keep it while it's
> > exported.
> > 
> > Then there's ecryptfs and unionfs, which probably need neither, but it
> > wouldn't hurt to do them anyway.
> > 
> > Still, even if there are only two callers, then moving stuff to up
> > doesn't make any sense.  Passing down a struct path is free for the
> > syscall case, it doesn't consume any stack space or extra CPU.  Do
> > please tell, why would that be such a bad thing?
> 
> Because we'd been that way before; see the shitpiles around ->lookup()
> getting nameidata, etc.  You'll end up with some callers passing NULL
> as ->mnt since they don't have anything better to pass, some stuff
> called *from* the damn thing caring to check for ->mnt being NULL,
> some stuff not caring about what ->mnt is at all and some assuming
> that it's not NULL.  Which will lead to exploding combinations that
> won't be noticed until somebody steps into such config.

Right, we do want to prevent that happening.

And for example moving read-only mount checks inside vfs_...() would
ensure that.

> As for the vfsmount-dependent checks (and any kind of MAC, while we are
> at it)...  They belong to callers, exactly because different callers may
> want different (amount of) checks.

And we end up random callers forgetting some of the checks, like we
have now with nfsd.  Not good at all.

I think it's still a lot better all the checks are always done, even
if not strictly necessary for a certain caller, than if the caller has
to make sure the necessary ones do get done.

Assuming of course, that all valid users _do_ have the vfsmount
available, which I think is true.  If you have a counterexample,
please let us know.

If not all (but most) callers have the vfsmount available, then a new
helper makes sense.

If there was only one caller which needed a certain check, then moving
that into the caller would be the right thing of course.  But that's
not the case here.

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