> -----Original Message----- > From: Daniil Dulov <D.Dulov@xxxxxxxxxx> > Sent: Monday, 4 March 2024 13:44 > To: Vadim Pasternak <vadimp@xxxxxxxxxx>; Hans de Goede > <hdegoede@xxxxxxxxxx> > Cc: Mark Gross <mgross@xxxxxxxxxxxxxxx>; Andy Shevchenko > <andy@xxxxxxxxxxxxx>; Darren Hart <dvhart@xxxxxxxxxxxxx>; platform- > driver-x86@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; lvc- > project@xxxxxxxxxxxxxxxx > Subject: RE: [PATCH] platform/mellanox: mlxreg-hotplug: Check pointer for > NULL before dereferencing it > > Hello! > > I suppose there is no sense to produce dev_err() inside > mlxreg_hotplug_work_helper() since item is dereferenced twice before we > call this function. Should we produce dev_err() inside the loop in > mlxreg_hotplug_work_handler() instead? Hi Daniil, I think would be correct just to remove from mlx reg_hotplug_work_helper() lines validating 'item' pointer. This is paranoic test, this pointer should never be NULL. It is safe to remove this validation. Thanks, Vadim. > > -----Original Message----- > From: Vadim Pasternak [mailto:vadimp@xxxxxxxxxx] > Sent: Monday, February 26, 2024 6:15 PM > To: Daniil Dulov <D.Dulov@xxxxxxxxxx>; Hans de Goede > <hdegoede@xxxxxxxxxx> > Cc: Mark Gross <mgross@xxxxxxxxxxxxxxx>; Andy Shevchenko > <andy@xxxxxxxxxxxxx>; Darren Hart <dvhart@xxxxxxxxxxxxx>; platform- > driver-x86@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; lvc- > project@xxxxxxxxxxxxxxxx > Subject: RE: [PATCH] platform/mellanox: mlxreg-hotplug: Check pointer for > NULL before dereferencing it > > > > > -----Original Message----- > > From: Daniil Dulov <d.dulov@xxxxxxxxxx> > > Sent: Monday, 26 February 2024 16:55 > > To: Hans de Goede <hdegoede@xxxxxxxxxx> > > Cc: Daniil Dulov <d.dulov@xxxxxxxxxx>; Mark Gross > > <mgross@xxxxxxxxxxxxxxx>; Andy Shevchenko <andy@xxxxxxxxxxxxx>; > Darren > > Hart <dvhart@xxxxxxxxxxxxx>; Vadim Pasternak <vadimp@xxxxxxxxxx>; > > platform-driver-x86@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; lvc- > > project@xxxxxxxxxxxxxxxx > > Subject: [PATCH] platform/mellanox: mlxreg-hotplug: Check pointer for NULL > > before dereferencing it > > > > mlxreg_hotplug_work_helper() implies that item can be NULL. There is a > > sanity check that checks item for NULL and then dereferences it. > > > > Even though, the comment before sanity check says that it can only happen > if > > some piece of hardware is broken, but in this case it will lead to NULL- > pointer > > dereference before the function is even called, so let's check it before > > dereferencing. > > > > Found by Linux Verification Center (linuxtesting.org) with SVACE. > > > > Fixes: c6acad68eb2d ("platform/mellanox: mlxreg-hotplug: Modify to use a > > regmap interface") > > Signed-off-by: Daniil Dulov <d.dulov@xxxxxxxxxx> > > --- > > drivers/platform/mellanox/mlxreg-hotplug.c | 16 +--------------- > > 1 file changed, 1 insertion(+), 15 deletions(-) > > > > diff --git a/drivers/platform/mellanox/mlxreg-hotplug.c > > b/drivers/platform/mellanox/mlxreg-hotplug.c > > index 5c022b258f91..524121b9f070 100644 > > --- a/drivers/platform/mellanox/mlxreg-hotplug.c > > +++ b/drivers/platform/mellanox/mlxreg-hotplug.c > > @@ -348,20 +348,6 @@ mlxreg_hotplug_work_helper(struct > > mlxreg_hotplug_priv_data *priv, > > u32 regval, bit; > > int ret; > > > > - /* > > - * Validate if item related to received signal type is valid. > > - * It should never happen, excepted the situation when some > > - * piece of hardware is broken. In such situation just produce > > - * error message and return. Caller must continue to handle the > > - * signals from other devices if any. > > - */ > > - if (unlikely(!item)) { > > - dev_err(priv->dev, "False signal: at offset:mask > > 0x%02x:0x%02x.\n", > > - item->reg, item->mask); > > - > > - return; > > - } > > It would be enough just to produce dev_err(priv->dev, "False signal\n"); > And return. > > > - > > /* Mask event. */ > > ret = regmap_write(priv->regmap, item->reg + > > MLXREG_HOTPLUG_MASK_OFF, > > 0); > > @@ -556,7 +542,7 @@ static void mlxreg_hotplug_work_handler(struct > > work_struct *work) > > > > /* Handle topology and health configuration changes. */ > > for (i = 0; i < pdata->counter; i++, item++) { > > - if (aggr_asserted & item->aggr_mask) { > > + if (item && (aggr_asserted & item->aggr_mask)) { > > if (item->health) > > mlxreg_hotplug_health_work_helper(priv, > > item); > > else > > -- > > 2.25.1
