[RESEND/PATCH] ext[234]: Return -EIO not -ESTALE on directory traversal missing inode

[RESEND/PATCH] ext[234]: Return -EIO not -ESTALE on directory traversal missing inode

[Bryan Donlan]

The ext[234]_iget() functions in the ext[234] family of filesystems returns
-ESTALE if invoked on a deleted inode, in order to report errors to NFS
properly. However, in ext[234]_lookup(), this -ESTALE can be propagated to
userspace if the filesystem is corrupted, and a inode is linked even
though it is marked as deleted. This leads to a misleading error message -
"Stale NFS file handle" - and confusion on the part of the admin.

The bug can be easily reproduced by creating a new filesystem, making a link
to an unused inode using debugfs, then mounting and attempting to ls -l
said link.

This patch thus changes ext[234]_lookup to return -EIO if it receives -ESTALE
from ext[234]_iget(), as ext[234] does for other filesystem metadata
corruption.

Signed-off-by: Bryan Donlan <bdonlan@gmail.com>
---

Apologies for the resend so quickly for those on the CC list - my mailer was
misconfigured and the mail rejected by vger.

 fs/ext2/namei.c |    8 ++++++--
 fs/ext3/namei.c |    8 ++++++--
 fs/ext4/namei.c |    8 ++++++--
 3 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/fs/ext2/namei.c b/fs/ext2/namei.c
index 90ea179..7dab3e8 100644
--- a/fs/ext2/namei.c
+++ b/fs/ext2/namei.c
@@ -66,8 +66,12 @@ static struct dentry *ext2_lookup(struct inode * dir, struct dentry *dentry, str
 	inode = NULL;
 	if (ino) {
 		inode = ext2_iget(dir->i_sb, ino);
-		if (IS_ERR(inode))
-			return ERR_CAST(inode);
+		if (unlikely(IS_ERR(inode))) {
+			if (PTR_ERR(inode) == -ESTALE)
+				return ERR_PTR(-EIO);
+			else
+				return ERR_CAST(inode);
+		}
 	}
 	return d_splice_alias(inode, dentry);
 }
diff --git a/fs/ext3/namei.c b/fs/ext3/namei.c
index 4db4ffa..625f5dc 100644
--- a/fs/ext3/namei.c
+++ b/fs/ext3/namei.c
@@ -1047,8 +1047,12 @@ static struct dentry *ext3_lookup(struct inode * dir, struct dentry *dentry, str
 			return ERR_PTR(-EIO);
 		}
 		inode = ext3_iget(dir->i_sb, ino);
-		if (IS_ERR(inode))
-			return ERR_CAST(inode);
+		if (unlikely(IS_ERR(inode))) {
+			if (PTR_ERR(inode) == -ESTALE)
+				return ERR_PTR(-EIO);
+			else
+				return ERR_CAST(inode);
+		}
 	}
 	return d_splice_alias(inode, dentry);
 }
diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index ba702bd..4b054b3 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -1052,8 +1052,12 @@ static struct dentry *ext4_lookup(struct inode *dir, struct dentry *dentry, stru
 			return ERR_PTR(-EIO);
 		}
 		inode = ext4_iget(dir->i_sb, ino);
-		if (IS_ERR(inode))
-			return ERR_CAST(inode);
+		if (unlikely(IS_ERR(inode))) {
+			if (PTR_ERR(inode) == -ESTALE)
+				return ERR_PTR(-EIO);
+			else
+				return ERR_CAST(inode);
+		}
 	}
 	return d_splice_alias(inode, dentry);
 }
-- 
1.5.6.3

Re: [RESEND/PATCH] ext[234]: Return -EIO not -ESTALE on directory traversal missing inode

[Theodore Tso]

On Sat, Feb 14, 2009 at 12:18:08AM -0500, Bryan Donlan wrote:
> The ext[234]_iget() functions in the ext[234] family of filesystems returns
> -ESTALE if invoked on a deleted inode, in order to report errors to NFS
> properly. However, in ext[234]_lookup(), this -ESTALE can be propagated to
> userspace if the filesystem is corrupted, and a inode is linked even
> though it is marked as deleted. 

I'm not sure what you mean by "inode is linked" here.  All you are
really proposing to do here is to remap the ESTALE error to EIO, yes?

