[PATCH] xfsprogs: libxfs: Don't forget to initialize the radix tree subsystem

[PATCH] xfsprogs: libxfs: Don't forget to initialize the radix tree subsystem

[Alex Elder]

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

Re: [PATCH] xfsprogs: libxfs: Don't forget to initialize the radix tree subsystem

[Eric Sandeen]

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

Re: [PATCH] xfsprogs: libxfs: Don't forget to initialize the radix tree subsystem

[Dave Chinner]

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

Re: [PATCH] xfsprogs: libxfs: Don't forget to initialize the radix tree subsystem

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs