* [RESEND/PATCH] ext[234]: Return -EIO not -ESTALE on directory traversal missing inode [not found] <1234588099-8445-1-git-send-email-bdonlan@fushizen.net> @ 2009-02-14 5:18 ` Bryan Donlan 2009-02-14 14:14 ` Theodore Tso 0 siblings, 1 reply; 9+ messages in thread From: Bryan Donlan @ 2009-02-14 5:18 UTC (permalink / raw) To: linux-ext4, linux-kernel; +Cc: sct, akpm, adilger, tytso, 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [RESEND/PATCH] ext[234]: Return -EIO not -ESTALE on directory traversal missing inode 2009-02-14 5:18 ` [RESEND/PATCH] ext[234]: Return -EIO not -ESTALE on directory traversal missing inode Bryan Donlan @ 2009-02-14 14:14 ` Theodore Tso 2009-02-15 4:53 ` Bryan Donlan 2009-02-17 1:10 ` [PATCH v2] ext[234]: Return -EIO not -ESTALE on directory traversal through deleted inode Bryan Donlan 0 siblings, 2 replies; 9+ messages in thread From: Theodore Tso @ 2009-02-14 14:14 UTC (permalink / raw) To: Bryan Donlan; +Cc: linux-ext4, linux-kernel, sct, akpm, adilger 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RESEND/PATCH] ext[234]: Return -EIO not -ESTALE on directory traversal missing inode 2009-02-14 14:14 ` Theodore Tso @ 2009-02-15 4:53 ` Bryan Donlan 2009-02-15 5:39 ` Theodore Tso 2009-02-17 1:10 ` [PATCH v2] ext[234]: Return -EIO not -ESTALE on directory traversal through deleted inode Bryan Donlan 1 sibling, 1 reply; 9+ messages in thread From: Bryan Donlan @ 2009-02-15 4:53 UTC (permalink / raw) To: Theodore Tso, Bryan Donlan, linux-ext4, linux-kernel, sct, akpm, adilger 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RESEND/PATCH] ext[234]: Return -EIO not -ESTALE on directory traversal missing inode 2009-02-15 4:53 ` Bryan Donlan @ 2009-02-15 5:39 ` Theodore Tso 0 siblings, 0 replies; 9+ messages in thread From: Theodore Tso @ 2009-02-15 5:39 UTC (permalink / raw) To: Bryan Donlan; +Cc: linux-ext4, linux-kernel, sct, akpm, adilger 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2] ext[234]: Return -EIO not -ESTALE on directory traversal through deleted inode 2009-02-14 14:14 ` Theodore Tso 2009-02-15 4:53 ` Bryan Donlan @ 2009-02-17 1:10 ` Bryan Donlan 2009-02-20 17:53 ` Bryan Donlan 1 sibling, 1 reply; 9+ messages in thread From: Bryan Donlan @ 2009-02-17 1:10 UTC (permalink / raw) To: linux-ext4, linux-kernel; +Cc: sct, akpm, adilger, tytso, 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2] ext[234]: Return -EIO not -ESTALE on directory traversal through deleted inode 2009-02-17 1:10 ` [PATCH v2] ext[234]: Return -EIO not -ESTALE on directory traversal through deleted inode Bryan Donlan @ 2009-02-20 17:53 ` Bryan Donlan 2009-02-20 17:59 ` Theodore Tso 2009-02-20 18:25 ` Andrew Morton 0 siblings, 2 replies; 9+ messages in thread From: Bryan Donlan @ 2009-02-20 17:53 UTC (permalink / raw) To: linux-ext4, linux-kernel; +Cc: sct, akpm, adilger, tytso, 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? ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] ext[234]: Return -EIO not -ESTALE on directory traversal through deleted inode 2009-02-20 17:53 ` Bryan Donlan @ 2009-02-20 17:59 ` Theodore Tso 2009-02-21 7:35 ` Bryan Donlan 2009-02-20 18:25 ` Andrew Morton 1 sibling, 1 reply; 9+ messages in thread From: Theodore Tso @ 2009-02-20 17:59 UTC (permalink / raw) To: Bryan Donlan; +Cc: linux-ext4, linux-kernel, sct, akpm, adilger 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] ext[234]: Return -EIO not -ESTALE on directory traversal through deleted inode 2009-02-20 17:59 ` Theodore Tso @ 2009-02-21 7:35 ` Bryan Donlan 0 siblings, 0 replies; 9+ messages in thread From: Bryan Donlan @ 2009-02-21 7:35 UTC (permalink / raw) To: Theodore Tso, Bryan Donlan, linux-ext4, linux-kernel, sct, akpm, adilger 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] ext[234]: Return -EIO not -ESTALE on directory traversal through deleted inode 2009-02-20 17:53 ` Bryan Donlan 2009-02-20 17:59 ` Theodore Tso @ 2009-02-20 18:25 ` Andrew Morton 1 sibling, 0 replies; 9+ messages in thread From: Andrew Morton @ 2009-02-20 18:25 UTC (permalink / raw) To: Bryan Donlan; +Cc: linux-ext4, linux-kernel, sct, adilger, tytso 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__. ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2009-02-21 7:35 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1234588099-8445-1-git-send-email-bdonlan@fushizen.net>
2009-02-14 5:18 ` [RESEND/PATCH] ext[234]: Return -EIO not -ESTALE on directory traversal missing inode Bryan Donlan
2009-02-14 14:14 ` Theodore Tso
2009-02-15 4:53 ` Bryan Donlan
2009-02-15 5:39 ` Theodore Tso
2009-02-17 1:10 ` [PATCH v2] ext[234]: Return -EIO not -ESTALE on directory traversal through deleted inode Bryan Donlan
2009-02-20 17:53 ` Bryan Donlan
2009-02-20 17:59 ` Theodore Tso
2009-02-21 7:35 ` Bryan Donlan
2009-02-20 18:25 ` Andrew Morton
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).