Re: [PATCH net-next v2 2/2] net: ipa: fix IPA validation
From: Leon Romanovsky <leon@kernel.org>
Date: 2021-03-21 08:35:58
Also in:
lkml
On Sat, Mar 20, 2021 at 09:17:29AM -0500, Alex Elder wrote:
quoted hunk ↗ jump to hunk
There are blocks of IPA code that sanity-check various values, at compile time where possible. Most of these checks can be done once during development but skipped for normal operation. These checks permit the driver to make certain assumptions, thereby avoiding the need for runtime error checking. The checks are defined conditionally, but not consistently. In some cases IPA_VALIDATION enables the optional checks, while in others IPA_VALIDATE is used. Fix this by using IPA_VALIDATION consistently. Signed-off-by: Alex Elder <redacted> --- drivers/net/ipa/Makefile | 2 +- drivers/net/ipa/gsi_trans.c | 8 ++++---- drivers/net/ipa/ipa_cmd.c | 4 ++-- drivers/net/ipa/ipa_cmd.h | 6 +++--- drivers/net/ipa/ipa_endpoint.c | 6 +++--- drivers/net/ipa/ipa_main.c | 6 +++--- drivers/net/ipa/ipa_mem.c | 6 +++--- drivers/net/ipa/ipa_table.c | 6 +++--- drivers/net/ipa/ipa_table.h | 6 +++--- 9 files changed, 25 insertions(+), 25 deletions(-)diff --git a/drivers/net/ipa/Makefile b/drivers/net/ipa/Makefile index afe5df1e6eeee..014ae36ac6004 100644 --- a/drivers/net/ipa/Makefile +++ b/drivers/net/ipa/Makefile@@ -1,5 +1,5 @@ # Un-comment the next line if you want to validate configuration data -#ccflags-y += -DIPA_VALIDATE +# ccflags-y += -DIPA_VALIDATION
Maybe netdev folks think differently here, but general rule that dead code and closed code is such, is not acceptable to in Linux kernel. <...>
-#ifdef IPA_VALIDATE +#ifdef IPA_VALIDATION if (!size || size % 8) return -EINVAL; if (count < max_alloc) return -EINVAL; if (!max_alloc) return -EINVAL; -#endif /* IPA_VALIDATE */ +#endif /* IPA_VALIDATION */
If it is possible to supply those values, the check should be always and not only under some closed config option.
quoted hunk ↗ jump to hunk
/* By allocating a few extra entries in our pool (one less * than the maximum number that will be requested in a@@ -140,14 +140,14 @@ int gsi_trans_pool_init_dma(struct device *dev, struct gsi_trans_pool *pool, dma_addr_t addr; void *virt; -#ifdef IPA_VALIDATE +#ifdef IPA_VALIDATION if (!size || size % 8) return -EINVAL; if (count < max_alloc) return -EINVAL; if (!max_alloc) return -EINVAL; -#endif /* IPA_VALIDATE */ +#endif /* IPA_VALIDATION */
Same <...>
quoted hunk ↗ jump to hunk
{ -#ifdef IPA_VALIDATE +#ifdef IPA_VALIDATION /* At one time we assumed a 64-bit build, allowing some do_div() * calls to be replaced by simple division or modulo operations. * We currently only perform divide and modulo operations on u32,@@ -768,7 +768,7 @@ static void ipa_validate_build(void) BUILD_BUG_ON(!ipa_aggr_granularity_val(IPA_AGGR_GRANULARITY)); BUILD_BUG_ON(ipa_aggr_granularity_val(IPA_AGGR_GRANULARITY) > field_max(AGGR_GRANULARITY_FMASK)); -#endif /* IPA_VALIDATE */ +#endif /* IPA_VALIDATION */
BUILD_BUG_ON()s are checked during compilation and not during runtime like IPA_VALIDATION promised. IMHO, the issue here is that this IPA code isn't release quality but some debug drop variant and it is far from expected from submitted code. Thanks