* [PATCH RFC net] net/mlx5: Avoid NULL dereference in dest_is_valid
@ 2025-06-18 18:27 Simon Horman
2025-06-18 19:35 ` Mark Bloch
2025-06-18 19:35 ` Saeed Mahameed
0 siblings, 2 replies; 4+ messages in thread
From: Simon Horman @ 2025-06-18 18:27 UTC (permalink / raw)
To: Saeed Mahameed, Leon Romanovsky, Tariq Toukan
Cc: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Mark Bloch, Paul Blakey, netdev, linux-rdma,
Simon Horman
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.
---
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->type == MLX5_FLOW_DESTINATION_TYPE_FLOW_TABLE &&
ft->type != dest->ft->type)
return false;
}
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH RFC net] net/mlx5: Avoid NULL dereference in dest_is_valid
2025-06-18 18:27 [PATCH RFC net] net/mlx5: Avoid NULL dereference in dest_is_valid Simon Horman
@ 2025-06-18 19:35 ` Mark Bloch
2025-06-18 19:49 ` Simon Horman
2025-06-18 19:35 ` Saeed Mahameed
1 sibling, 1 reply; 4+ messages in thread
From: Mark Bloch @ 2025-06-18 19:35 UTC (permalink / raw)
To: Simon Horman, Saeed Mahameed, Leon Romanovsky, Tariq Toukan
Cc: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Mark Bloch, Paul Blakey, netdev, linux-rdma
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!
> ---
> 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->type == MLX5_FLOW_DESTINATION_TYPE_FLOW_TABLE &&
> ft->type != dest->ft->type)
> return false;
> }
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH RFC net] net/mlx5: Avoid NULL dereference in dest_is_valid
2025-06-18 19:35 ` Mark Bloch
@ 2025-06-18 19:49 ` Simon Horman
0 siblings, 0 replies; 4+ messages in thread
From: Simon Horman @ 2025-06-18 19:49 UTC (permalink / raw)
To: Mark Bloch, Saeed Mahameed
Cc: Saeed Mahameed, Leon Romanovsky, Tariq Toukan, Andrew Lunn,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Mark Bloch, Paul Blakey, netdev, linux-rdma
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.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH RFC net] net/mlx5: Avoid NULL dereference in dest_is_valid
2025-06-18 18:27 [PATCH RFC net] net/mlx5: Avoid NULL dereference in dest_is_valid Simon Horman
2025-06-18 19:35 ` Mark Bloch
@ 2025-06-18 19:35 ` Saeed Mahameed
1 sibling, 0 replies; 4+ messages in thread
From: Saeed Mahameed @ 2025-06-18 19:35 UTC (permalink / raw)
To: Simon Horman
Cc: Saeed Mahameed, Leon Romanovsky, Tariq Toukan, Andrew Lunn,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Mark Bloch, Paul Blakey, netdev, linux-rdma
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;
> }
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2025-06-18 19:50 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-06-18 18:27 [PATCH RFC net] net/mlx5: Avoid NULL dereference in dest_is_valid Simon Horman
2025-06-18 19:35 ` Mark Bloch
2025-06-18 19:49 ` Simon Horman
2025-06-18 19:35 ` Saeed Mahameed
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).