Re: [PATCH RFC net] net/mlx5: Avoid NULL dereference in dest_is_valid

[Simon Horman <horms@kernel.org>]

On Wed, Jun 18, 2025 at 10:35:25PM +0300, Mark Bloch wrote:
> 
> 
> On 18/06/2025 21:27, Simon Horman wrote:
> > Elsewhere in dest_is_valid it is assumed that dest may be NULL.
> > But the line updated by this patch dereferences dest unconditionally.
> > This seems to be inconsistent.
> > 
> > Flagged by Smatch.
> > Compile tested only.
> > 
> > Fixes: ff189b435682 ("net/mlx5: Add ignore level support fwd to table rules")
> > Signed-off-by: Simon Horman <horms@kernel.org>
> > ---
> > I am posting this as an RFC as I am not completely sure this change is
> > necessary. F.e. an invariant that I'm unaware of may preclude dest
> > from being NULL in this case.
> 
> _mlx5_add_flow_rules() does:
>         for (i = 0; i < dest_num; i++) {
>                 if (!dest_is_valid(&dest[i], flow_act, ft))
>                         return ERR_PTR(-EINVAL);
>         }
> 
> so indeed dest must be valid inside dest_is_valid().\
> No defensive programming for this as all the callers are part of mlx5
> driver and they shouldn't pass dest_num that doesn't match how many
> dests are passed.
> 
> If anything I would drop the other checks for dest inside that function.
> I'll add that to my TODO list.
> 
> Thanks!

...

On Wed, Jun 18, 2025 at 12:35:37PM -0700, Saeed Mahameed wrote:
> On 18 Jun 19:27, Simon Horman wrote:

...

> > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/fs_core.c b/drivers/net/ethernet/mellanox/mlx5/core/fs_core.c
> > index a8046200d376..7eeab93a1aa9 100644
> > --- a/drivers/net/ethernet/mellanox/mlx5/core/fs_core.c
> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/fs_core.c
> > @@ -2041,7 +2041,8 @@ static bool dest_is_valid(struct mlx5_flow_destination *dest,
> > 		    ft->type != FS_FT_NIC_TX)
> > 			return false;
> > 
> > -		if (dest->type == MLX5_FLOW_DESTINATION_TYPE_FLOW_TABLE &&
> > +		if (dest &&
> 
> dest can't be null is it is always taken from entries of an array of
> mlx5_flow_destination.
> 
> 2 solutions:
>   1. remove other `if (dest && ...` checks in this function,      to not
> confuse smatch.
>   2. pass dest as value. there's only one caller of this function.      it
> is a 40B struct, don't know what the perf impact on flow rule
>      insertion rate.
> 
> I prefer solution 1.


Thanks Saeed and Mark for your quick response,
and sorry for the false positive.

I agree that removing the other dest checks makes sense.
And of course there is no urgency for that on my side.