Re: NFS4 crack

["J. Bruce Fields" <bfields@fieldses.org>]

On Mon, Sep 19, 2005 at 12:35:47PM +0200, Christoph Hellwig wrote:
> On Sun, Sep 18, 2005 at 10:36:15AM -0400, J. Bruce Fields wrote:
> > On Sun, Sep 18, 2005 at 12:21:00PM +0200, Christoph Hellwig wrote:
> > > The fs handling in fs/nfs/nfs4recovery.c is rather broken in addition.
> > 
> > For example?
> 
>  - opens a directory O_RDWR which open_namei wouldn't even allow

OK, thanks, fixed locally.

>  - tries to build dentry list from vfs_readdir callback, leading to
>    deadlocks on filesystems that take the same lock from readdir
>    and lookup

So it appears that nfsd has long made the requirement that filesystems
not do this.  Does this need to be documented somehwere?

>  - resets fsuid/fsgids without checks, synchronization or callouts
>    into subsystems that care (security, keys, ptrace)

I think the model here was nfsd_setuser(), which does essentially the
same thing.  Is this an nfsd bug?

>  - looks up /var/lib/nfs/v4recovery without ensuring it's a directory

Oops, thanks.

> and probably a few more if one tried to look at it for more than five
> minutes.  This is code that could be a third of the size if written
> in userpsace and actually had a chance to be correct there, nevermind
> the policy violations.

That's a couple good bugs identified, thanks, but I'm not convinced that
this would be significantly simpler from userspace.

We'd need two pieces of user<->kernel interface:

	1. An upcall to userspace to tell it about new client state.  We
	   also need to be able to wait for userspace to commit something
	   to disk, as the information has to survive a reboot.
	2. A way for userspace to dump recorded state to the kernel the
	   next time nfsd starts up.

Number 1 could be done with something like hotplug, I guess.  (It can be
told to wait for the userspace helper to exit, right?)

Another file in the nfsd filesystem might work for the second interface.

We also considered accomplishing number 1 by appending records to a log
file.  Userspace could hand in a file descriptor to use for this
purpose.  We'd still need the second interface.

--b.