| Message ID | Y6wCbyttJ+WVzmZX@gondor.apana.org.au (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | crypto: caam - Avoid GCC memset bug warning | expand |
On Wed, Dec 28, 2022 at 04:46:39PM +0800, Herbert Xu wrote: > Certain versions of gcc don't like the memcpy with a NULL dst > (which only happens with a zero length). This only happens > when debugging is enabled so add an if clause to work around > these warnings. > > A similar warning used to be generated by sparse but that was > fixed years ago. > > Link: https://lore.kernel.org/lkml/202210290446.qBayTfzl-lkp@intel.com > Reported-by: kernel test robot <lkp@intel.com> > Reported-by: Kees Cook <keescook@chromium.org> > Reported-by: Uwe Kleine-K�nig <u.kleine-koenig@pengutronix.de> Huh, broken encoding in the mail. I'd appreciate someone to doublecheck it's fine in the final commit. Tested-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Best regards Uwe
On Wed, Dec 28, 2022 at 04:46:39PM +0800, Herbert Xu wrote: > Certain versions of gcc don't like the memcpy with a NULL dst > (which only happens with a zero length). This only happens > when debugging is enabled so add an if clause to work around > these warnings. > > A similar warning used to be generated by sparse but that was > fixed years ago. > > Link: https://lore.kernel.org/lkml/202210290446.qBayTfzl-lkp@intel.com > Reported-by: kernel test robot <lkp@intel.com> > Reported-by: Kees Cook <keescook@chromium.org> > Reported-by: Uwe Kleine-K�nig <u.kleine-koenig@pengutronix.de> > Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> > > diff --git a/drivers/crypto/caam/desc_constr.h b/drivers/crypto/caam/desc_constr.h > index 62ce6421bb3f..824c94d44f94 100644 > --- a/drivers/crypto/caam/desc_constr.h > +++ b/drivers/crypto/caam/desc_constr.h > @@ -163,7 +163,8 @@ static inline void append_data(u32 * const desc, const void *data, int len) > { > u32 *offset = desc_end(desc); > > - if (len) /* avoid sparse warning: memcpy with byte count of 0 */ > + /* Avoid gcc warning: memcpy with data == NULL */ > + if (!IS_ENABLED(CONFIG_CRYPTO_DEV_FSL_CAAM_DEBUG) || data) I just tried: For me a plain if (data) is also enough to make both gcc and sparse happy. (On a related note, sparse reports: CHECK drivers/crypto/caam/jr.c drivers/crypto/caam/jr.c: note: in included file (through arch/arm64/include/asm/io.h, include/linux/io.h, include/linux/irq.h, ...): include/asm-generic/io.h:290:22: warning: incorrect type in argument 1 (different base types) include/asm-generic/io.h:290:22: expected unsigned long long [usertype] val include/asm-generic/io.h:290:22: got restricted __le64 [usertype] include/asm-generic/io.h:290:22: warning: incorrect type in argument 1 (different base types) include/asm-generic/io.h:290:22: expected unsigned long long [usertype] val include/asm-generic/io.h:290:22: got restricted __le64 [usertype] Didn't look into that though.) Best regards Uwe
On Wed, Dec 28, 2022 at 12:30:35PM +0100, Uwe Kleine-König wrote: > > > - if (len) /* avoid sparse warning: memcpy with byte count of 0 */ > > + /* Avoid gcc warning: memcpy with data == NULL */ > > + if (!IS_ENABLED(CONFIG_CRYPTO_DEV_FSL_CAAM_DEBUG) || data) > > I just tried: For me a plain > > if (data) > > is also enough to make both gcc and sparse happy. Of course it is. The point of the extra condition is to remove the unnecessary check on data unless we are in debugging mode (as it is only needed in debugging mode to work around the buggy compiler). > (On a related note, sparse reports: > > CHECK drivers/crypto/caam/jr.c > drivers/crypto/caam/jr.c: note: in included file (through arch/arm64/include/asm/io.h, include/linux/io.h, include/linux/irq.h, ...): > include/asm-generic/io.h:290:22: warning: incorrect type in argument 1 (different base types) > include/asm-generic/io.h:290:22: expected unsigned long long [usertype] val > include/asm-generic/io.h:290:22: got restricted __le64 [usertype] > include/asm-generic/io.h:290:22: warning: incorrect type in argument 1 (different base types) > include/asm-generic/io.h:290:22: expected unsigned long long [usertype] val > include/asm-generic/io.h:290:22: got restricted __le64 [usertype] That's a bug in include/asm-generic/io.h. It feeds an __le64 to __raw_writeq which wants a u64. Cheers,
From: Herbert Xu > Sent: 29 December 2022 01:49 > > On Wed, Dec 28, 2022 at 12:30:35PM +0100, Uwe Kleine-König wrote: > > > > > - if (len) /* avoid sparse warning: memcpy with byte count of 0 */ > > > + /* Avoid gcc warning: memcpy with data == NULL */ > > > + if (!IS_ENABLED(CONFIG_CRYPTO_DEV_FSL_CAAM_DEBUG) || data) > > > > I just tried: For me a plain > > > > if (data) > > > > is also enough to make both gcc and sparse happy. > > Of course it is. The point of the extra condition is to remove > the unnecessary check on data unless we are in debugging mode > (as it is only needed in debugging mode to work around the buggy > compiler). IIRC the 'problematic' case is one where 'len' and 'data' are actually compile-time zeros - in which case you don't want to call memcpy() at all. In all other cases I think there is something to copy so you don't really want the check (or the one in memcpy() will do). Whether (builtin_constant_p(data) && !data) is good enough is another matter. It might need the (sizeof *(1 ? (void *)(data) : (int *)0) == 1) test. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
diff --git a/drivers/crypto/caam/desc_constr.h b/drivers/crypto/caam/desc_constr.h index 62ce6421bb3f..824c94d44f94 100644 --- a/drivers/crypto/caam/desc_constr.h +++ b/drivers/crypto/caam/desc_constr.h @@ -163,7 +163,8 @@ static inline void append_data(u32 * const desc, const void *data, int len) { u32 *offset = desc_end(desc); - if (len) /* avoid sparse warning: memcpy with byte count of 0 */ + /* Avoid gcc warning: memcpy with data == NULL */ + if (!IS_ENABLED(CONFIG_CRYPTO_DEV_FSL_CAAM_DEBUG) || data) memcpy(offset, data, len); (*desc) = cpu_to_caam32(caam32_to_cpu(*desc) +
Certain versions of gcc don't like the memcpy with a NULL dst (which only happens with a zero length). This only happens when debugging is enabled so add an if clause to work around these warnings. A similar warning used to be generated by sparse but that was fixed years ago. Link: https://lore.kernel.org/lkml/202210290446.qBayTfzl-lkp@intel.com Reported-by: kernel test robot <lkp@intel.com> Reported-by: Kees Cook <keescook@chromium.org> Reported-by: Uwe Kleine-K�nig <u.kleine-koenig@pengutronix.de> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>