[PATCH] xfs: Fix undefined behavior of shift into sign bit

[PATCH] xfs: Fix undefined behavior of shift into sign bit

[Geert Uytterhoeven]

With gcc-5:

    In file included from ./include/trace/define_trace.h:102:0,
		     from ./fs/xfs/scrub/trace.h:988,
		     from fs/xfs/scrub/trace.c:40:
    ./fs/xfs/./scrub/trace.h: In function ‘trace_raw_output_xchk_fsgate_class’:
    ./fs/xfs/scrub/scrub.h:111:28: error: initializer element is not constant
     #define XREP_ALREADY_FIXED (1 << 31) /* checking our repair work */
				^

Shifting the (signed) value 1 into the sign bit is undefined behavior.

Fix this for all definitions in the file by shifting "1U" instead of
"1".

This was exposed by the first user added in commit 466c525d6d35e691
("xfs: minimize overhead of drain wakeups by using jump labels").

Fixes: 160b5a784525e8a4 ("xfs: hoist the already_fixed variable to the scrub context")
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 fs/xfs/scrub/scrub.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/xfs/scrub/scrub.h b/fs/xfs/scrub/scrub.h
index b38e93830ddea2b0..e113f2f5c254b085 100644
--- a/fs/xfs/scrub/scrub.h
+++ b/fs/xfs/scrub/scrub.h
@@ -105,10 +105,10 @@ struct xfs_scrub {
 };
 
 /* XCHK state flags grow up from zero, XREP state flags grown down from 2^31 */
-#define XCHK_TRY_HARDER		(1 << 0)  /* can't get resources, try again */
-#define XCHK_FSGATES_DRAIN	(1 << 2)  /* defer ops draining enabled */
-#define XCHK_NEED_DRAIN		(1 << 3)  /* scrub needs to drain defer ops */
-#define XREP_ALREADY_FIXED	(1 << 31) /* checking our repair work */
+#define XCHK_TRY_HARDER		(1U << 0)  /* can't get resources, try again */
+#define XCHK_FSGATES_DRAIN	(1U << 2)  /* defer ops draining enabled */
+#define XCHK_NEED_DRAIN		(1U << 3)  /* scrub needs to drain defer ops */
+#define XREP_ALREADY_FIXED	(1U << 31) /* checking our repair work */
 
 /*
  * The XCHK_FSGATES* flags reflect functionality in the main filesystem that
-- 
2.34.1

Re: [PATCH] xfs: Fix undefined behavior of shift into sign bit

[Darrick J. Wong]

On Mon, May 08, 2023 at 02:06:34PM +0200, Geert Uytterhoeven wrote:
> With gcc-5:
> 
>     In file included from ./include/trace/define_trace.h:102:0,
> 		     from ./fs/xfs/scrub/trace.h:988,
> 		     from fs/xfs/scrub/trace.c:40:
>     ./fs/xfs/./scrub/trace.h: In function ‘trace_raw_output_xchk_fsgate_class’:
>     ./fs/xfs/scrub/scrub.h:111:28: error: initializer element is not constant
>      #define XREP_ALREADY_FIXED (1 << 31) /* checking our repair work */
> 				^
> 
> Shifting the (signed) value 1 into the sign bit is undefined behavior.
> 
> Fix this for all definitions in the file by shifting "1U" instead of
> "1".
> 
> This was exposed by the first user added in commit 466c525d6d35e691
> ("xfs: minimize overhead of drain wakeups by using jump labels").
> 
> Fixes: 160b5a784525e8a4 ("xfs: hoist the already_fixed variable to the scrub context")
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
>  fs/xfs/scrub/scrub.h | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/xfs/scrub/scrub.h b/fs/xfs/scrub/scrub.h
> index b38e93830ddea2b0..e113f2f5c254b085 100644
> --- a/fs/xfs/scrub/scrub.h
> +++ b/fs/xfs/scrub/scrub.h
> @@ -105,10 +105,10 @@ struct xfs_scrub {
>  };
>  
>  /* XCHK state flags grow up from zero, XREP state flags grown down from 2^31 */
> -#define XCHK_TRY_HARDER		(1 << 0)  /* can't get resources, try again */
> -#define XCHK_FSGATES_DRAIN	(1 << 2)  /* defer ops draining enabled */
> -#define XCHK_NEED_DRAIN		(1 << 3)  /* scrub needs to drain defer ops */
> -#define XREP_ALREADY_FIXED	(1 << 31) /* checking our repair work */
> +#define XCHK_TRY_HARDER		(1U << 0)  /* can't get resources, try again */
> +#define XCHK_FSGATES_DRAIN	(1U << 2)  /* defer ops draining enabled */
> +#define XCHK_NEED_DRAIN		(1U << 3)  /* scrub needs to drain defer ops */
> +#define XREP_ALREADY_FIXED	(1U << 31) /* checking our repair work */

DOH.  My mistake. :(

Does gcc have an explicit warning for that?  I turned on W=12e on gcc
11.3 and UBSAN and neither complain about this.

Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

>  
>  /*
>   * The XCHK_FSGATES* flags reflect functionality in the main filesystem that
> -- 
> 2.34.1
>