Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 8 May 2023 08:29:52 -0700
From:   "Darrick J. Wong" <djwong@kernel.org>
To:     Geert Uytterhoeven <geert+renesas@glider.be>
Cc:     Dave Chinner <dchinner@redhat.com>, linux-xfs@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] xfs: Fix undefined behavior of shift into sign bit
Message-ID: <20230508152952.GA858799@frogsfrogsfrogs>
References: <20230508120634.2598765-1-geert+renesas@glider.be>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20230508120634.2598765-1-geert+renesas@glider.be>
Precedence: bulk

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
>