unlink(nonexistent): EROFS or ENOENT?

unlink(nonexistent): EROFS or ENOENT?

[Michael Tokarev]

Hello.

Just noticed that at least on ext4, unlinking a
non-existing file from a read-only filesystem
results in EROFS instead of ENOENT.  I'd expect
it return ENOENT - it is more logical, at least
in my opinion.

For one, (readonly) NFS mount returns ENOENT in
this case.

Thanks!

/mjt

Re: unlink(nonexistent): EROFS or ENOENT?

[Michael Tokarev]

29.05.2011 20:08, Michael Tokarev пишет:
> Hello.
> 
> Just noticed that at least on ext4, unlinking a
> non-existing file from a read-only filesystem
> results in EROFS instead of ENOENT.  I'd expect
> it return ENOENT - it is more logical, at least
> in my opinion.

http://pubs.opengroup.org/onlinepubs/009695399/functions/unlink.html
this case is quite clear:

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

Ie, the entry is a _part_ of a file system, so it should be
_existing_ entry to start with.

> For one, (readonly) NFS mount returns ENOENT in
> this case.
> 
> Thanks!

/mjt

Re: unlink(nonexistent): EROFS or ENOENT?

[Ted Ts'o]

On Sun, May 29, 2011 at 08:08:55PM +0400, Michael Tokarev wrote:
> Hello.
> 
> Just noticed that at least on ext4, unlinking a
> non-existing file from a read-only filesystem
> results in EROFS instead of ENOENT.  I'd expect
> it return ENOENT - it is more logical, at least
> in my opinion.
> 
> For one, (readonly) NFS mount returns ENOENT in
> this case.

Um, it doesn't for me.   Testing on v3.0-rc1:

# ls /test/foo; rm /test/foo
ls: cannot access /test/foo: No such file or directory
rm: cannot remove `/test/foo': No such file or directory
# ls /test/null; rm /test/null
/test/null
rm: cannot remove `/test/null': Read-only file system
# grep test /proc/mounts
/dev/vdb /test ext4 ro,relatime,user_xattr,barrier=1,data=ordered 0 0

				- Ted

Re: unlink(nonexistent): EROFS or ENOENT?

[Michael Tokarev]

Thank you for the answer.  I thought noone will reply... ;)

06.06.2011 07:39, Ted Ts'o wrote:
> On Sun, May 29, 2011 at 08:08:55PM +0400, Michael Tokarev wrote:
>> Hello.
>>
>> Just noticed that at least on ext4, unlinking a
>> non-existing file from a read-only filesystem
>> results in EROFS instead of ENOENT.  I'd expect
>> it return ENOENT - it is more logical, at least
>> in my opinion.
>>
>> For one, (readonly) NFS mount returns ENOENT in
>> this case.
> 
> Um, it doesn't for me.   Testing on v3.0-rc1:
> 
> # ls /test/foo; rm /test/foo
> ls: cannot access /test/foo: No such file or directory
> rm: cannot remove `/test/foo': No such file or directory

This is a hack in coreutils rm to work around this
kernel change.  The comment at
 http://git.savannah.gnu.org/cgit/coreutils.git/tree/src/remove.c#n450
says:

  /* The unlinkat from kernels like linux-2.6.32 reports EROFS even for
     nonexistent files.  When the file is indeed missing, map that to ENOENT,
     so that rm -f ignores it, as required.  Even without -f, this is useful
     because it makes rm print the more precise diagnostic.  */

so that rm(1) calls stat(2) to see if the file actually
exist if unlinkat() returned EROFS, and turns this errno
into ENOENT.

That is, rm(1) output is not a good indicator.  Use

  strace rm -f /test/foo 2>&1 | grep unlink

to see the actual errno reported by the kernel.

Here's the POSIX description of unlink (and unlinkat) again:
http://pubs.opengroup.org/onlinepubs/009695399/functions/unlink.html

Thanks!

/mjt

Re: unlink(nonexistent): EROFS or ENOENT?

[Michael Tokarev]

06.06.2011 21:13, Michael Tokarev wrote:
> Thank you for the answer.  I thought noone will reply... ;)
> 
> 06.06.2011 07:39, Ted Ts'o wrote:
>> On Sun, May 29, 2011 at 08:08:55PM +0400, Michael Tokarev wrote:
>>> Hello.
>>>
>>> Just noticed that at least on ext4, unlinking a
>>> non-existing file from a read-only filesystem
>>> results in EROFS instead of ENOENT.  I'd expect
>>> it return ENOENT - it is more logical, at least
>>> in my opinion.
>>>
>>> For one, (readonly) NFS mount returns ENOENT in
>>> this case.
>>
>> Um, it doesn't for me.   Testing on v3.0-rc1:
>>
>> # ls /test/foo; rm /test/foo
>> ls: cannot access /test/foo: No such file or directory
>> rm: cannot remove `/test/foo': No such file or directory
> 
> This is a hack in coreutils rm to work around this
> kernel change.  The comment at
>  http://git.savannah.gnu.org/cgit/coreutils.git/tree/src/remove.c#n450
> says:
> 
>   /* The unlinkat from kernels like linux-2.6.32 reports EROFS even for
>      nonexistent files.  When the file is indeed missing, map that to ENOENT,
>      so that rm -f ignores it, as required.  Even without -f, this is useful
>      because it makes rm print the more precise diagnostic.  */
> 
> so that rm(1) calls stat(2) to see if the file actually
> exist if unlinkat() returned EROFS, and turns this errno
> into ENOENT.

And another followup to this, -- the original case when I actually
noticed the problem.   A readonly-mounted root filesystem with /etc
in git (the repository is in /var, symlinked from /etc/.git).  I
deleted a few files from /etc (when it was readwrite), and noticed
that I forgot to commit the change.  So I used `git rm oldfiles' and
voila, git, for the first time, refused to commit stuff for me in
this configuration, -- before, I was always able to _commit_ the
changes even if the working tree is read-only.  It works for
everything but not for unlinks.

> That is, rm(1) output is not a good indicator.  Use
> 
>   strace rm -f /test/foo 2>&1 | grep unlink
> 
> to see the actual errno reported by the kernel.
> 
> Here's the POSIX description of unlink (and unlinkat) again:
> http://pubs.opengroup.org/onlinepubs/009695399/functions/unlink.html
> 
> Thanks!
> 
> /mjt
> --
> 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/

Re: unlink(nonexistent): EROFS or ENOENT?

[Ted Ts'o]

On Mon, Jun 06, 2011 at 09:13:23PM +0400, Michael Tokarev wrote:
> Thank you for the answer.  I thought noone will reply... ;)
> 
> >> Just noticed that at least on ext4, unlinking a
> >> non-existing file from a read-only filesystem
> >> results in EROFS instead of ENOENT.  I'd expect
> >> it return ENOENT - it is more logical, at least
> >> in my opinion.

>   /* The unlinkat from kernels like linux-2.6.32 reports EROFS even for
>      nonexistent files.  When the file is indeed missing, map that to ENOENT,
>      so that rm -f ignores it, as required.  Even without -f, this is useful
>      because it makes rm print the more precise diagnostic.  */

OK, I see what's going on.  This check is in the VFS layer, so it
affects all filesystems; it's not an ext4-specific thing.

Patch coming shortly.

	    		      	     		   - Ted

[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

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