| Message ID | 20240409190820.227554-13-tariqt@nvidia.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | mlx5 misc fixes | expand |
On Tue, Apr 09, 2024 at 10:08:20PM +0300, Tariq Toukan wrote: > Check if devcom holds an error pointer and return immediately. > > This fixes Smatch static checker warning: > drivers/net/ethernet/mellanox/mlx5/core/lib/sd.c:221 sd_register() > error: 'devcom' dereferencing possible ERR_PTR() > > Fixes: d3d057666090 ("net/mlx5: SD, Implement devcom communication and primary election") > Reported-by: Dan Carpenter <dan.carpenter@linaro.org> > Link: https://lore.kernel.org/all/f09666c8-e604-41f6-958b-4cc55c73faf9@gmail.com/T/ > Signed-off-by: Tariq Toukan <tariqt@nvidia.com> > Reviewed-by: Gal Pressman <gal@nvidia.com> > --- > drivers/net/ethernet/mellanox/mlx5/core/lib/sd.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lib/sd.c b/drivers/net/ethernet/mellanox/mlx5/core/lib/sd.c > index 5b28084e8a03..adbafed44ce7 100644 > --- a/drivers/net/ethernet/mellanox/mlx5/core/lib/sd.c > +++ b/drivers/net/ethernet/mellanox/mlx5/core/lib/sd.c > @@ -213,8 +213,8 @@ static int sd_register(struct mlx5_core_dev *dev) > sd = mlx5_get_sd(dev); > devcom = mlx5_devcom_register_component(dev->priv.devc, MLX5_DEVCOM_SD_GROUP, > sd->group_id, NULL, dev); > - if (!devcom) > - return -ENOMEM; > + if (IS_ERR_OR_NULL(devcom)) > + return devcom ? PTR_ERR(devcom) : -ENOMEM; Why not just change mlx5_devcom_register_component() to return ERR_PTR(-EINVAL); instead of NULL? Then the callers could just do: if (IS_ERR(devcom)) return PTR_ERR(devcom); We only have a sample size of 4 callers but doing it in this non-standard way seems to introduce bugs in 25% of the callers. regards, dan carpenter
On 10/04/2024 16:24, Dan Carpenter wrote: > On Tue, Apr 09, 2024 at 10:08:20PM +0300, Tariq Toukan wrote: >> Check if devcom holds an error pointer and return immediately. >> >> This fixes Smatch static checker warning: >> drivers/net/ethernet/mellanox/mlx5/core/lib/sd.c:221 sd_register() >> error: 'devcom' dereferencing possible ERR_PTR() >> >> Fixes: d3d057666090 ("net/mlx5: SD, Implement devcom communication and primary election") >> Reported-by: Dan Carpenter <dan.carpenter@linaro.org> >> Link: https://lore.kernel.org/all/f09666c8-e604-41f6-958b-4cc55c73faf9@gmail.com/T/ >> Signed-off-by: Tariq Toukan <tariqt@nvidia.com> >> Reviewed-by: Gal Pressman <gal@nvidia.com> >> --- >> drivers/net/ethernet/mellanox/mlx5/core/lib/sd.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lib/sd.c b/drivers/net/ethernet/mellanox/mlx5/core/lib/sd.c >> index 5b28084e8a03..adbafed44ce7 100644 >> --- a/drivers/net/ethernet/mellanox/mlx5/core/lib/sd.c >> +++ b/drivers/net/ethernet/mellanox/mlx5/core/lib/sd.c >> @@ -213,8 +213,8 @@ static int sd_register(struct mlx5_core_dev *dev) >> sd = mlx5_get_sd(dev); >> devcom = mlx5_devcom_register_component(dev->priv.devc, MLX5_DEVCOM_SD_GROUP, >> sd->group_id, NULL, dev); >> - if (!devcom) >> - return -ENOMEM; >> + if (IS_ERR_OR_NULL(devcom)) >> + return devcom ? PTR_ERR(devcom) : -ENOMEM; > > Why not just change mlx5_devcom_register_component() to return > ERR_PTR(-EINVAL); instead of NULL? Then the callers could just do: > > if (IS_ERR(devcom)) > return PTR_ERR(devcom); > > We only have a sample size of 4 callers but doing it in this > non-standard way seems to introduce bugs in 25% of the callers. > > regards, > dan carpenter > > Hi Dan, Touching mlx5_devcom_register_component() as a fix for commit d3d057666090 ("net/mlx5: SD, Implement devcom communication and primary election") doesn't look right to me. In addition, I prefer minimal fixes to net. After this one is accepted to net, we can enhance the code in a followup patch to net-next. Regards, Tariq
On Wed, 10 Apr 2024 21:16:37 +0300 Tariq Toukan wrote: > Touching mlx5_devcom_register_component() as a fix for commit > d3d057666090 ("net/mlx5: SD, Implement devcom communication and primary > election") doesn't look right to me. > In addition, I prefer minimal fixes to net. > > After this one is accepted to net, we can enhance the code in a followup > patch to net-next. I think the official guidance from Greg is "fix it right, worry about size of the change when stable backport fails". mlx5_devcom_register_component() has 4 callers, please follow Dan's advice. I'll apply other 11 patches.
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lib/sd.c b/drivers/net/ethernet/mellanox/mlx5/core/lib/sd.c index 5b28084e8a03..adbafed44ce7 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/lib/sd.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/lib/sd.c @@ -213,8 +213,8 @@ static int sd_register(struct mlx5_core_dev *dev) sd = mlx5_get_sd(dev); devcom = mlx5_devcom_register_component(dev->priv.devc, MLX5_DEVCOM_SD_GROUP, sd->group_id, NULL, dev); - if (!devcom) - return -ENOMEM; + if (IS_ERR_OR_NULL(devcom)) + return devcom ? PTR_ERR(devcom) : -ENOMEM; sd->devcom = devcom;
Check if devcom holds an error pointer and return immediately. This fixes Smatch static checker warning: drivers/net/ethernet/mellanox/mlx5/core/lib/sd.c:221 sd_register() error: 'devcom' dereferencing possible ERR_PTR() Fixes: d3d057666090 ("net/mlx5: SD, Implement devcom communication and primary election") Reported-by: Dan Carpenter <dan.carpenter@linaro.org> Link: https://lore.kernel.org/all/f09666c8-e604-41f6-958b-4cc55c73faf9@gmail.com/T/ Signed-off-by: Tariq Toukan <tariqt@nvidia.com> Reviewed-by: Gal Pressman <gal@nvidia.com> --- drivers/net/ethernet/mellanox/mlx5/core/lib/sd.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)