* [PATCH] xfsprogs: libxfs: Don't forget to initialize the radix tree subsystem
@ 2011-10-13 18:50 Alex Elder
2011-10-13 19:51 ` Eric Sandeen
` (2 more replies)
0 siblings, 3 replies; 4+ messages in thread
From: Alex Elder @ 2011-10-13 18:50 UTC (permalink / raw)
To: xfs
The libxfs code uses radix tree routines to manage a mount
point's m_perag_tree. But the radix tree routines assume
that radix_tree_init() has been called to initialize the
height_to_maxindex[] global array, and this was not being
done.
This showed up when running mkfs.xfs on an ia64 system. Since
it wasn't initialized, the array was filled with zeroes. The
first time radix_tree_extend() got called (with index 0), the
height would be set to 1 and all would seem fine.
The *second* time it got called (with index 1) a problem would
arise--though we were apparently "lucky" enough for it not to
matter. The following loop would simply reference invalid slots
beyond the end of the array until it happened upon one that was
non-zero. (I've expanded the function radix_tree_maxindex() here.)
/* Figure out what the height should be. */
height = root->height + 1;
while (index > height_to_maxindex[height])
height++;
As an example, this looped 1937 times before it found a non-zere
value that would cause it to break out of the loop.
Even that *seemed* to be OK. But at the end of mkfs.xfs, when
it calls libxfs_umount(), non-initialized "slots" are dereferenced
and we hit a fault.
Wow.
Signed-off-by: Alex Elder <aelder@sgi.com>
---
libxfs/init.c | 2 ++
1 file changed, 2 insertions(+)
Index: b/libxfs/init.c
===================================================================
--- a/libxfs/init.c
+++ b/libxfs/init.c
@@ -249,6 +249,8 @@ libxfs_init(libxfs_init_t *a)
fd = -1;
flags = (a->isreadonly | a->isdirect);
+ radix_tree_init();
+
if (a->volname) {
if(!check_open(a->volname,flags,&rawfile,&blockfile))
goto done;
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] xfsprogs: libxfs: Don't forget to initialize the radix tree subsystem
2011-10-13 18:50 [PATCH] xfsprogs: libxfs: Don't forget to initialize the radix tree subsystem Alex Elder
@ 2011-10-13 19:51 ` Eric Sandeen
2011-10-13 22:47 ` Dave Chinner
2011-10-14 12:34 ` Christoph Hellwig
2 siblings, 0 replies; 4+ messages in thread
From: Eric Sandeen @ 2011-10-13 19:51 UTC (permalink / raw)
To: Alex Elder; +Cc: xfs
On 10/13/11 1:50 PM, Alex Elder wrote:
> The libxfs code uses radix tree routines to manage a mount
> point's m_perag_tree. But the radix tree routines assume
> that radix_tree_init() has been called to initialize the
> height_to_maxindex[] global array, and this was not being
> done.
>
> This showed up when running mkfs.xfs on an ia64 system. Since
> it wasn't initialized, the array was filled with zeroes. The
> first time radix_tree_extend() got called (with index 0), the
> height would be set to 1 and all would seem fine.
>
> The *second* time it got called (with index 1) a problem would
> arise--though we were apparently "lucky" enough for it not to
> matter. The following loop would simply reference invalid slots
> beyond the end of the array until it happened upon one that was
> non-zero. (I've expanded the function radix_tree_maxindex() here.)
>
> /* Figure out what the height should be. */
> height = root->height + 1;
> while (index > height_to_maxindex[height])
> height++;
>
> As an example, this looped 1937 times before it found a non-zere
> value that would cause it to break out of the loop.
>
> Even that *seemed* to be OK. But at the end of mkfs.xfs, when
> it calls libxfs_umount(), non-initialized "slots" are dereferenced
> and we hit a fault.
>
> Wow.
>
> Signed-off-by: Alex Elder <aelder@sgi.com>
I suppose if we have an init function we may as well call it at
least once. ;)
Reviewed-by: Eric Sandeen <sandeen@redhat.com>
> ---
> libxfs/init.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> Index: b/libxfs/init.c
> ===================================================================
> --- a/libxfs/init.c
> +++ b/libxfs/init.c
> @@ -249,6 +249,8 @@ libxfs_init(libxfs_init_t *a)
> fd = -1;
> flags = (a->isreadonly | a->isdirect);
>
> + radix_tree_init();
> +
> if (a->volname) {
> if(!check_open(a->volname,flags,&rawfile,&blockfile))
> goto done;
>
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
>
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] xfsprogs: libxfs: Don't forget to initialize the radix tree subsystem
2011-10-13 18:50 [PATCH] xfsprogs: libxfs: Don't forget to initialize the radix tree subsystem Alex Elder
2011-10-13 19:51 ` Eric Sandeen
@ 2011-10-13 22:47 ` Dave Chinner
2011-10-14 12:34 ` Christoph Hellwig
2 siblings, 0 replies; 4+ messages in thread
From: Dave Chinner @ 2011-10-13 22:47 UTC (permalink / raw)
To: Alex Elder; +Cc: xfs
On Thu, Oct 13, 2011 at 01:50:14PM -0500, Alex Elder wrote:
> The libxfs code uses radix tree routines to manage a mount
> point's m_perag_tree. But the radix tree routines assume
> that radix_tree_init() has been called to initialize the
> height_to_maxindex[] global array, and this was not being
> done.
>
> This showed up when running mkfs.xfs on an ia64 system. Since
> it wasn't initialized, the array was filled with zeroes. The
> first time radix_tree_extend() got called (with index 0), the
> height would be set to 1 and all would seem fine.
>
> The *second* time it got called (with index 1) a problem would
> arise--though we were apparently "lucky" enough for it not to
> matter. The following loop would simply reference invalid slots
> beyond the end of the array until it happened upon one that was
> non-zero. (I've expanded the function radix_tree_maxindex() here.)
>
> /* Figure out what the height should be. */
> height = root->height + 1;
> while (index > height_to_maxindex[height])
> height++;
>
> As an example, this looped 1937 times before it found a non-zere
> value that would cause it to break out of the loop.
>
> Even that *seemed* to be OK. But at the end of mkfs.xfs, when
> it calls libxfs_umount(), non-initialized "slots" are dereferenced
> and we hit a fault.
>
> Wow.
>
> Signed-off-by: Alex Elder <aelder@sgi.com>
/me wonders why valgrind didn't catch that
Anyway, the fix looks good, and well caught!
Reviewed-by: Dave Chinner <dchinner@redhat.com>
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] xfsprogs: libxfs: Don't forget to initialize the radix tree subsystem
2011-10-13 18:50 [PATCH] xfsprogs: libxfs: Don't forget to initialize the radix tree subsystem Alex Elder
2011-10-13 19:51 ` Eric Sandeen
2011-10-13 22:47 ` Dave Chinner
@ 2011-10-14 12:34 ` Christoph Hellwig
2 siblings, 0 replies; 4+ messages in thread
From: Christoph Hellwig @ 2011-10-14 12:34 UTC (permalink / raw)
To: Alex Elder; +Cc: xfs
Looks good,
Reviewed-by: Christoph Hellwig <hch@lst.de>
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2011-10-14 12:34 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-10-13 18:50 [PATCH] xfsprogs: libxfs: Don't forget to initialize the radix tree subsystem Alex Elder
2011-10-13 19:51 ` Eric Sandeen
2011-10-13 22:47 ` Dave Chinner
2011-10-14 12:34 ` Christoph Hellwig
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox