[PATCH] vfs: make unlink() return ENOENT in preference to EROFS

[PATCH] vfs: make unlink() return ENOENT in preference to EROFS

[Theodore Ts'o]

If user space attempts to unlink a non-existent file, and the file
system is mounted read-only, return ENOENT instead of EROFS.  Either
error code is arguably valid/correct, but ENOENT is a more specific
error message.

Reported-by: Michael Tokarev <mjt@tls.msk.ru>
Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
---
 fs/namei.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index 1ab641f..a9edbe0 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2708,9 +2708,9 @@ static long do_unlinkat(int dfd, const char __user *pathname)
 	error = PTR_ERR(dentry);
 	if (!IS_ERR(dentry)) {
 		/* Why not before? Because we want correct error value */
-		if (nd.last.name[nd.last.len])
-			goto slashes;
 		inode = dentry->d_inode;
+		if (nd.last.name[nd.last.len] || !inode)
+			goto slashes;
 		if (inode)
 			ihold(inode);
 		error = mnt_want_write(nd.path.mnt);
-- 
1.7.4.1.22.gec8e1.dirty

Re: [PATCH] vfs: make unlink() return ENOENT in preference to EROFS

[Arnaud Lacombe]

Hi,

On Mon, Jun 6, 2011 at 4:36 PM, Theodore Ts'o <tytso@mit.edu> wrote:
> If user space attempts to unlink a non-existent file, and the file
> system is mounted read-only, return ENOENT instead of EROFS.  Either
> error code is arguably valid/correct, but ENOENT is a more specific
> error message.
>
> Reported-by: Michael Tokarev <mjt@tls.msk.ru>
> Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
> ---
>  fs/namei.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fs/namei.c b/fs/namei.c
> index 1ab641f..a9edbe0 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -2708,9 +2708,9 @@ static long do_unlinkat(int dfd, const char __user *pathname)
>        error = PTR_ERR(dentry);
>        if (!IS_ERR(dentry)) {
>                /* Why not before? Because we want correct error value */
> -               if (nd.last.name[nd.last.len])
> -                       goto slashes;
>                inode = dentry->d_inode;
> +               if (nd.last.name[nd.last.len] || !inode)
> +                       goto slashes;
>                if (inode)
>                        ihold(inode);
It seems to me that this conditional will now always be verified as
you jump to `slashes' whenever `inode' is NULL, no ?

 - Arnaud

>                error = mnt_want_write(nd.path.mnt);
> --
> 1.7.4.1.22.gec8e1.dirty
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

[PATCH -v2] vfs: make unlink() return ENOENT in preference to EROFS

[Theodore Ts'o]

If user space attempts to unlink a non-existent file, and the file
system is mounted read-only, return ENOENT instead of EROFS.  Either
error code is arguably valid/correct, but ENOENT is a more specific
error message.

Reported-by: Michael Tokarev <mjt@tls.msk.ru>
Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
---
 fs/namei.c |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index 1ab641f..30398db 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2708,11 +2708,10 @@ static long do_unlinkat(int dfd, const char __user *pathname)
 	error = PTR_ERR(dentry);
 	if (!IS_ERR(dentry)) {
 		/* Why not before? Because we want correct error value */
-		if (nd.last.name[nd.last.len])
-			goto slashes;
 		inode = dentry->d_inode;
-		if (inode)
-			ihold(inode);
+		if (nd.last.name[nd.last.len] || !inode)
+			goto slashes;
+		ihold(inode);
 		error = mnt_want_write(nd.path.mnt);
 		if (error)
 			goto exit2;
-- 
1.7.4.1.22.gec8e1.dirty

Re: [PATCH -v2] vfs: make unlink() return ENOENT in preference to EROFS

[Al Viro]

On Mon, Jun 06, 2011 at 04:58:13PM -0400, Theodore Ts'o wrote:
> If user space attempts to unlink a non-existent file, and the file
> system is mounted read-only, return ENOENT instead of EROFS.  Either
> error code is arguably valid/correct, but ENOENT is a more specific
> error message.

Umm...  I can live with that.  What about rmdir(2)?  We have similar situation
there as well.  If we care about one, why not the other?

Mind you, I'm not at all convinced that it matters enough to bother, but
yes, ENOENT is a bit more specific (and likelier to be handled by luserland
code).

Re: [PATCH -v2] vfs: make unlink() return ENOENT in preference to EROFS

[Michael Tokarev]

07.06.2011 02:30, Al Viro wrote:
> On Mon, Jun 06, 2011 at 04:58:13PM -0400, Theodore Ts'o wrote:
>> If user space attempts to unlink a non-existent file, and the file
>> system is mounted read-only, return ENOENT instead of EROFS.  Either
>> error code is arguably valid/correct, but ENOENT is a more specific
>> error message.
> 
> Umm...  I can live with that.  What about rmdir(2)?  We have similar situation
> there as well.  If we care about one, why not the other?

I think both should be fixed.

> Mind you, I'm not at all convinced that it matters enough to bother, but
> yes, ENOENT is a bit more specific (and likelier to be handled by luserland
> code).

The problem which triggered the initial thread and Ted's patch was me
trying to commit some changes from read-only /etc into git tree.  This
works for everything but deletes, since `git rm' barfs when unlink for
a non-existing file returns EROFS.  rm(1) has been patched especially
for this case at about kernel 2.6.32 time, as shown in comments at
 http://git.savannah.gnu.org/cgit/coreutils.git/tree/src/remove.c#n450 ,
but git has not (yet), and I suspect git isn't the only leftover, there
are other applications to patch still, if the kernel will continue to
return EROFS.

Besides, POSIX says (http://pubs.opengroup.org/onlinepubs/009695399/functions/unlink.html):

  [EROFS]
    The directory entry to be unlinked
    is part of a read-only file system

so it clearly states that the entry should exists for EROFS, ie, to
be _part_ of the filesystem.

Thanks!

/mjt

[PATCH -v3] vfs: make unlink() and rmdir() return ENOENT in preference to EROFS

[Theodore Ts'o]

If user space attempts to remove a non-existent file or directory, and
the file system is mounted read-only, return ENOENT instead of EROFS.
Either error code is arguably valid/correct, but ENOENT is a more
specific error message.

Reported-by: Michael Tokarev <mjt@tls.msk.ru>
Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
---
 fs/namei.c |   11 +++++++----
 1 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index 1ab641f..647ca6e 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2623,6 +2623,10 @@ static long do_rmdir(int dfd, const char __user *pathname)
 	error = PTR_ERR(dentry);
 	if (IS_ERR(dentry))
 		goto exit2;
+	if (!dentry->d_inode) {
+		error = -ENOENT;
+		goto exit3;
+	}
 	error = mnt_want_write(nd.path.mnt);
 	if (error)
 		goto exit3;
@@ -2708,11 +2712,10 @@ static long do_unlinkat(int dfd, const char __user *pathname)
 	error = PTR_ERR(dentry);
 	if (!IS_ERR(dentry)) {
 		/* Why not before? Because we want correct error value */
-		if (nd.last.name[nd.last.len])
-			goto slashes;
 		inode = dentry->d_inode;
-		if (inode)
-			ihold(inode);
+		if (nd.last.name[nd.last.len] || !inode)
+			goto slashes;
+		ihold(inode);
 		error = mnt_want_write(nd.path.mnt);
 		if (error)
 			goto exit2;
-- 
1.7.4.1.22.gec8e1.dirty

Re: [PATCH -v3] vfs: make unlink() and rmdir() return ENOENT in preference to EROFS

[Al Viro]

On Mon, Jun 06, 2011 at 07:19:40PM -0400, Theodore Ts'o wrote:
> If user space attempts to remove a non-existent file or directory, and
> the file system is mounted read-only, return ENOENT instead of EROFS.
> Either error code is arguably valid/correct, but ENOENT is a more
> specific error message.
> 
> Reported-by: Michael Tokarev <mjt@tls.msk.ru>
> Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>

Applied.  In for-next for now, will go into for-linus once Linus picks
the last pull request...

Re: [PATCH -v3] vfs: make unlink() and rmdir() return ENOENT in preference to EROFS

[Michael Tokarev]

07.06.2011 03:19, Theodore Ts'o wrote:
> If user space attempts to remove a non-existent file or directory, and
> the file system is mounted read-only, return ENOENT instead of EROFS.
> Either error code is arguably valid/correct, but ENOENT is a more
> specific error message.
> 
> Reported-by: Michael Tokarev <mjt@tls.msk.ru>
> Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>

Thank you very much for this patch.  I had no chance to test it before,
now I've tested it and it works as expected - now both rm(1) and rmdir(1)
behave correctly, and git rm works too.

It's interesting to note that it's been this way for several years
(when the change occured?), and there has been no complains so far... ;)

Thanks!

/mjt

Re: [-v3] vfs: make unlink() and rmdir() return ENOENT in preference to EROFS

[Michael Tokarev]

07.06.2011 03:19, Theodore Tso wrote:
> If user space attempts to remove a non-existent file or directory, and
> the file system is mounted read-only, return ENOENT instead of EROFS.
> Either error code is arguably valid/correct, but ENOENT is a more
> specific error message.
> 
> Reported-by: Michael Tokarev <mjt@tls.msk.ru>
> Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
> 
> ---
> fs/namei.c |   11 +++++++----
>  1 files changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index 1ab641f..647ca6e 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -2623,6 +2623,10 @@ static long do_rmdir(int dfd, const char __user *pathname)
>  	error = PTR_ERR(dentry);
>  	if (IS_ERR(dentry))
>  		goto exit2;
> +	if (!dentry->d_inode) {
> +		error = -ENOENT;
> +		goto exit3;
> +	}
>  	error = mnt_want_write(nd.path.mnt);
>  	if (error)
>  		goto exit3;
> @@ -2708,11 +2712,10 @@ static long do_unlinkat(int dfd, const char __user *pathname)
>  	error = PTR_ERR(dentry);
>  	if (!IS_ERR(dentry)) {
>  		/* Why not before? Because we want correct error value */
> -		if (nd.last.name[nd.last.len])
> -			goto slashes;
>  		inode = dentry->d_inode;
> -		if (inode)
> -			ihold(inode);
> +		if (nd.last.name[nd.last.len] || !inode)
> +			goto slashes;
> +		ihold(inode);
>  		error = mnt_want_write(nd.path.mnt);
>  		if (error)
>  			goto exit2;

With this patch applied on top of 2.6.39 I can trigger a BUG_ON
condition like this:

[  211.506827] ------------[ cut here ]------------
[  211.506919] kernel BUG at fs/inode.c:1409!
[  211.507003] invalid opcode: 0000 [#1] SMP
[  211.507095] last sysfs file: /sys/devices/pci0000:00/0000:00:11.0/host5/target5:0:0/5:0:0:0/block/sdb/uevent
[  211.507284] CPU 0
[  211.507318] Modules linked in: bnep rfcomm bluetooth rfkill autofs4 quota_v2 quota_tree fuse nfsd nfs lockd fscache auth_rpcgss
[  211.509512]
[  211.509547] Pid: 3159, comm: rm Not tainted 2.6.39-amd64 #1 System manufacturer System Product Name/M3A78-EM
[  211.509761] RIP: 0010:[<ffffffff81137e58>]  [<ffffffff81137e58>] iput+0x198/0x1d0
[  211.509918] RSP: 0018:ffff880169af3e88  EFLAGS: 00010202
[  211.509983] RAX: 0000000000000000 RBX: ffff8801809988a0 RCX: 00000000ffff984e
[  211.509983] RDX: ffff8801877075a8 RSI: ffffffffa012f1b0 RDI: ffff8801809988a0
[  211.509983] RBP: ffff8801809988a0 R08: d018000000000000 R09: ffff8801809987f0
[  211.509983] R10: ffff88012c258338 R11: 0000000000000000 R12: ffff880131062ec0
[  211.509983] R13: ffff880131062ec0 R14: ffff88019e836080 R15: ffff88012b6f5f10
[  211.509983] FS:  0000000000000000(0000) GS:ffff88019fc00000(0063) knlGS:00000000f765f8d0
[  211.509983] CS:  0010 DS: 002b ES: 002b CR0: 000000008005003b
[  211.509983] CR2: 00000000082390ac CR3: 000000018734c000 CR4: 00000000000006f0
[  211.509983] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[  211.509983] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
[  211.509983] Process rm (pid: 3159, threadinfo ffff880169af2000, task ffff88018734a0c0)
[  211.509983] Stack:
[  211.509983]  ffff8801877075e8 ffff880180997c00 ffff8801809988a0 ffff880131062ec0
[  211.509983]  ffff880131062ec0 ffffffff811333b0 ffff880180997c00 ffff8801809988a0
[  211.509983]  ffff880180997c5c ffffffff811345aa ffff88012b6f5f00 ffff88012b6f5f00
[  211.509983] Call Trace:
[  211.509983]  [<ffffffff811333b0>] ? d_kill+0xf0/0x130
[  211.509983]  [<ffffffff811345aa>] ? dput+0xca/0x180
[  211.509983]  [<ffffffff81121054>] ? fput+0x1b4/0x270
[  211.509983]  [<ffffffff8111d43c>] ? filp_close+0x5c/0x90
[  211.509983]  [<ffffffff8111d4fb>] ? sys_close+0x8b/0xe0
[  211.509983]  [<ffffffff8134f8a0>] ? cstar_dispatch+0x7/0x2e
[  211.509983] Code: d1 48 8b 15 8b ab 35 00 48 89 42 08 48 89 53 78 48 c7 83 80 00 00 00 c0 29 49 81 48 89 05 71 ab 35 00 83 05 e
[  211.509983] RIP  [<ffffffff81137e58>] iput+0x198/0x1d0
[  211.509983]  RSP <ffff880169af3e88>
[  211.546040] ---[ end trace f3b7c3bd13474628 ]---

This is happening on a debian system when running update-menus while
lxde is running.  When update-menus is run it executes

 rm -rf /var/lib/menu-xdg/desktop-directories/menu-xdg/

and regenerates it, and lxde is watching that directory somehow
in order to notice updates (I think anyway).  I can't trigger
this BUG_ON without my X running (but I did not try other
desktop environments).

The place where this BUG_ON is looks like this:

void iput(struct inode *inode)
{
        if (inode) {
                BUG_ON(inode->i_state & I_CLEAR);

                if (atomic_dec_and_lock(&inode->i_count, &inode->i_lock))
                        iput_final(inode);
        }
}


Something fishy is going on there...

Thank you!

/mjt