Re: [PATCH net 1/2] mlxsw: spectrum_acl_bloom_filter: Expand chunk_key_offsets[chunk_index]

[WangYuli]

Hi Ido Schimmel,

On 2025/3/13 23:25, Ido Schimmel wrote:

> I would like to get a warning if 'chunk_count' is larger than
> 'MLXSW_BLOOM_KEY_CHUNKS' since it should never happen.
>
> I was able to reproduce the build failure and the following patch seems
> to solve it for me. It removes the 'max_chunks' argument since it's
> always 'MLXSW_BLOOM_KEY_CHUNKS' (3) and verifies that the number of
> chunks that was calculated is never greater than it.
>
> WangYuli, can you please test it?
>

My tests still show the same compilation failing.

Indeed, I have already tried similar fixes during my investigation, but to no avail.

> Also, if you want it in net.git (as opposed to net-next.git), then it
> needs a Fixes tag:
>
> Fixes: 7585cacdb978 ("mlxsw: spectrum_acl: Add Bloom filter handling")
>     
    OK
> And I don't think we need patch #2.
>     
    While #2 is admittedly a secondary modification compared to #1,
    should you be agreeable, I believe it's still preferable to combine
    #2 with #1 instead of outright discarding #2.
> diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_acl_bloom_filter.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_acl_bloom_filter.c
> index a54eedb69a3f..a1ab5b09a6c5 100644
> --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_acl_bloom_filter.c
> +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_acl_bloom_filter.c
> @@ -231,7 +231,7 @@ static u16 mlxsw_sp2_acl_bf_crc(const u8 *buffer, size_t len)
>  static void
>  __mlxsw_sp_acl_bf_key_encode(struct mlxsw_sp_acl_atcam_region *aregion,
>  			     struct mlxsw_sp_acl_atcam_entry *aentry,
> -			     char *output, u8 *len, u8 max_chunks, u8 pad_bytes,
> +			     char *output, u8 *len, u8 pad_bytes,
>  			     u8 key_offset, u8 chunk_key_len, u8 chunk_len)
>  {
>  	struct mlxsw_afk_key_info *key_info = aregion->region->key_info;
> @@ -241,10 +241,14 @@ __mlxsw_sp_acl_bf_key_encode(struct mlxsw_sp_acl_atcam_region *aregion,
>  
>  	block_count = mlxsw_afk_key_info_blocks_count_get(key_info);
>  	chunk_count = 1 + ((block_count - 1) >> 2);
> +	if (WARN_ON_ONCE(chunk_count > MLXSW_BLOOM_KEY_CHUNKS)) {
> +		*len = 0;
> +		return;
> +	}
>  	erp_region_id = cpu_to_be16(aentry->ht_key.erp_id |
>  				   (aregion->region->id << 4));
> -	for (chunk_index = max_chunks - chunk_count; chunk_index < max_chunks;
> -	     chunk_index++) {
> +	for (chunk_index = MLXSW_BLOOM_KEY_CHUNKS - chunk_count;
> +	     chunk_index < MLXSW_BLOOM_KEY_CHUNKS; chunk_index++) {
>  		memset(chunk, 0, pad_bytes);
>  		memcpy(chunk + pad_bytes, &erp_region_id,
>  		       sizeof(erp_region_id));
> @@ -262,7 +266,6 @@ mlxsw_sp2_acl_bf_key_encode(struct mlxsw_sp_acl_atcam_region *aregion,
>  			    char *output, u8 *len)
>  {
>  	__mlxsw_sp_acl_bf_key_encode(aregion, aentry, output, len,
> -				     MLXSW_BLOOM_KEY_CHUNKS,
>  				     MLXSW_SP2_BLOOM_CHUNK_PAD_BYTES,
>  				     MLXSW_SP2_BLOOM_CHUNK_KEY_OFFSET,
>  				     MLXSW_SP2_BLOOM_CHUNK_KEY_BYTES,
> @@ -379,7 +382,6 @@ mlxsw_sp4_acl_bf_key_encode(struct mlxsw_sp_acl_atcam_region *aregion,
>  	u8 chunk_count = 1 + ((block_count - 1) >> 2);
>  
>  	__mlxsw_sp_acl_bf_key_encode(aregion, aentry, output, len,
> -				     MLXSW_BLOOM_KEY_CHUNKS,
>  				     MLXSW_SP4_BLOOM_CHUNK_PAD_BYTES,
>  				     MLXSW_SP4_BLOOM_CHUNK_KEY_OFFSET,
>  				     MLXSW_SP4_BLOOM_CHUNK_KEY_BYTES,
>
>

--
WangYuli

Attachment: OpenPGP_0xC5DA1F3046F40BEE.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature