Re: [PATCH 10/10] misc: fix ubsan warnings

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Fri, Oct 27, 2017 at 08:48:07AM -0500, Eric Sandeen wrote:
> On 10/26/17 5:15 PM, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Fix all the things ubsan warned about.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  include/bitops.h   |    8 ++++----
> >  include/command.h  |    2 +-
> >  include/xfs_arch.h |    2 +-
> >  3 files changed, 6 insertions(+), 6 deletions(-)
> > 
> > 
> > diff --git a/include/bitops.h b/include/bitops.h
> > index 44599a7..53a5aa0 100644
> > --- a/include/bitops.h
> > +++ b/include/bitops.h
> > @@ -13,19 +13,19 @@ static inline int fls(int x)
> >  	if (!x)
> >  		return 0;
> >  	if (!(x & 0xffff0000u)) {
> > -		x <<= 16;
> > +		x = (x & 0xFFFFU) << 16;
> 
> xfsprogs almost exclusively uses lowercase hex - as it does in the line
> above your new code.  ;)  I can change it on the way in unless you object.

Ok, sounds good to me.

> >  		r -= 16;
> >  	}
> >  	if (!(x & 0xff000000u)) {
> > -		x <<= 8;
> > +		x = (x & 0xFFFFFFU) << 8;
> >  		r -= 8;
> >  	}
> >  	if (!(x & 0xf0000000u)) {
> > -		x <<= 4;
> > +		x = (x & 0xFFFFFFFU) << 4;
> >  		r -= 4;
> >  	}
> >  	if (!(x & 0xc0000000u)) {
> > -		x <<= 2;
> > +		x = (x & 0x3FFFFFFFU) << 2;
> >  		r -= 2;
> >  	}
> >  	if (!(x & 0x80000000u)) {
> > diff --git a/include/command.h b/include/command.h
> > index fb3f5c7..59eab96 100644
> > --- a/include/command.h
> > +++ b/include/command.h
> > @@ -25,7 +25,7 @@
> >   * not iterate the command args function callout and so can be used
> >   * for functions like "help" that should only ever be run once.
> >   */
> > -#define CMD_FLAG_ONESHOT	(1<<31)
> > +#define CMD_FLAG_ONESHOT	(1U<<31)
> >  #define CMD_FLAG_FOREIGN_OK	(1<<30)	/* command not restricted to XFS */
> >  #define CMD_FLAG_LIBRARY	(1<<29)	/* command provided by libxcmd */
> 
> What complained about this?  cmd flags is a signed int, right, so why is
> this a problem?  (And why are we only using the top 3 bits?  And let's just do 1U
> on every flag if it's really needed, but why is it needed?  I must be missing
> something.)

I think the complaint is that "1" is signed int, and ubsan doesn't like
us shifting a value bit into the sign bit.

But yeah, I suppose if we 'U'ize one of them we could do it for all.

(Though really, why not start with the lower bits?)

--D

> > diff --git a/include/xfs_arch.h b/include/xfs_arch.h
> > index 186cadb..34fcd4d 100644
> > --- a/include/xfs_arch.h
> > +++ b/include/xfs_arch.h
> > @@ -253,7 +253,7 @@ static inline uint16_t get_unaligned_be16(void *p)
> >  static inline uint32_t get_unaligned_be32(void *p)
> >  {
> >  	uint8_t *__p = p;
> > -        return __p[0] << 24 | __p[1] << 16 | __p[2] << 8 | __p[3];
> > +        return (uint32_t)__p[0] << 24 | __p[1] << 16 | __p[2] << 8 | __p[3];
> >  }
> >  
> >  static inline uint64_t get_unaligned_be64(void *p)
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html