Re: [PATCH] ncpfs: fix rmdir returns Device or resource busy

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

On Thu, Jun 13, 2013 at 03:01:22AM +0100, Al Viro wrote:
> On Fri, Jun 07, 2013 at 05:14:52PM +0100, Al Viro wrote:
> > On Fri, Jun 07, 2013 at 11:09:05AM -0500, Dave Chiluk wrote:
> > > Can't you just use the patch from my original e-mail?  Anyhow I attached
> > > it an already signed-off patch.
> > > 
> > > Al Viro Can you integrate it now?
> > 
> > Applied...  FWIW, patch directly in mail body is more convenient to deal with.
> 
> Actually, looking at that stuff...  Why are we bothering with -EBUSY for
> removal of busy directories on ncpfs, anyway?  It's not just rmdir(), it's
> overwriting rename() as well.  IS_DEADDIR checks in fs/namei.c and fs/readdir.c
> mean that the only method of ncpfs directories that might get called after
> successful removal is ->setattr() and it would be trivial to add the check
> in ncp_notify_change() that would make it fail for dead directories without
> bothering the server at all...
> 
> Related question: what happens if you open / unlink / fchmod on ncpfs?

Speaking of crap used only by ncpfs: I think we can use ->d_iput() to get rid
of d_validate() for good.  The only remaining user is ncpfs; what happens there
is that we use the page cache of directory to cache the references to dentries
made by readdir.  We could do the following trick:
	* have ->d_fsdata for these dentries a pointer into the cache page where
the reference back to dentry is stored
	* ->freepage() for those pages consisting of
		grab global spinlock
		go through all dentries still pointed to by pointers in that
page, zeroing ->d_fsdata
		drop the spinlock
	* ->d_iput() for those dentries consisting of
		grab the same spinlock
		if ->d_fsdata is non-zero, store NULL at the address pointed
to by it
		drop the spinlock
	* ncp_dget_fpos() would
		grab that spinlock
		check if the reference to dentry in the position we are
interested in is non-NULL
			grab ->d_lock
			if DCACHE_DENTRY_KILLED is not set
				bump ->d_count
				drop ->d_lock
				drop the spinlock
				return dentry
			// dentry is doomed
			clear the reference
			drop ->d_lock
		drop the spinlock
		return NULL
	* ncp_fill_cache() would insert the sucker into cache and set
->d_fsdata under the same spinlock.

IOW, instead of wanking with untrusted pointers to dentries, we simply make
sure we clean the pointer when dentry is going away and clean the reference
from dentry to the location of that pointer when the page is going away.

Objections?  I can do a patch along those lines, but I've nothing to test it
on.  Had that been cifs, I could at least use samba to test the fucker, but
I've no idea how to do that with ncpfs and I'm not too fond of checking how
much bitrot has mars_nwe suffered...