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

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

[Dave Chiluk]

1d2ef5901483004d74947bbf78d5146c24038fe7 caused a regression in ncpfs such that
directories could no longer be removed.  This was because ncp_rmdir checked
to see if a dentry could be unhashed before allowing it to be removed. Since
1d2ef5901483004d74947bbf78d5146c24038fe7 introduced a change that incremented
dentry->d_count causing it to always be greater than 1 unhash would always
fail.  Thus causing the error path in ncp_rmdir to always be taken.  Removing
this error path is safe as unhashing is still accomplished by calls to dput
from vfs_rmdir.
---
 fs/ncpfs/dir.c |    9 ---------
 1 file changed, 9 deletions(-)

diff --git a/fs/ncpfs/dir.c b/fs/ncpfs/dir.c
index 8163260..6792ce1 100644
--- a/fs/ncpfs/dir.c
+++ b/fs/ncpfs/dir.c
@@ -1029,15 +1029,6 @@ static int ncp_rmdir(struct inode *dir, struct dentry *dentry)
 	DPRINTK("ncp_rmdir: removing %s/%s\n",
 		dentry->d_parent->d_name.name, dentry->d_name.name);
 
-	/*
-	 * fail with EBUSY if there are still references to this
-	 * directory.
-	 */
-	dentry_unhash(dentry);
-	error = -EBUSY;
-	if (!d_unhashed(dentry))
-		goto out;
-
 	len = sizeof(__name);
 	error = ncp_io2vol(server, __name, &len, dentry->d_name.name,
 			   dentry->d_name.len, !ncp_preserve_case(dir));
-- 
1.7.9.5

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

[Dave Chiluk]

Any thoughts on this?  NCPFS seems to be the forgotten, left-behind,
red-headed stepchild of the fs community.

Dave.

On 05/28/2013 05:50 PM, Dave Chiluk wrote:
> 1d2ef5901483004d74947bbf78d5146c24038fe7 caused a regression in ncpfs such that
> directories could no longer be removed.  This was because ncp_rmdir checked
> to see if a dentry could be unhashed before allowing it to be removed. Since
> 1d2ef5901483004d74947bbf78d5146c24038fe7 introduced a change that incremented
> dentry->d_count causing it to always be greater than 1 unhash would always
> fail.  Thus causing the error path in ncp_rmdir to always be taken.  Removing
> this error path is safe as unhashing is still accomplished by calls to dput
> from vfs_rmdir.
> ---
>  fs/ncpfs/dir.c |    9 ---------
>  1 file changed, 9 deletions(-)
> 
> diff --git a/fs/ncpfs/dir.c b/fs/ncpfs/dir.c
> index 8163260..6792ce1 100644
> --- a/fs/ncpfs/dir.c
> +++ b/fs/ncpfs/dir.c
> @@ -1029,15 +1029,6 @@ static int ncp_rmdir(struct inode *dir, struct dentry *dentry)
>  	DPRINTK("ncp_rmdir: removing %s/%s\n",
>  		dentry->d_parent->d_name.name, dentry->d_name.name);
>  
> -	/*
> -	 * fail with EBUSY if there are still references to this
> -	 * directory.
> -	 */
> -	dentry_unhash(dentry);
> -	error = -EBUSY;
> -	if (!d_unhashed(dentry))
> -		goto out;
> -
>  	len = sizeof(__name);
>  	error = ncp_io2vol(server, __name, &len, dentry->d_name.name,
>  			   dentry->d_name.len, !ncp_preserve_case(dir));
> 

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

[Dave Chiluk]

Petr do you still have commit rights to ncpfs?  Can you please commit it
to upstream or do I have to get Al to do that?

Dave.


