| Message ID | 20250405100017.77498-1-bsdhenrymartin@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v1] net/mlx5: Fix null-ptr-deref in mlx5_create_inner_ttc_table() | expand |
> Add NULL check for mlx5_get_flow_namespace() returns in > mlx5_create_inner_ttc_table() to prevent NULL pointer dereference. * You would like to adjust the error handling in some function implementations from a common subdirectory. How do you think about to offer such changes in a corresponding patch series? * Can any other summary phrase variant become more desirable accordingly? * Would any blank lines become also desirable after added statements? Regards, Markus
On 4/5/25 12:00 PM, Henry Martin wrote: > Add NULL check for mlx5_get_flow_namespace() returns in > mlx5_create_inner_ttc_table() to prevent NULL pointer dereference. > > Fixes: 137f3d50ad2a ("net/mlx5: Support matching on l4_type for ttc_table") > Signed-off-by: Henry Martin <bsdhenrymartin@gmail.com> > --- > drivers/net/ethernet/mellanox/mlx5/core/lib/fs_ttc.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lib/fs_ttc.c b/drivers/net/ethernet/mellanox/mlx5/core/lib/fs_ttc.c > index eb3bd9c7f66e..4e964ca5367e 100644 > --- a/drivers/net/ethernet/mellanox/mlx5/core/lib/fs_ttc.c > +++ b/drivers/net/ethernet/mellanox/mlx5/core/lib/fs_ttc.c > @@ -655,6 +655,8 @@ struct mlx5_ttc_table *mlx5_create_inner_ttc_table(struct mlx5_core_dev *dev, > } > > ns = mlx5_get_flow_namespace(dev, params->ns_type); > + if (!ns) > + return ERR_PTR(-EOPNOTSUPP); I suspect the ns_type the caller always sets a valid 'ns_type', so the NULL ptr is not really possible here. At very least an empty line after the return statement will make the code more readable and the commit message should be rewritten to better describe the issue. Similar considerations apply to the other mlx5 fixes. Thanks, Paolo
… > > +++ b/drivers/net/ethernet/mellanox/mlx5/core/lib/fs_ttc.c > > @@ -655,6 +655,8 @@ struct mlx5_ttc_table *mlx5_create_inner_ttc_table(struct mlx5_core_dev *dev, > > } > > > > ns = mlx5_get_flow_namespace(dev, params->ns_type); > > + if (!ns) > > + return ERR_PTR(-EOPNOTSUPP); > > I suspect the ns_type the caller always sets a valid 'ns_type', so the > NULL ptr is not really possible here. Is there a need to mark such a check result as “unlikely”? Regards, Markus
On 08/04/2025 15:25, Markus Elfring wrote: > … >>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/lib/fs_ttc.c >>> @@ -655,6 +655,8 @@ struct mlx5_ttc_table *mlx5_create_inner_ttc_table(struct mlx5_core_dev *dev, >>> } >>> >>> ns = mlx5_get_flow_namespace(dev, params->ns_type); >>> + if (!ns) >>> + return ERR_PTR(-EOPNOTSUPP); >> >> I suspect the ns_type the caller always sets a valid 'ns_type', so the >> NULL ptr is not really possible here. > > Is there a need to mark such a check result as “unlikely”? > Please don't. I'm fine with simply adding the check, as Paolo suggested. When TTC was originally introduced, its functionality was more limited, and reaching this point in the driver meant we could be certain the namespace existed. Now that TTC has become more advanced, adding this check makes sense and I'm okay with it. Mark > Regards, > Markus >
Thank you for the review. This check will be kept, and I'll follow Paolo's suggestion about adding a blank line before the return statements in the v2. Mark Bloch <mbloch@nvidia.com> 于2025年4月8日周二 23:01写道： > > > > On 08/04/2025 15:25, Markus Elfring wrote: > > … > >>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/lib/fs_ttc.c > >>> @@ -655,6 +655,8 @@ struct mlx5_ttc_table *mlx5_create_inner_ttc_table(struct mlx5_core_dev *dev, > >>> } > >>> > >>> ns = mlx5_get_flow_namespace(dev, params->ns_type); > >>> + if (!ns) > >>> + return ERR_PTR(-EOPNOTSUPP); > >> > >> I suspect the ns_type the caller always sets a valid 'ns_type', so the > >> NULL ptr is not really possible here. > > > > Is there a need to mark such a check result as “unlikely”? > > > > Please don't. I'm fine with simply adding the check, as > Paolo suggested. When TTC was originally introduced, its > functionality was more limited, and reaching this point in the driver > meant we could be certain the namespace existed. Now that TTC has > become more advanced, adding this check makes sense and I'm okay with > it. > > Mark > > > Regards, > > Markus > > >
> Thank you for the review. This check will be kept, and I'll follow Paolo's > suggestion about adding a blank line before the return statements in the v2. after? Regards, Markus
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lib/fs_ttc.c b/drivers/net/ethernet/mellanox/mlx5/core/lib/fs_ttc.c index eb3bd9c7f66e..4e964ca5367e 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/lib/fs_ttc.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/lib/fs_ttc.c @@ -655,6 +655,8 @@ struct mlx5_ttc_table *mlx5_create_inner_ttc_table(struct mlx5_core_dev *dev, } ns = mlx5_get_flow_namespace(dev, params->ns_type); + if (!ns) + return ERR_PTR(-EOPNOTSUPP); groups = use_l4_type ? &inner_ttc_groups[TTC_GROUPS_USE_L4_TYPE] : &inner_ttc_groups[TTC_GROUPS_DEFAULT];
Add NULL check for mlx5_get_flow_namespace() returns in mlx5_create_inner_ttc_table() to prevent NULL pointer dereference. Fixes: 137f3d50ad2a ("net/mlx5: Support matching on l4_type for ttc_table") Signed-off-by: Henry Martin <bsdhenrymartin@gmail.com> --- drivers/net/ethernet/mellanox/mlx5/core/lib/fs_ttc.c | 2 ++ 1 file changed, 2 insertions(+)