| Message ID | 150905615569.28563.17233201567221670601.stgit@magnolia (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
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. > 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.) > 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
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 -- 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
On 10/27/17 11:14 AM, Darrick J. Wong wrote: > On Fri, Oct 27, 2017 at 08:48:07AM -0500, Eric Sandeen wrote: ... >>> 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. Oh right, duh. > 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?) Heh, so: io/init.h: #define CMD_NOFILE_OK (1<<0) /* command doesn't need an open file */ #define CMD_NOMAP_OK (1<<1) /* command doesn't need a mapped region */ #define CMD_FOREIGN_OK CMD_FLAG_FOREIGN_OK include/command.h: #define CMD_FLAG_ONESHOT (1<<31) #define CMD_FLAG_FOREIGN_OK (1<<30) /* command not restricted to XFS */ #define CMD_FLAG_LIBRARY (1<<29) /* command provided by libxcmd */ What could possibly go wrong? -Eric -- 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
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; 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 */ 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)