On 05/31/2013 05:22 PM, Petr Vandrovec wrote:
> Looks OK to me.
> 
> As I said elsewhere, I do not use ncpfs for years so I cannot provide
> any sensible maintainership for it anymore :-(
> 
> Petr
> 
> On May 31, 2013 2:40 PM, "Dave Chiluk" <chiluk@canonical.com
> <mailto:chiluk@canonical.com>> wrote:
> 
>     Any thoughts on this?  NCPFS seems to be the forgotten, left-behind,
>     red-headed stepchild of the fs community.
> 
>     Dave.
> 
>     On 05/28/2013 05:50 PM, Dave Chiluk wrote:
>     > 1d2ef5901483004d74947bbf78d5146c24038fe7 caused a regression in
>     ncpfs such that
>     > directories could no longer be removed.  This was because
>     ncp_rmdir checked
>     > to see if a dentry could be unhashed before allowing it to be
>     removed. Since
>     > 1d2ef5901483004d74947bbf78d5146c24038fe7 introduced a change that
>     incremented
>     > dentry->d_count causing it to always be greater than 1 unhash
>     would always
>     > fail.  Thus causing the error path in ncp_rmdir to always be
>     taken.  Removing
>     > this error path is safe as unhashing is still accomplished by
>     calls to dput
>     > from vfs_rmdir.
>     > ---
>     >  fs/ncpfs/dir.c |    9 ---------
>     >  1 file changed, 9 deletions(-)
>     >
>     > diff --git a/fs/ncpfs/dir.c b/fs/ncpfs/dir.c
>     > index 8163260..6792ce1 100644
>     > --- a/fs/ncpfs/dir.c
>     > +++ b/fs/ncpfs/dir.c
>     > @@ -1029,15 +1029,6 @@ static int ncp_rmdir(struct inode *dir,
>     struct dentry *dentry)
>     >       DPRINTK("ncp_rmdir: removing %s/%s\n",
>     >               dentry->d_parent->d_name.name <http://d_name.name>,
>     dentry->d_name.name <http://d_name.name>);
>     >
>     > -     /*
>     > -      * fail with EBUSY if there are still references to this
>     > -      * directory.
>     > -      */
>     > -     dentry_unhash(dentry);
>     > -     error = -EBUSY;
>     > -     if (!d_unhashed(dentry))
>     > -             goto out;
>     > -
>     >       len = sizeof(__name);
>     >       error = ncp_io2vol(server, __name, &len, dentry->d_name.name
>     <http://d_name.name>,
>     >                          dentry->d_name.len, !ncp_preserve_case(dir));
>     >
> 

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

[Petr Vandrovec]

On Wed, Jun 5, 2013 at 1:20 PM, Dave Chiluk <chiluk@canonical.com> wrote:
> Petr do you still have commit rights to ncpfs?  Can you please commit it
> to upstream or do I have to get Al to do that?

Hi,
  only thing I can do is to add

Signed-off-by: Petr Vandrovec <petr@vandrovec.name>

on your patch and forward it to Al.  Unfortunately patch below is
already whitespace-damaged :-(  So you can either send it to me, and
I'll resend it to Al, or you can do that directly...

Petr

> Dave.
>
>
> On 05/31/2013 05:22 PM, Petr Vandrovec wrote:
>> Looks OK to me.
>>
>> As I said elsewhere, I do not use ncpfs for years so I cannot provide
>> any sensible maintainership for it anymore :-(
>>
>> Petr
>>
>> On May 31, 2013 2:40 PM, "Dave Chiluk" <chiluk@canonical.com
>> <mailto:chiluk@canonical.com>> wrote:
>>
>>     Any thoughts on this?  NCPFS seems to be the forgotten, left-behind,
>>     red-headed stepchild of the fs community.
>>
>>     Dave.
>>
>>     On 05/28/2013 05:50 PM, Dave Chiluk wrote:
>>     > 1d2ef5901483004d74947bbf78d5146c24038fe7 caused a regression in
>>     ncpfs such that
>>     > directories could no longer be removed.  This was because
>>     ncp_rmdir checked
>>     > to see if a dentry could be unhashed before allowing it to be
>>     removed. Since
>>     > 1d2ef5901483004d74947bbf78d5146c24038fe7 introduced a change that
>>     incremented
>>     > dentry->d_count causing it to always be greater than 1 unhash
>>     would always
>>     > fail.  Thus causing the error path in ncp_rmdir to always be
>>     taken.  Removing
>>     > this error path is safe as unhashing is still accomplished by
>>     calls to dput
>>     > from vfs_rmdir.
>>     > ---
>>     >  fs/ncpfs/dir.c |    9 ---------
>>     >  1 file changed, 9 deletions(-)
>>     >
>>     > diff --git a/fs/ncpfs/dir.c b/fs/ncpfs/dir.c
>>     > index 8163260..6792ce1 100644
>>     > --- a/fs/ncpfs/dir.c
>>     > +++ b/fs/ncpfs/dir.c
>>     > @@ -1029,15 +1029,6 @@ static int ncp_rmdir(struct inode *dir,
>>     struct dentry *dentry)
>>     >       DPRINTK("ncp_rmdir: removing %s/%s\n",
>>     >               dentry->d_parent->d_name.name <http://d_name.name>,
>>     dentry->d_name.name <http://d_name.name>);
>>     >
>>     > -     /*
>>     > -      * fail with EBUSY if there are still references to this
>>     > -      * directory.
>>     > -      */
>>     > -     dentry_unhash(dentry);
>>     > -     error = -EBUSY;
>>     > -     if (!d_unhashed(dentry))
>>     > -             goto out;
>>     > -
>>     >       len = sizeof(__name);
>>     >       error = ncp_io2vol(server, __name, &len, dentry->d_name.name
>>     <http://d_name.name>,
>>     >                          dentry->d_name.len, !ncp_preserve_case(dir));
>>     >
>>
>

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

[Dave Chiluk]

[-- Attachment #1: Type: text/plain, Size: 3048 bytes --]

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?

Dave.

On 06/07/2013 01:43 AM, Petr Vandrovec wrote:
> On Wed, Jun 5, 2013 at 1:20 PM, Dave Chiluk <chiluk@canonical.com> wrote:
>> Petr do you still have commit rights to ncpfs?  Can you please commit it
>> to upstream or do I have to get Al to do that?
> 
> Hi,
>   only thing I can do is to add
> 
> Signed-off-by: Petr Vandrovec <petr@vandrovec.name>
> 
> on your patch and forward it to Al.  Unfortunately patch below is
> already whitespace-damaged :-(  So you can either send it to me, and
> I'll resend it to Al, or you can do that directly...
> 
> Petr
> 
>> Dave.
>>
>>
>> On 05/31/2013 05:22 PM, Petr Vandrovec wrote:
>>> Looks OK to me.
>>>
>>> As I said elsewhere, I do not use ncpfs for years so I cannot provide
>>> any sensible maintainership for it anymore :-(
>>>
>>> Petr
>>>
>>> On May 31, 2013 2:40 PM, "Dave Chiluk" <chiluk@canonical.com
>>> <mailto:chiluk@canonical.com>> wrote:
>>>
>>>     Any thoughts on this?  NCPFS seems to be the forgotten, left-behind,
>>>     red-headed stepchild of the fs community.
>>>
>>>     Dave.
>>>
>>>     On 05/28/2013 05:50 PM, Dave Chiluk wrote:
>>>     > 1d2ef5901483004d74947bbf78d5146c24038fe7 caused a regression in
>>>     ncpfs such that
>>>     > directories could no longer be removed.  This was because
>>>     ncp_rmdir checked
>>>     > to see if a dentry could be unhashed before allowing it to be
>>>     removed. Since
>>>     > 1d2ef5901483004d74947bbf78d5146c24038fe7 introduced a change that
>>>     incremented
>>>     > dentry->d_count causing it to always be greater than 1 unhash
>>>     would always
>>>     > fail.  Thus causing the error path in ncp_rmdir to always be
>>>     taken.  Removing
>>>     > this error path is safe as unhashing is still accomplished by
>>>     calls to dput
>>>     > from vfs_rmdir.
>>>     > ---
>>>     >  fs/ncpfs/dir.c |    9 ---------
>>>     >  1 file changed, 9 deletions(-)
>>>     >
>>>     > diff --git a/fs/ncpfs/dir.c b/fs/ncpfs/dir.c
>>>     > index 8163260..6792ce1 100644
>>>     > --- a/fs/ncpfs/dir.c
>>>     > +++ b/fs/ncpfs/dir.c
>>>     > @@ -1029,15 +1029,6 @@ static int ncp_rmdir(struct inode *dir,
>>>     struct dentry *dentry)
>>>     >       DPRINTK("ncp_rmdir: removing %s/%s\n",
>>>     >               dentry->d_parent->d_name.name <http://d_name.name>,
>>>     dentry->d_name.name <http://d_name.name>);
>>>     >
>>>     > -     /*
>>>     > -      * fail with EBUSY if there are still references to this
>>>     > -      * directory.
>>>     > -      */
>>>     > -     dentry_unhash(dentry);
>>>     > -     error = -EBUSY;
>>>     > -     if (!d_unhashed(dentry))
>>>     > -             goto out;
>>>     > -
>>>     >       len = sizeof(__name);
>>>     >       error = ncp_io2vol(server, __name, &len, dentry->d_name.name
>>>     <http://d_name.name>,
>>>     >                          dentry->d_name.len, !ncp_preserve_case(dir));
>>>     >
>>>
>>
> 


[-- Attachment #2: 0001-ncpfs-fix-rmdir-returns-Device-or-resource-busy.patch --]
[-- Type: text/x-patch, Size: 1574 bytes --]

>From 6af7fb5452c6e6edd10316338982ff85ada2cfcf Mon Sep 17 00:00:00 2001
From: Dave Chiluk <chiluk@canonical.com>
Date: Tue, 28 May 2013 16:06:08 -0500
Subject: [PATCH] ncpfs: fix rmdir returns Device or resource busy

1d2ef5901483004d74947bbf78d5146c24038fe7 caused a regression in ncpfs such that
directories could no longer be removed.  This was because ncp_rmdir checked
to see if a dentry could be unhashed before allowing it to be removed. Since
1d2ef5901483004d74947bbf78d5146c24038fe7 introduced a change that incremented
dentry->d_count causing it to always be greater than 1 unhash would always
fail.  Thus causing the error path in ncp_rmdir to always be taken.  Removing
this error path is safe as unhashing is still accomplished by calls to dput
from vfs_rmdir.

Signed-off-by: Dave Chiluk <chiluk@canonical.com>
Signed-off-by: Petr Vandrovec <petr@vandrovec.name>
---
 fs/ncpfs/dir.c |    9 ---------
 1 file changed, 9 deletions(-)

diff --git a/fs/ncpfs/dir.c b/fs/ncpfs/dir.c
index 8163260..6792ce1 100644
--- a/fs/ncpfs/dir.c
+++ b/fs/ncpfs/dir.c
@@ -1029,15 +1029,6 @@ static int ncp_rmdir(struct inode *dir, struct dentry *dentry)
 	DPRINTK("ncp_rmdir: removing %s/%s\n",
 		dentry->d_parent->d_name.name, dentry->d_name.name);
 
-	/*
-	 * fail with EBUSY if there are still references to this
-	 * directory.
-	 */
-	dentry_unhash(dentry);
-	error = -EBUSY;
-	if (!d_unhashed(dentry))
-		goto out;
-
 	len = sizeof(__name);
 	error = ncp_io2vol(server, __name, &len, dentry->d_name.name,
 			   dentry->d_name.len, !ncp_preserve_case(dir));
-- 
1.7.9.5

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

[Al Viro]

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.

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

[Al Viro]

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?

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

[Al Viro]

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

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

[Dave Chiluk]

On 06/13/2013 01:42 AM, Al Viro wrote:
> 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...
> 
I'm afraid you are way beyond my current vfs experience level on this
one.  While you're getting rid of things you might consider
dentry_unhash as well, as only hpfs_unlink, ncp_rmdir, and ncp_rename
call that.

If you get a patch together, I'll do my best to test it.  Looks like
only ncp_readdir calls that, so afaik, a few varying ls commands should
be all that's needed for a test.

Dave.
p.s. are you sure you don't just want to just deprecate all of ncpfs?

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

[Al Viro]

On Thu, Jun 13, 2013 at 11:19:26PM -0500, Dave Chiluk wrote:

> I'm afraid you are way beyond my current vfs experience level on this
> one.  While you're getting rid of things you might consider
> dentry_unhash as well, as only hpfs_unlink, ncp_rmdir, and ncp_rename
> call that.

The trouble is, hpfs_unlink() really wants it, so we probably won't be
able to kill that off.

> If you get a patch together, I'll do my best to test it.  Looks like
> only ncp_readdir calls that, so afaik, a few varying ls commands should
> be all that's needed for a test.

... combined with memory pressure and changes to directory, to test the
invalidation logics.

> Dave.
> p.s. are you sure you don't just want to just deprecate all of ncpfs?

Don't tempt me ;-)  As far as I'm concerned, everything NetWare-related is
best dealt by fine folks from Miskatonic University, with all the precautions
due when working with spawn of the Old Ones...

Speaking of the madness and perversion: take a look at ncp_fill_cache().
What happens there is that we try to find or create a dentry according
to the directory entry we've got from server, then stuff a reference to
it into page cache of directory inode and call filldir for that sucker.
	* if dentry allocation fails, we skip stuffing a reference into
page cache.  Result: garbage pointer left there.  _Another_ result:
if that happens more than page size / sizeof(pointer) times and then
we finally manage to allocate an entry (or just find one already in
dcache), we hit this:
        if (ctl.idx >= NCP_DIRCACHE_SIZE) {
                if (ctl.page) {
                        kunmap(ctl.page);
                        SetPageUptodate(ctl.page);
                        unlock_page(ctl.page);
                        page_cache_release(ctl.page);
                }
                ctl.cache = NULL;
                ctl.idx  -= NCP_DIRCACHE_SIZE;
                ctl.ofs  += 1;
                ctl.page  = grab_cache_page(&dir->i_data, ctl.ofs);
                if (ctl.page)
                        ctl.cache = kmap(ctl.page);
        }
ctx.idx was being incrmented on each entry, so now we are past
NCP_DIRCACHE_SIZE * 2.  We subtract NCP_DIRCACHE_SIZE, increment
ctl.ofs (page number), grab that page... and proceed to
        if (ctl.cache) {
                ctl.cache->dentry[ctl.idx] = newdent;
                valid = 1;
        }
which stuffs pointer past the end of that page.  And no, the caller
won't stop calling that on the first failure - if ->f_pos is large
enough, we'll record the failure in ctl.valid and have ncp_fill_cache()
return true - ctl.filled is false (== filldir hadn't told us to stop,
since we hadn't called it at all), so ctl.valid || !ctl.filled is
true.  IOW, the loop in caller will keep calling that sucker.

What's more, if ctl.valid is set to 0, there's no point bothering with
page cache anymore - it won't be used at all and on the next readdir()
we'll reread from scratch.

Even better, OOM for inode allocation is treated differently - we stuff
a reference to negative dentry into page cache, so that ncp_dget_fpos()
will find it, notice that it's negative and return NULL.  At which
point the caller will invalidate the damn cache and reread everything
from scratch.  Why bother stuffing it there at all?

BTW, in ncp_fill_cache() we have a provably pointless
                if (!ino)
                        ino = find_inode_number(dentry, &qname);
Check it out - any path that can lead there with ino == 0 will *not*
have a positive dentry with such name, so this find_inode_number()
call is just "waste some time and return 0".  Cargo-cult, plain and
simple...

Grr...  When has that code been read the last time?

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

[Al Viro]

On Sat, Jun 15, 2013 at 06:09:39AM +0100, Al Viro wrote:

> BTW, in ncp_fill_cache() we have a provably pointless
>                 if (!ino)
>                         ino = find_inode_number(dentry, &qname);
> Check it out - any path that can lead there with ino == 0 will *not*
> have a positive dentry with such name, so this find_inode_number()
> call is just "waste some time and return 0".  Cargo-cult, plain and
> simple...

Incidentally, the only other caller of find_inode_number() is equally
pointless, so I'm very inclined to kill the damn function off.  Sure,
it's exported.  And I'm fairly sure that its out-of-tree users are
just as fishy (as in Innsmouth)...

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

[Dave Chiluk]

On 06/12/2013 09:01 PM, 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.
I checked
git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git
and don't see the change, is there somewhere else you applied it, or has
it just not been uploaded yet.

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

As for rename: mv dir1 dir2 works.  I was expecting it to fail similar
to rmdir, but I'm guessing if I trace the code new_dentry->d_count just
happens to = 1 preventing the error path from being taken.

> 
> Related question: what happens if you open / unlink / fchmod on ncpfs?
> 
fchmod returned errno 13: Permission denied

Let me know if you need anything else tested.  Also, please take
everything I say with a grain of salt as this is the first and hopefully
last time, I will ever have to change code in ncpfs.  Frankly, as it has
clearly fallen into disrepair, I'd actually love to see it deprecated in
favour of any other more active network file systems.

Dave.

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

[Luis Henriques]

Dave Chiluk <chiluk@canonical.com> writes:

> 1d2ef5901483004d74947bbf78d5146c24038fe7 caused a regression in ncpfs such that
> directories could no longer be removed.  This was because ncp_rmdir checked
> to see if a dentry could be unhashed before allowing it to be removed. Since
> 1d2ef5901483004d74947bbf78d5146c24038fe7 introduced a change that incremented
> dentry->d_count causing it to always be greater than 1 unhash would always
> fail.  Thus causing the error path in ncp_rmdir to always be taken.  Removing
> this error path is safe as unhashing is still accomplished by calls to dput
> from vfs_rmdir.

Stable kernels starting with 3.0 also contain the offending commit, so
I believe this patch should be applied to stable kernels as well.

Upstream commit is 698b8223631472bf982ed570b0812faa61955683.

Cheers,
-- 
Luis

> ---
>  fs/ncpfs/dir.c |    9 ---------
>  1 file changed, 9 deletions(-)
>
> diff --git a/fs/ncpfs/dir.c b/fs/ncpfs/dir.c
> index 8163260..6792ce1 100644
> --- a/fs/ncpfs/dir.c
> +++ b/fs/ncpfs/dir.c
> @@ -1029,15 +1029,6 @@ static int ncp_rmdir(struct inode *dir, struct dentry *dentry)
>  	DPRINTK("ncp_rmdir: removing %s/%s\n",
>  		dentry->d_parent->d_name.name, dentry->d_name.name);
>  
> -	/*
> -	 * fail with EBUSY if there are still references to this
> -	 * directory.
> -	 */
> -	dentry_unhash(dentry);
> -	error = -EBUSY;
> -	if (!d_unhashed(dentry))
> -		goto out;
> -
>  	len = sizeof(__name);
>  	error = ncp_io2vol(server, __name, &len, dentry->d_name.name,
>  			   dentry->d_name.len, !ncp_preserve_case(dir));

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

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1018 bytes --]

On Wed, 2013-06-19 at 10:30 +0100, Luis Henriques wrote:
> Dave Chiluk <chiluk@canonical.com> writes:
> 
> > 1d2ef5901483004d74947bbf78d5146c24038fe7 caused a regression in ncpfs such that
> > directories could no longer be removed.  This was because ncp_rmdir checked
> > to see if a dentry could be unhashed before allowing it to be removed. Since
> > 1d2ef5901483004d74947bbf78d5146c24038fe7 introduced a change that incremented
> > dentry->d_count causing it to always be greater than 1 unhash would always
> > fail.  Thus causing the error path in ncp_rmdir to always be taken.  Removing
> > this error path is safe as unhashing is still accomplished by calls to dput
> > from vfs_rmdir.
> 
> Stable kernels starting with 3.0 also contain the offending commit, so
> I believe this patch should be applied to stable kernels as well.
> 
> Upstream commit is 698b8223631472bf982ed570b0812faa61955683.

Queued up for 3.2, thanks.

Ben.

-- 
Ben Hutchings
Knowledge is power.  France is bacon.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]