From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from relay.sgi.com (relay3.corp.sgi.com [198.149.34.15]) by oss.sgi.com (Postfix) with ESMTP id 6DE3C7F47 for ; Fri, 9 Oct 2015 08:23:44 -0500 (CDT) Received: from cuda.sgi.com (cuda2.sgi.com [192.48.176.25]) by relay3.corp.sgi.com (Postfix) with ESMTP id EAC1DAC001 for ; Fri, 9 Oct 2015 06:23:43 -0700 (PDT) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by cuda.sgi.com with ESMTP id yljqQXQXofo8HlbO (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO) for ; Fri, 09 Oct 2015 06:23:39 -0700 (PDT) Date: Fri, 9 Oct 2015 09:23:37 -0400 From: Brian Foster Subject: Re: [PATCH 1/4] libxfs: avoid negative (and full-width) shifts in radix-tree.c Message-ID: <20151009132337.GB27982@bfoster.bfoster> References: <56170906.5090301@redhat.com> <56170955.9020105@sandeen.net> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <56170955.9020105@sandeen.net> List-Id: XFS Filesystem from SGI List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: xfs-bounces@oss.sgi.com Sender: xfs-bounces@oss.sgi.com To: Eric Sandeen Cc: xfs@oss.sgi.com 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 > 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 > > Signed-off-by: Eric Sandeen > --- Reviewed-by: Brian Foster > 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 _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs