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

From: Al Viro
Date: Thu Jun 13 2013 - 02:43:04 EST


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