* [PATCH] xfsprogs: fix inode crash in xfs_repair
@ 2013-08-13 22:13 Mark Tinguely
2013-08-14 6:40 ` Dave Chinner
0 siblings, 1 reply; 6+ messages in thread
From: Mark Tinguely @ 2013-08-13 22:13 UTC (permalink / raw)
To: xfs
[-- Attachment #1: xfs_progs-fix-repair-crash-new-ichunk.patch --]
[-- Type: text/plain, Size: 1702 bytes --]
Adding the lost+found in phase 6 could allocate an inode from
a new inode chunk. That newly created chunk was not around in
the scan phase, and is not in the avl tree which will result
in a NULL dereference.
This patch adds the newly created inode chunk and inodes as if
found in the scan phase.
Metadata dump available for future tests.
Signed-off-by: Mark Tinguely <tinguely@sgi.com>
---
repair/incore_ino.c | 2 +-
repair/phase6.c | 15 +++++++++++++++
2 files changed, 16 insertions(+), 1 deletion(-)
Index: b/repair/incore_ino.c
===================================================================
--- a/repair/incore_ino.c
+++ b/repair/incore_ino.c
@@ -700,7 +700,7 @@ get_inode_parent(ino_tree_node_t *irec,
return(0LL);
}
-static void
+void
alloc_ex_data(ino_tree_node_t *irec)
{
parent_list_t *ptbl;
Index: b/repair/phase6.c
===================================================================
--- a/repair/phase6.c
+++ b/repair/phase6.c
@@ -930,6 +930,21 @@ mk_orphanage(xfs_mount_t *mp)
irec = find_inode_rec(mp,
XFS_INO_TO_AGNO(mp, ino),
XFS_INO_TO_AGINO(mp, ino));
+
+ if (irec == NULL && XFS_INO_TO_AGNO(mp, ino) < mp->m_sb.sb_agcount &&
+ ip != NULL && ip->i_d.di_magic == XFS_DINODE_MAGIC) {
+
+ /*
+ * add the newly allocated inode chunk to the avl tree.
+ */
+ irec = set_inode_free_alloc(mp, XFS_INO_TO_AGNO(mp, ino),
+ XFS_INO_TO_AGINO(mp, ino));
+ alloc_ex_data(irec);
+
+ for (i = 0; i < XFS_INODES_PER_CHUNK; i++)
+ set_inode_free(irec, i);
+ }
+
ino_offset = get_inode_offset(mp, ino, irec);
/*
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] xfsprogs: fix inode crash in xfs_repair
2013-08-13 22:13 [PATCH] xfsprogs: fix inode crash in xfs_repair Mark Tinguely
@ 2013-08-14 6:40 ` Dave Chinner
2013-08-14 13:33 ` Mark Tinguely
0 siblings, 1 reply; 6+ messages in thread
From: Dave Chinner @ 2013-08-14 6:40 UTC (permalink / raw)
To: Mark Tinguely; +Cc: xfs
On Tue, Aug 13, 2013 at 05:13:31PM -0500, Mark Tinguely wrote:
> Adding the lost+found in phase 6 could allocate an inode from
> a new inode chunk. That newly created chunk was not around in
> the scan phase, and is not in the avl tree which will result
> in a NULL dereference.
>
> This patch adds the newly created inode chunk and inodes as if
> found in the scan phase.
>
> Metadata dump available for future tests.
>
> Signed-off-by: Mark Tinguely <tinguely@sgi.com>
> ---
> repair/incore_ino.c | 2 +-
> repair/phase6.c | 15 +++++++++++++++
> 2 files changed, 16 insertions(+), 1 deletion(-)
>
> Index: b/repair/incore_ino.c
> ===================================================================
> --- a/repair/incore_ino.c
> +++ b/repair/incore_ino.c
> @@ -700,7 +700,7 @@ get_inode_parent(ino_tree_node_t *irec,
> return(0LL);
> }
>
> -static void
> +void
> alloc_ex_data(ino_tree_node_t *irec)
> {
> parent_list_t *ptbl;
> Index: b/repair/phase6.c
> ===================================================================
> --- a/repair/phase6.c
> +++ b/repair/phase6.c
> @@ -930,6 +930,21 @@ mk_orphanage(xfs_mount_t *mp)
> irec = find_inode_rec(mp,
> XFS_INO_TO_AGNO(mp, ino),
> XFS_INO_TO_AGINO(mp, ino));
> +
> + if (irec == NULL && XFS_INO_TO_AGNO(mp, ino) < mp->m_sb.sb_agcount &&
> + ip != NULL && ip->i_d.di_magic == XFS_DINODE_MAGIC) {
I don't understand this check.
We've already dereferenced ip several lines above to increment the
link count and get the inode number stored in ino, so the ip != NULL
is unnecessary.
We've just allocated the inode, so why would the magic number be
wrong? And why would the inode number lie in a non-existent
allocation group?
> + /*
> + * add the newly allocated inode chunk to the avl tree.
> + */
I can see from the code we are allocating and irec, inserting it
into the AVL tree and marking all the inodes in the chunk as free.
The comment should explain *why* we need to do this.
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] xfsprogs: fix inode crash in xfs_repair
2013-08-14 6:40 ` Dave Chinner
@ 2013-08-14 13:33 ` Mark Tinguely
2013-08-15 0:33 ` Dave Chinner
0 siblings, 1 reply; 6+ messages in thread
From: Mark Tinguely @ 2013-08-14 13:33 UTC (permalink / raw)
To: Dave Chinner; +Cc: xfs
On 08/14/13 01:40, Dave Chinner wrote:
> On Tue, Aug 13, 2013 at 05:13:31PM -0500, Mark Tinguely wrote:
>> Adding the lost+found in phase 6 could allocate an inode from
>> a new inode chunk. That newly created chunk was not around in
>> the scan phase, and is not in the avl tree which will result
>> in a NULL dereference.
>>
>> This patch adds the newly created inode chunk and inodes as if
>> found in the scan phase.
>>
>> Metadata dump available for future tests.
>>
>> Signed-off-by: Mark Tinguely<tinguely@sgi.com>
>> ---
>> repair/incore_ino.c | 2 +-
>> repair/phase6.c | 15 +++++++++++++++
>> 2 files changed, 16 insertions(+), 1 deletion(-)
>>
>> Index: b/repair/incore_ino.c
>> ===================================================================
>> --- a/repair/incore_ino.c
>> +++ b/repair/incore_ino.c
>> @@ -700,7 +700,7 @@ get_inode_parent(ino_tree_node_t *irec,
>> return(0LL);
>> }
>>
>> -static void
>> +void
>> alloc_ex_data(ino_tree_node_t *irec)
>> {
>> parent_list_t *ptbl;
>> Index: b/repair/phase6.c
>> ===================================================================
>> --- a/repair/phase6.c
>> +++ b/repair/phase6.c
>> @@ -930,6 +930,21 @@ mk_orphanage(xfs_mount_t *mp)
>> irec = find_inode_rec(mp,
>> XFS_INO_TO_AGNO(mp, ino),
>> XFS_INO_TO_AGINO(mp, ino));
>> +
>> + if (irec == NULL&& XFS_INO_TO_AGNO(mp, ino)< mp->m_sb.sb_agcount&&
>> + ip != NULL&& ip->i_d.di_magic == XFS_DINODE_MAGIC) {
>
> I don't understand this check.
>
> We've already dereferenced ip several lines above to increment the
> link count and get the inode number stored in ino, so the ip != NULL
> is unnecessary.
>
> We've just allocated the inode, so why would the magic number be
> wrong? And why would the inode number lie in a non-existent
> allocation group?
>
just being being paranoid.
>> + /*
>> + * add the newly allocated inode chunk to the avl tree.
>> + */
>
> I can see from the code we are allocating and irec, inserting it
> into the AVL tree and marking all the inodes in the chunk as free.
> The comment should explain *why* we need to do this.
>
> Cheers,
>
> Dave.
--Mark.
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] xfsprogs: fix inode crash in xfs_repair
2013-08-14 13:33 ` Mark Tinguely
@ 2013-08-15 0:33 ` Dave Chinner
2013-08-15 14:07 ` Mark Tinguely
0 siblings, 1 reply; 6+ messages in thread
From: Dave Chinner @ 2013-08-15 0:33 UTC (permalink / raw)
To: Mark Tinguely; +Cc: xfs
On Wed, Aug 14, 2013 at 08:33:03AM -0500, Mark Tinguely wrote:
> On 08/14/13 01:40, Dave Chinner wrote:
> >On Tue, Aug 13, 2013 at 05:13:31PM -0500, Mark Tinguely wrote:
> >>Adding the lost+found in phase 6 could allocate an inode from
> >>a new inode chunk. That newly created chunk was not around in
> >>the scan phase, and is not in the avl tree which will result
> >>in a NULL dereference.
> >>
> >>This patch adds the newly created inode chunk and inodes as if
> >>found in the scan phase.
> >>
> >>Metadata dump available for future tests.
> >>
> >>Signed-off-by: Mark Tinguely<tinguely@sgi.com>
> >>---
> >> repair/incore_ino.c | 2 +-
> >> repair/phase6.c | 15 +++++++++++++++
> >> 2 files changed, 16 insertions(+), 1 deletion(-)
> >>
> >>Index: b/repair/incore_ino.c
> >>===================================================================
> >>--- a/repair/incore_ino.c
> >>+++ b/repair/incore_ino.c
> >>@@ -700,7 +700,7 @@ get_inode_parent(ino_tree_node_t *irec,
> >> return(0LL);
> >> }
> >>
> >>-static void
> >>+void
> >> alloc_ex_data(ino_tree_node_t *irec)
> >> {
> >> parent_list_t *ptbl;
> >>Index: b/repair/phase6.c
> >>===================================================================
> >>--- a/repair/phase6.c
> >>+++ b/repair/phase6.c
> >>@@ -930,6 +930,21 @@ mk_orphanage(xfs_mount_t *mp)
> >> irec = find_inode_rec(mp,
> >> XFS_INO_TO_AGNO(mp, ino),
> >> XFS_INO_TO_AGINO(mp, ino));
> >>+
> >>+ if (irec == NULL&& XFS_INO_TO_AGNO(mp, ino)< mp->m_sb.sb_agcount&&
> >>+ ip != NULL&& ip->i_d.di_magic == XFS_DINODE_MAGIC) {
BTW, Mark, you're mailer is doing weird things to whitespace in code
when it's quoting quoted code.
> >I don't understand this check.
> >
> >We've already dereferenced ip several lines above to increment the
> >link count and get the inode number stored in ino, so the ip != NULL
> >is unnecessary.
> >
> >We've just allocated the inode, so why would the magic number be
> >wrong? And why would the inode number lie in a non-existent
> >allocation group?
> >
>
> just being being paranoid.
It's the same code as in the kernel - if is that broken that it
can't tell it didn't allocate a real inode, then we've got bigger
problems. We design the code to return an error when it fails so we
don't have to robustly check every possible error condition at every
call site. So really the only check needed is "if (!irec) {...}"
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] xfsprogs: fix inode crash in xfs_repair
2013-08-15 0:33 ` Dave Chinner
@ 2013-08-15 14:07 ` Mark Tinguely
2013-08-15 21:47 ` Dave Chinner
0 siblings, 1 reply; 6+ messages in thread
From: Mark Tinguely @ 2013-08-15 14:07 UTC (permalink / raw)
To: Dave Chinner; +Cc: xfs
On 08/14/13 19:33, Dave Chinner wrote:
> On Wed, Aug 14, 2013 at 08:33:03AM -0500, Mark Tinguely wrote:
>> On 08/14/13 01:40, Dave Chinner wrote:
>>> On Tue, Aug 13, 2013 at 05:13:31PM -0500, Mark Tinguely wrote:
>>>> Adding the lost+found in phase 6 could allocate an inode from
>>>> a new inode chunk. That newly created chunk was not around in
>>>> the scan phase, and is not in the avl tree which will result
>>>> in a NULL dereference.
>>>>
>>>> This patch adds the newly created inode chunk and inodes as if
>>>> found in the scan phase.
>>>>
>>>> Metadata dump available for future tests.
>>>>
>>>> Signed-off-by: Mark Tinguely<tinguely@sgi.com>
>>>> ---
>>>> repair/incore_ino.c | 2 +-
>>>> repair/phase6.c | 15 +++++++++++++++
>>>> 2 files changed, 16 insertions(+), 1 deletion(-)
>>>>
>>>> Index: b/repair/incore_ino.c
>>>> ===================================================================
>>>> --- a/repair/incore_ino.c
>>>> +++ b/repair/incore_ino.c
>>>> @@ -700,7 +700,7 @@ get_inode_parent(ino_tree_node_t *irec,
>>>> return(0LL);
>>>> }
>>>>
>>>> -static void
>>>> +void
>>>> alloc_ex_data(ino_tree_node_t *irec)
>>>> {
>>>> parent_list_t *ptbl;
>>>> Index: b/repair/phase6.c
>>>> ===================================================================
>>>> --- a/repair/phase6.c
>>>> +++ b/repair/phase6.c
>>>> @@ -930,6 +930,21 @@ mk_orphanage(xfs_mount_t *mp)
>>>> irec = find_inode_rec(mp,
>>>> XFS_INO_TO_AGNO(mp, ino),
>>>> XFS_INO_TO_AGINO(mp, ino));
>>>> +
>>>> + if (irec == NULL&& XFS_INO_TO_AGNO(mp, ino)< mp->m_sb.sb_agcount&&
>>>> + ip != NULL&& ip->i_d.di_magic == XFS_DINODE_MAGIC) {
>
> BTW, Mark, you're mailer is doing weird things to whitespace in code
> when it's quoting quoted code.
Thanks.
>
>>> I don't understand this check.
>>>
>>> We've already dereferenced ip several lines above to increment the
>>> link count and get the inode number stored in ino, so the ip != NULL
>>> is unnecessary.
>>>
>>> We've just allocated the inode, so why would the magic number be
>>> wrong? And why would the inode number lie in a non-existent
>>> allocation group?
>>>
>>
>> just being being paranoid.
>
> It's the same code as in the kernel - if is that broken that it
> can't tell it didn't allocate a real inode, then we've got bigger
> problems. We design the code to return an error when it fails so we
> don't have to robustly check every possible error condition at every
> call site. So really the only check needed is "if (!irec) {...}"
>
> Cheers,
>
> Dave.
The inode number check came from find_inode_rec(ag, agino)
(repair/incore.h). A bad inode number will also return a NULL irec.
--Mark.
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] xfsprogs: fix inode crash in xfs_repair
2013-08-15 14:07 ` Mark Tinguely
@ 2013-08-15 21:47 ` Dave Chinner
0 siblings, 0 replies; 6+ messages in thread
From: Dave Chinner @ 2013-08-15 21:47 UTC (permalink / raw)
To: Mark Tinguely; +Cc: xfs
On Thu, Aug 15, 2013 at 09:07:41AM -0500, Mark Tinguely wrote:
> On 08/14/13 19:33, Dave Chinner wrote:
> >On Wed, Aug 14, 2013 at 08:33:03AM -0500, Mark Tinguely wrote:
> >>>I don't understand this check.
> >>>
> >>>We've already dereferenced ip several lines above to increment the
> >>>link count and get the inode number stored in ino, so the ip != NULL
> >>>is unnecessary.
> >>>
> >>>We've just allocated the inode, so why would the magic number be
> >>>wrong? And why would the inode number lie in a non-existent
> >>>allocation group?
> >>>
> >>
> >>just being being paranoid.
> >
> >It's the same code as in the kernel - if is that broken that it
> >can't tell it didn't allocate a real inode, then we've got bigger
> >problems. We design the code to return an error when it fails so we
> >don't have to robustly check every possible error condition at every
> >call site. So really the only check needed is "if (!irec) {...}"
>
> The inode number check came from find_inode_rec(ag, agino)
> (repair/incore.h). A bad inode number will also return a NULL irec.
Yes, it will. But we just allocated the inode, so it's guaranteed to
have a valid inode number. We've got an inode *from a trusted
source* so we don't need to validate it the same way we need to
validate an inode number that we've pulled of disk that might be
corrupted.
Paranoia is great when using data from an untrusted source. But when
the data is from a trusted source, extreme paranoia is not necessary
and just makes the code hard to read...
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-08-15 21:48 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-08-13 22:13 [PATCH] xfsprogs: fix inode crash in xfs_repair Mark Tinguely
2013-08-14 6:40 ` Dave Chinner
2013-08-14 13:33 ` Mark Tinguely
2013-08-15 0:33 ` Dave Chinner
2013-08-15 14:07 ` Mark Tinguely
2013-08-15 21:47 ` Dave Chinner
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox