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

[Saeed Mahameed <saeed@kernel.org>]

On 18 Jun 19:27, Simon Horman wrote:
>Elsewhere in dest_is_valid it is assumed that dest may be NULL.
It can't be null, so we need to remove other checks elsewhere.
see below:

>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.
>---
> drivers/net/ethernet/mellanox/mlx5/core/fs_core.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
>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.

>+		    dest->type == MLX5_FLOW_DESTINATION_TYPE_FLOW_TABLE &&
> 		    ft->type != dest->ft->type)
> 			return false;
> 	}
>
>