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 <[email protected]>
Signed-off-by: "Theodore Ts'o" <[email protected]>
---
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
Hi,
On Mon, Jun 6, 2011 at 4:36 PM, Theodore Ts'o <[email protected]> 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 <[email protected]>
> Signed-off-by: "Theodore Ts'o" <[email protected]>
> ---
> ?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 [email protected]
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at ?http://www.tux.org/lkml/
>
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 <[email protected]>
Signed-off-by: "Theodore Ts'o" <[email protected]>
---
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
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).
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
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 <[email protected]>
Signed-off-by: "Theodore Ts'o" <[email protected]>
---
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
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 <[email protected]>
> Signed-off-by: "Theodore Ts'o" <[email protected]>
Applied. In for-next for now, will go into for-linus once Linus picks
the last pull request...
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 <[email protected]>
> Signed-off-by: "Theodore Ts'o" <[email protected]>
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