> Apologies for the resend so quickly for those on the CC list - my mailer was
> misconfigured and the mail rejected by vger.

RESEND is a code word meaning --- "I sent this a few days/weeks ago
and no one responded; please look at it again?" I was confused when I
saw this, because I didn't remember seeing this, either in my inbox or
in the ext4 patchwork (which is really great and helping me make sure
I don't miss patches), so I spent a few extra minutes googling to see
what had gotten missed until I finally got to your apologies section....

I received two copies with the RESEND keyword; so I guess that means
you ended up sending it three times?  Anyway, apparently the first
time you sent it your mailer was so badly misconfigured that it got
dropped by mit.edu's mailer as well.  :-) In general if you end up
resending, don't worry about flagging it as a resend; deleting
duplicates is easy enough to do.

> --- a/fs/ext2/namei.c
> +++ b/fs/ext2/namei.c
> @@ -66,8 +66,12 @@ static struct dentry *ext2_lookup(struct inode * dir, struct dentry *dentry, str
>  	inode = NULL;
>  	if (ino) {
>  		inode = ext2_iget(dir->i_sb, ino);
> -		if (IS_ERR(inode))
> -			return ERR_CAST(inode);
> +		if (unlikely(IS_ERR(inode))) {

I'm dubious about unlikely() here; OTOH, penalizing the error case
seems reasonable.

> +			if (PTR_ERR(inode) == -ESTALE)

Please add a call to ext2_error() here, and in make a similar change
for the ext3 and ext4 patches:

       	     	     		ext2_error(dir->i_sb, "ext2_lockup",
					   "deleted inode referenced: %u",
					   ino);

> +				return ERR_PTR(-EIO);
> +			else
> +				return ERR_CAST(inode);
> +		}
>  	}
>  	return d_splice_alias(inode, dentry);
>  }

						- Ted

Re: [RESEND/PATCH] ext[234]: Return -EIO not -ESTALE on directory traversal missing inode

[Bryan Donlan]

On Sat, Feb 14, 2009 at 9:14 AM, Theodore Tso <tytso@mit.edu> wrote:
> On Sat, Feb 14, 2009 at 12:18:08AM -0500, Bryan Donlan wrote:
>> The ext[234]_iget() functions in the ext[234] family of filesystems returns
>> -ESTALE if invoked on a deleted inode, in order to report errors to NFS
>> properly. However, in ext[234]_lookup(), this -ESTALE can be propagated to
>> userspace if the filesystem is corrupted, and a inode is linked even
>> though it is marked as deleted.
>
> I'm not sure what you mean by "inode is linked" here.  All you are
> really proposing to do here is to remap the ESTALE error to EIO, yes?

Sorry, I meant "if the an inode marked as deleted is linked in a
directory" - describing what causes the condition. But yes, I'm
basically proposing to remap ESTALE to EIO here.

>> Apologies for the resend so quickly for those on the CC list - my mailer was
>> misconfigured and the mail rejected by vger.
>
> RESEND is a code word meaning --- "I sent this a few days/weeks ago
> and no one responded; please look at it again?" I was confused when I
> saw this, because I didn't remember seeing this, either in my inbox or
> in the ext4 patchwork (which is really great and helping me make sure
> I don't miss patches), so I spent a few extra minutes googling to see
> what had gotten missed until I finally got to your apologies section....

Sorry about that; it's my first patch submission so I'm not entirely
up to speed on such keywords...

> I received two copies with the RESEND keyword; so I guess that means
> you ended up sending it three times?  Anyway, apparently the first
> time you sent it your mailer was so badly misconfigured that it got
> dropped by mit.edu's mailer as well.  :-) In general if you end up
> resending, don't worry about flagging it as a resend; deleting
> duplicates is easy enough to do.

Okay then - hopefully it won't happen again, but I'll keep that in
mind if there is a next time :)

>
>> --- a/fs/ext2/namei.c
>> +++ b/fs/ext2/namei.c
>> @@ -66,8 +66,12 @@ static struct dentry *ext2_lookup(struct inode * dir, struct dentry *dentry, str
>>       inode = NULL;
>>       if (ino) {
>>               inode = ext2_iget(dir->i_sb, ino);
>> -             if (IS_ERR(inode))
>> -                     return ERR_CAST(inode);
>> +             if (unlikely(IS_ERR(inode))) {
>
> I'm dubious about unlikely() here; OTOH, penalizing the error case
> seems reasonable.

I can leave it without the unlikely(), as it was before, but as far as
I can tell, this should never happen under a non-corrupted, non-broken
hardware filesystem, so it seems like a reasonable annotation to me.

>> +                     if (PTR_ERR(inode) == -ESTALE)
>
> Please add a call to ext2_error() here, and in make a similar change
> for the ext3 and ext4 patches:
>
>                                ext2_error(dir->i_sb, "ext2_lockup",
>                                           "deleted inode referenced: %u",
>                                           ino);

Okay, I'll update the patch and resubmit.

Thanks,

Bryan Donlan

Re: [RESEND/PATCH] ext[234]: Return -EIO not -ESTALE on directory traversal missing inode

[Bryan Donlan]

On Sat, Feb 14, 2009 at 9:14 AM, Theodore Tso <tytso@mit.edu> wrote:
> On Sat, Feb 14, 2009 at 12:18:08AM -0500, Bryan Donlan wrote:
>> The ext[234]_iget() functions in the ext[234] family of filesystems returns
>> -ESTALE if invoked on a deleted inode, in order to report errors to NFS
>> properly. However, in ext[234]_lookup(), this -ESTALE can be propagated to
>> userspace if the filesystem is corrupted, and a inode is linked even
>> though it is marked as deleted.
>
> I'm not sure what you mean by "inode is linked" here.  All you are
> really proposing to do here is to remap the ESTALE error to EIO, yes?

Sorry, I meant "if the an inode marked as deleted is linked in a
directory" - describing what causes the condition. But yes, I'm
basically proposing to remap ESTALE to EIO here.

>> Apologies for the resend so quickly for those on the CC list - my mailer was
>> misconfigured and the mail rejected by vger.
>
> RESEND is a code word meaning --- "I sent this a few days/weeks ago
> and no one responded; please look at it again?" I was confused when I
> saw this, because I didn't remember seeing this, either in my inbox or
> in the ext4 patchwork (which is really great and helping me make sure
> I don't miss patches), so I spent a few extra minutes googling to see
> what had gotten missed until I finally got to your apologies section....

Sorry about that; it's my first patch submission so I'm not entirely
up to speed on such keywords...

> I received two copies with the RESEND keyword; so I guess that means
> you ended up sending it three times?  Anyway, apparently the first
> time you sent it your mailer was so badly misconfigured that it got
> dropped by mit.edu's mailer as well.  :-) In general if you end up
> resending, don't worry about flagging it as a resend; deleting
> duplicates is easy enough to do.

Okay then - hopefully it won't happen again, but I'll keep that in
mind if there is a next time :)

>
>> --- a/fs/ext2/namei.c
>> +++ b/fs/ext2/namei.c
>> @@ -66,8 +66,12 @@ static struct dentry *ext2_lookup(struct inode * dir, struct dentry *dentry, str
>>       inode = NULL;
>>       if (ino) {
>>               inode = ext2_iget(dir->i_sb, ino);
>> -             if (IS_ERR(inode))
>> -                     return ERR_CAST(inode);
>> +             if (unlikely(IS_ERR(inode))) {
>
> I'm dubious about unlikely() here; OTOH, penalizing the error case
> seems reasonable.

I can leave it without the unlikely(), as it was before, but as far as
I can tell, this should never happen under a non-corrupted, non-broken
hardware filesystem, so it seems like a reasonable annotation to me.

>> +                     if (PTR_ERR(inode) == -ESTALE)
>
> Please add a call to ext2_error() here, and in make a similar change
> for the ext3 and ext4 patches:
>
>                                ext2_error(dir->i_sb, "ext2_lockup",
>                                           "deleted inode referenced: %u",
>                                           ino);

Okay, I'll update the patch and resubmit.

Thanks,

Bryan Donlan

Re: [RESEND/PATCH] ext[234]: Return -EIO not -ESTALE on directory traversal missing inode

[Theodore Tso]

On Sat, Feb 14, 2009 at 11:53:48PM -0500, Bryan Donlan wrote:
> > I'm dubious about unlikely() here; OTOH, penalizing the error case
> > seems reasonable.
> 
> I can leave it without the unlikely(), as it was before, but as far as
> I can tell, this should never happen under a non-corrupted, non-broken
> hardware filesystem, so it seems like a reasonable annotation to me.

You're right.  I was looking at the wrong place in the source, and
thought this could happen if the lookup failed; but yes, you're right,
this case can only happen if the filesystem is corrupted or there is
an I/O error.

						- Ted

[PATCH v2] ext[234]: Return -EIO not -ESTALE on directory traversal through deleted inode

[Bryan Donlan]

The ext[234]_iget() functions in the ext[234] family of filesystems returns
-ESTALE if invoked on a deleted inode, in order to report errors to NFS
properly. However, in ext[234]_lookup(), this -ESTALE can be propagated to
userspace if the filesystem is corrupted such that a directory entry
references a deleted inode. This leads to a misleading error message - "Stale
NFS file handle" - and confusion on the part of the admin.

The bug can be easily reproduced by creating a new filesystem, making a link
to an unused inode using debugfs, then mounting and attempting to ls -l
said link.

This patch thus changes ext[234]_lookup to return -EIO if it receives -ESTALE
from ext[234]_iget(), as ext[234] does for other filesystem metadata
corruption; and also invokes the appropriate ext*_error functions when
this case is detected.

Signed-off-by: Bryan Donlan <bdonlan@gmail.com>
---
 fs/ext2/namei.c |   12 ++++++++++--
 fs/ext3/namei.c |   12 ++++++++++--
 fs/ext4/namei.c |   12 ++++++++++--
 3 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/fs/ext2/namei.c b/fs/ext2/namei.c
index 90ea179..e3d2d34 100644
--- a/fs/ext2/namei.c
+++ b/fs/ext2/namei.c
@@ -66,8 +66,16 @@ static struct dentry *ext2_lookup(struct inode * dir, struct dentry *dentry, str
 	inode = NULL;
 	if (ino) {
 		inode = ext2_iget(dir->i_sb, ino);
-		if (IS_ERR(inode))
-			return ERR_CAST(inode);
+		if (unlikely(IS_ERR(inode))) {
+			if (PTR_ERR(inode) == -ESTALE) {
+				ext2_error(dir->i_sb, "ext2_lookup",
+						"deleted inode referenced: %lu",
+						ino);
+				return ERR_PTR(-EIO);
+			} else {
+				return ERR_CAST(inode);
+			}
+		}
 	}
 	return d_splice_alias(inode, dentry);
 }
diff --git a/fs/ext3/namei.c b/fs/ext3/namei.c
index 4db4ffa..7a99af0 100644
--- a/fs/ext3/namei.c
+++ b/fs/ext3/namei.c
@@ -1047,8 +1047,16 @@ static struct dentry *ext3_lookup(struct inode * dir, struct dentry *dentry, str
 			return ERR_PTR(-EIO);
 		}
 		inode = ext3_iget(dir->i_sb, ino);
-		if (IS_ERR(inode))
-			return ERR_CAST(inode);
+		if (unlikely(IS_ERR(inode))) {
+			if (PTR_ERR(inode) == -ESTALE) {
+				ext3_error(dir->i_sb, "ext2_lookup",
+						"deleted inode referenced: %lu",
+						ino);
+				return ERR_PTR(-EIO);
+			} else {
+				return ERR_CAST(inode);
+			}
+		}
 	}
 	return d_splice_alias(inode, dentry);
 }
diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index ba702bd..a61637b 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -1052,8 +1052,16 @@ static struct dentry *ext4_lookup(struct inode *dir, struct dentry *dentry, stru
 			return ERR_PTR(-EIO);
 		}
 		inode = ext4_iget(dir->i_sb, ino);
-		if (IS_ERR(inode))
-			return ERR_CAST(inode);
+		if (unlikely(IS_ERR(inode))) {
+			if (PTR_ERR(inode) == -ESTALE) {
+				ext4_error(dir->i_sb, "ext2_lookup",
+						"deleted inode referenced: %u",
+						ino);
+				return ERR_PTR(-EIO);
+			} else {
+				return ERR_CAST(inode);
+			}
+		}
 	}
 	return d_splice_alias(inode, dentry);
 }
-- 
1.5.6.3

Re: [PATCH v2] ext[234]: Return -EIO not -ESTALE on directory traversal through deleted inode

[Bryan Donlan]

On Mon, Feb 16, 2009 at 8:10 PM, Bryan Donlan <bdonlan@gmail.com> wrote:

> +               if (unlikely(IS_ERR(inode))) {
> +                       if (PTR_ERR(inode) == -ESTALE) {
> +                               ext3_error(dir->i_sb, "ext2_lookup",
> +                                               "deleted inode referenced: %lu",
> +                                               ino);
> +                               return ERR_PTR(-EIO);
> +                       } else {
> +                               return ERR_CAST(inode);
> +                       }
> +               }

I just noticed that I forgot to edit the function name in the
ext3_error and ext4_error invocations... Would it be better to send a
delta to fix this or resubmit the whole thing?

Re: [PATCH v2] ext[234]: Return -EIO not -ESTALE on directory traversal through deleted inode

[Bryan Donlan]

On Mon, Feb 16, 2009 at 8:10 PM, Bryan Donlan <bdonlan@gmail.com> wrote:

> +               if (unlikely(IS_ERR(inode))) {
> +                       if (PTR_ERR(inode) == -ESTALE) {
> +                               ext3_error(dir->i_sb, "ext2_lookup",
> +                                               "deleted inode referenced: %lu",
> +                                               ino);
> +                               return ERR_PTR(-EIO);
> +                       } else {
> +                               return ERR_CAST(inode);
> +                       }
> +               }

I just noticed that I forgot to edit the function name in the
ext3_error and ext4_error invocations... Would it be better to send a
delta to fix this or resubmit the whole thing?

Re: [PATCH v2] ext[234]: Return -EIO not -ESTALE on directory traversal through deleted inode

[Theodore Tso]

On Fri, Feb 20, 2009 at 12:53:39PM -0500, Bryan Donlan wrote:
> On Mon, Feb 16, 2009 at 8:10 PM, Bryan Donlan <bdonlan@gmail.com> wrote:
> 
> > +               if (unlikely(IS_ERR(inode))) {
> > +                       if (PTR_ERR(inode) == -ESTALE) {
> > +                               ext3_error(dir->i_sb, "ext2_lookup",
> > +                                               "deleted inode referenced: %lu",
> > +                                               ino);
> > +                               return ERR_PTR(-EIO);
> > +                       } else {
> > +                               return ERR_CAST(inode);
> > +                       }
> > +               }
> 
> I just noticed that I forgot to edit the function name in the
> ext3_error and ext4_error invocations... Would it be better to send a
> delta to fix this or resubmit the whole thing?

It's already been pulled into akpm's tree as separate patches.  I'll
fix up the ext4 one by hand; probably better for you to send
replacement patches for ext3 separately to akpm and ask him to replace.

I'd suggest using __FUNC__ instead of hard-coding the function name, BTW...

    	    	  	   	      		  - Ted

Re: [PATCH v2] ext[234]: Return -EIO not -ESTALE on directory traversal through deleted inode

[Bryan Donlan]

On Fri, Feb 20, 2009 at 12:59 PM, Theodore Tso <tytso@mit.edu> wrote:
> On Fri, Feb 20, 2009 at 12:53:39PM -0500, Bryan Donlan wrote:
>> On Mon, Feb 16, 2009 at 8:10 PM, Bryan Donlan <bdonlan@gmail.com> wrote:
>>
>> > +               if (unlikely(IS_ERR(inode))) {
>> > +                       if (PTR_ERR(inode) == -ESTALE) {
>> > +                               ext3_error(dir->i_sb, "ext2_lookup",
>> > +                                               "deleted inode referenced: %lu",
>> > +                                               ino);
>> > +                               return ERR_PTR(-EIO);
>> > +                       } else {
>> > +                               return ERR_CAST(inode);
>> > +                       }
>> > +               }
>>
>> I just noticed that I forgot to edit the function name in the
>> ext3_error and ext4_error invocations... Would it be better to send a
>> delta to fix this or resubmit the whole thing?
>
> It's already been pulled into akpm's tree as separate patches.  I'll
> fix up the ext4 one by hand; probably better for you to send
> replacement patches for ext3 separately to akpm and ask him to replace.
>
> I'd suggest using __FUNC__ instead of hard-coding the function name, BTW...

akpm's already fixed up his copies. Sorry for the inconvenience there
- I made sure to get the printf format specifiers right but on the
other hand completely missed the comparatively obvious function name
:)

Thanks,

Bryan Donlan

Re: [PATCH v2] ext[234]: Return -EIO not -ESTALE on directory traversal through deleted inode

[Bryan Donlan]

On Fri, Feb 20, 2009 at 12:59 PM, Theodore Tso <tytso@mit.edu> wrote:
> On Fri, Feb 20, 2009 at 12:53:39PM -0500, Bryan Donlan wrote:
>> On Mon, Feb 16, 2009 at 8:10 PM, Bryan Donlan <bdonlan@gmail.com> wrote:
>>
>> > +               if (unlikely(IS_ERR(inode))) {
>> > +                       if (PTR_ERR(inode) == -ESTALE) {
>> > +                               ext3_error(dir->i_sb, "ext2_lookup",
>> > +                                               "deleted inode referenced: %lu",
>> > +                                               ino);
>> > +                               return ERR_PTR(-EIO);
>> > +                       } else {
>> > +                               return ERR_CAST(inode);
>> > +                       }
>> > +               }
>>
>> I just noticed that I forgot to edit the function name in the
>> ext3_error and ext4_error invocations... Would it be better to send a
>> delta to fix this or resubmit the whole thing?
>
> It's already been pulled into akpm's tree as separate patches.  I'll
> fix up the ext4 one by hand; probably better for you to send
> replacement patches for ext3 separately to akpm and ask him to replace.
>
> I'd suggest using __FUNC__ instead of hard-coding the function name, BTW...

akpm's already fixed up his copies. Sorry for the inconvenience there
- I made sure to get the printf format specifiers right but on the
other hand completely missed the comparatively obvious function name
:)

Thanks,

Bryan Donlan

Re: [PATCH v2] ext[234]: Return -EIO not -ESTALE on directory traversal through deleted inode

[Andrew Morton]

On Fri, 20 Feb 2009 12:53:39 -0500 Bryan Donlan <bdonlan@gmail.com> wrote:

> On Mon, Feb 16, 2009 at 8:10 PM, Bryan Donlan <bdonlan@gmail.com> wrote:
> 
> > +               if (unlikely(IS_ERR(inode))) {
> > +                       if (PTR_ERR(inode) == -ESTALE) {
> > +                               ext3_error(dir->i_sb, "ext2_lookup",
> > +                                               "deleted inode referenced: %lu",
> > +                                               ino);
> > +                               return ERR_PTR(-EIO);
> > +                       } else {
> > +                               return ERR_CAST(inode);
> > +                       }
> > +               }
> 
> I just noticed that I forgot to edit the function name in the
> ext3_error and ext4_error invocations... Would it be better to send a
> delta to fix this or resubmit the whole thing?

I just edited the diffs on my copies - switched all three to __func__.

Re: [PATCH v2] ext[234]: Return -EIO not -ESTALE on directory traversal through deleted inode

[Andrew Morton]

On Fri, 20 Feb 2009 12:53:39 -0500 Bryan Donlan <bdonlan@gmail.com> wrote:

> On Mon, Feb 16, 2009 at 8:10 PM, Bryan Donlan <bdonlan@gmail.com> wrote:
> 
> > +               if (unlikely(IS_ERR(inode))) {
> > +                       if (PTR_ERR(inode) == -ESTALE) {
> > +                               ext3_error(dir->i_sb, "ext2_lookup",
> > +                                               "deleted inode referenced: %lu",
> > +                                               ino);
> > +                               return ERR_PTR(-EIO);
> > +                       } else {
> > +                               return ERR_CAST(inode);
> > +                       }
> > +               }
> 
> I just noticed that I forgot to edit the function name in the
> ext3_error and ext4_error invocations... Would it be better to send a
> delta to fix this or resubmit the whole thing?

I just edited the diffs on my copies - switched all three to __func__.