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? -----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
