Re: [PATCH 1/4] libxfs: avoid negative (and full-width) shifts in radix-tree.c

Brian Foster
2015-10-09T13:23:44Z

On Thu, Oct 08, 2015 at 07:24:53PM -0500, Eric Sandeen wrote:
> Pull this commit over from the kernel, as libubsan spotted a
> bad shift at runtime:
>
> commit 430d275a399175c7c0673459738979287ec1fd22
> Author: Peter Lund <firefly@vax64.dk>
> Date: Tue Oct 16 23:29:35 2007 -0700
>
> avoid negative (and full-width) shifts in radix-tree.c
>
> Negative shifts are not allowed in C (the result is undefined). Same thing
> with full-width shifts.
>
> It works on most platforms but not on the VAX with gcc 4.0.1 (it results in an
> "operand reserved" fault).
>
> Shifting by more than the width of the value on the left is also not
> allowed. I think the extra '>> 1' tacked on at the end in the original
> code was an attempt to work around that. Getting rid of that is an extra
> feature of this patch.
>
> Here's the chapter and verse, taken from the final draft of the C99
> standard ("6.5.7 Bitwise shift operators", paragraph 3):
>
> "The integer promotions are performed on each of the operands. The
> type of the result is that of the promoted left operand. If the
> value of the right operand is negative or is greater than or equal
> to the width of the promoted left operand, the behavior is
> undefined."
>
> Thank you to Jan-Benedict Glaw, Christoph Hellwig, Maciej Rozycki, Pekka
> Enberg, Andreas Schwab, and Christoph Lameter for review. Special thanks
> to Andreas for spotting that my fix only removed half the undefined
> behaviour.
>
> Signed-off-by: Peter Lund <firefly@vax64.dk>
>
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

> libxfs/radix-tree.c | 14 ++++++++------
> 1 files changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/libxfs/radix-tree.c b/libxfs/radix-tree.c
> index 4d44ab4..eef9c36 100644
> --- a/libxfs/radix-tree.c
> +++ b/libxfs/radix-tree.c
> @@ -784,12 +784,14 @@ int radix_tree_tagged(struct radix_tree_root *root, unsigned int tag)
>
> static unsigned long __maxindex(unsigned int height)
> {
> - unsigned int tmp = height * RADIX_TREE_MAP_SHIFT;
> - unsigned long index = (~0UL >> (RADIX_TREE_INDEX_BITS - tmp - 1)) >> 1;
> -
> - if (tmp >= RADIX_TREE_INDEX_BITS)
> - index = ~0UL;
> - return index;
> + unsigned int width = height * RADIX_TREE_MAP_SHIFT;
> + int shift = RADIX_TREE_INDEX_BITS - width;
> +
> + if (shift < 0)
> + return ~0UL;
> + if (shift >= BITS_PER_LONG)
> + return 0UL;
> + return ~0UL >> shift;
> }
>
> static void radix_tree_init_maxindex(void)
> --
> 1.7.1
>
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs