| Message ID | 20231127063433.1549064-1-chentao@kylinos.cn (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | Ilpo Järvinen |
| Headers | show |
| Series | platform/mellanox: Add a null pointer check in mlxbf_pmc_create_groups | expand |
Hi, On 11/27/23 07:34, Kunwu Chan wrote: > devm_kasprintf() returns a pointer to dynamically allocated memory > which can be NULL upon failure. > > Fixes: 1a218d312e65 ("platform/mellanox: mlxbf-pmc: Add Mellanox BlueField PMC driver") > Signed-off-by: Kunwu Chan <chentao@kylinos.cn> Thanks, patch looks good to me: Reviewed-by: Hans de Goede <hdegoede@redhat.com> Regards, Hans > --- > drivers/platform/mellanox/mlxbf-pmc.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/platform/mellanox/mlxbf-pmc.c b/drivers/platform/mellanox/mlxbf-pmc.c > index 0b427fc24a96..59bbe5e13f6b 100644 > --- a/drivers/platform/mellanox/mlxbf-pmc.c > +++ b/drivers/platform/mellanox/mlxbf-pmc.c > @@ -1882,6 +1882,8 @@ static int mlxbf_pmc_create_groups(struct device *dev, int blk_num) > pmc->block[blk_num].block_attr_grp.attrs = pmc->block[blk_num].block_attr; > pmc->block[blk_num].block_attr_grp.name = devm_kasprintf( > dev, GFP_KERNEL, pmc->block_name[blk_num]); > + if (!pmc->block[blk_num].block_attr_grp.name) > + return -ENOMEM; > pmc->groups[pmc->group_num] = &pmc->block[blk_num].block_attr_grp; > pmc->group_num++; >
On Mon, 27 Nov 2023, Kunwu Chan wrote: > devm_kasprintf() returns a pointer to dynamically allocated memory > which can be NULL upon failure. > > Fixes: 1a218d312e65 ("platform/mellanox: mlxbf-pmc: Add Mellanox BlueField PMC driver") > Signed-off-by: Kunwu Chan <chentao@kylinos.cn> > --- > drivers/platform/mellanox/mlxbf-pmc.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/platform/mellanox/mlxbf-pmc.c b/drivers/platform/mellanox/mlxbf-pmc.c > index 0b427fc24a96..59bbe5e13f6b 100644 > --- a/drivers/platform/mellanox/mlxbf-pmc.c > +++ b/drivers/platform/mellanox/mlxbf-pmc.c > @@ -1882,6 +1882,8 @@ static int mlxbf_pmc_create_groups(struct device *dev, int blk_num) > pmc->block[blk_num].block_attr_grp.attrs = pmc->block[blk_num].block_attr; > pmc->block[blk_num].block_attr_grp.name = devm_kasprintf( > dev, GFP_KERNEL, pmc->block_name[blk_num]); > + if (!pmc->block[blk_num].block_attr_grp.name) > + return -ENOMEM; > pmc->groups[pmc->group_num] = &pmc->block[blk_num].block_attr_grp; > pmc->group_num++; I'm totally lost, why did you fix only one devm_kasprintf() location? Don't all of them need this check?
Thanks for your reply. Cause i don't know how to deal with in some scenario，such as in 'mlxbf_pmc_init_perftype_counter', when 'attr->dev_attr.attr.name' is null, should return '-ENOMEM' or 'continue' the loop? So I'm going to solve it one by one. Thanks again, Kunwu On 2023/11/28 17:51, Ilpo Järvinen wrote: > On Mon, 27 Nov 2023, Kunwu Chan wrote: > >> devm_kasprintf() returns a pointer to dynamically allocated memory >> which can be NULL upon failure. >> >> Fixes: 1a218d312e65 ("platform/mellanox: mlxbf-pmc: Add Mellanox BlueField PMC driver") >> Signed-off-by: Kunwu Chan <chentao@kylinos.cn> >> --- >> drivers/platform/mellanox/mlxbf-pmc.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/drivers/platform/mellanox/mlxbf-pmc.c b/drivers/platform/mellanox/mlxbf-pmc.c >> index 0b427fc24a96..59bbe5e13f6b 100644 >> --- a/drivers/platform/mellanox/mlxbf-pmc.c >> +++ b/drivers/platform/mellanox/mlxbf-pmc.c >> @@ -1882,6 +1882,8 @@ static int mlxbf_pmc_create_groups(struct device *dev, int blk_num) >> pmc->block[blk_num].block_attr_grp.attrs = pmc->block[blk_num].block_attr; >> pmc->block[blk_num].block_attr_grp.name = devm_kasprintf( >> dev, GFP_KERNEL, pmc->block_name[blk_num]); >> + if (!pmc->block[blk_num].block_attr_grp.name) >> + return -ENOMEM; >> pmc->groups[pmc->group_num] = &pmc->block[blk_num].block_attr_grp; >> pmc->group_num++; > > I'm totally lost, why did you fix only one devm_kasprintf() location? > Don't all of them need this check? >
Hi Vadim, Could you please take a look at this and give advice to Kunwu so we can get all of them squashed in one go. On Thu, 30 Nov 2023, Kunwu Chan wrote: > Thanks for your reply. > > Cause i don't know how to deal with in some scenario，such as in > 'mlxbf_pmc_init_perftype_counter', when 'attr->dev_attr.attr.name' is null, > should return '-ENOMEM' or 'continue' the loop? I'd have thought returning -ENOMEM would be safe because it just ends up failing probe()? ...And it's not that likely to occur in the first place.
Hi Ilpo! > -----Original Message----- > From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > Sent: Thursday, 30 November 2023 17:26 > To: Kunwu Chan <chentao@kylinos.cn>; Vadim Pasternak > <vadimp@nvidia.com> > Cc: Hans de Goede <hdegoede@redhat.com>; jiri@resnulli.us; Shravan > Ramani <shravankr@nvidia.com>; kunwu.chan@hotmail.com; platform- > driver-x86@vger.kernel.org; LKML <linux-kernel@vger.kernel.org> > Subject: Re: [PATCH] platform/mellanox: Add a null pointer check in > mlxbf_pmc_create_groups > > Hi Vadim, > > Could you please take a look at this and give advice to Kunwu so we can get all > of them squashed in one go. It seems there are six calls devm_kasprintf(), which requires checking pointer. I guess, it is correct to return '-ENOMEM' for any failure. I see there is another problem in mlxbf_pmc_probe() - it lacks error flow for: pmc->hwmon_dev = devm_hwmon_device_register_with_groups( dev, "bfperf", pmc, pmc->groups); Need to add: if (IS_ERR(pmc->hwmon_dev)) return PTR_ERR(pmc->hwmon_dev); Sharvan, David, Could you, please, have look? > > On Thu, 30 Nov 2023, Kunwu Chan wrote: > > > Thanks for your reply. > > > > Cause i don't know how to deal with in some scenario，such as in > > 'mlxbf_pmc_init_perftype_counter', when 'attr->dev_attr.attr.name' is > > null, should return '-ENOMEM' or 'continue' the loop? > > I'd have thought returning -ENOMEM would be safe because it just ends up > failing probe()? ...And it's not that likely to occur in the first place. > > -- > i. > > > > > So I'm going to solve it one by one. > > > > Thanks again, > > Kunwu > > > > On 2023/11/28 17:51, Ilpo Järvinen wrote: > > > On Mon, 27 Nov 2023, Kunwu Chan wrote: > > > > > > > devm_kasprintf() returns a pointer to dynamically allocated memory > > > > which can be NULL upon failure. > > > > > > > > Fixes: 1a218d312e65 ("platform/mellanox: mlxbf-pmc: Add Mellanox > > > > BlueField PMC driver") > > > > Signed-off-by: Kunwu Chan <chentao@kylinos.cn> > > > > --- > > > > drivers/platform/mellanox/mlxbf-pmc.c | 2 ++ > > > > 1 file changed, 2 insertions(+) > > > > > > > > diff --git a/drivers/platform/mellanox/mlxbf-pmc.c > > > > b/drivers/platform/mellanox/mlxbf-pmc.c > > > > index 0b427fc24a96..59bbe5e13f6b 100644 > > > > --- a/drivers/platform/mellanox/mlxbf-pmc.c > > > > +++ b/drivers/platform/mellanox/mlxbf-pmc.c > > > > @@ -1882,6 +1882,8 @@ static int mlxbf_pmc_create_groups(struct > > > > device *dev, int blk_num) > > > > pmc->block[blk_num].block_attr_grp.attrs = > > > > pmc->block[blk_num].block_attr; > > > > pmc->block[blk_num].block_attr_grp.name = devm_kasprintf( > > > > dev, GFP_KERNEL, pmc->block_name[blk_num]); > > > > + if (!pmc->block[blk_num].block_attr_grp.name) > > > > + return -ENOMEM; > > > > pmc->groups[pmc->group_num] = &pmc- > >block[blk_num].block_attr_grp; > > > > pmc->group_num++; > > > > > > I'm totally lost, why did you fix only one devm_kasprintf() location? > > > Don't all of them need this check? > > > > >
Hi Vadim, Thanks for your reply. I will follw your suggestions and add some ‘fixes’ label in v2 patch. Thanks again, Kunwu On 2023/12/1 00:01, Vadim Pasternak wrote: > Hi Ilpo! > >> -----Original Message----- >> From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> >> Sent: Thursday, 30 November 2023 17:26 >> To: Kunwu Chan <chentao@kylinos.cn>; Vadim Pasternak >> <vadimp@nvidia.com> >> Cc: Hans de Goede <hdegoede@redhat.com>; jiri@resnulli.us; Shravan >> Ramani <shravankr@nvidia.com>; kunwu.chan@hotmail.com; platform- >> driver-x86@vger.kernel.org; LKML <linux-kernel@vger.kernel.org> >> Subject: Re: [PATCH] platform/mellanox: Add a null pointer check in >> mlxbf_pmc_create_groups >> >> Hi Vadim, >> >> Could you please take a look at this and give advice to Kunwu so we can get all >> of them squashed in one go. > > It seems there are six calls devm_kasprintf(), which requires checking pointer. > > I guess, it is correct to return '-ENOMEM' for any failure. > > I see there is another problem in mlxbf_pmc_probe() - it lacks error flow for: > pmc->hwmon_dev = devm_hwmon_device_register_with_groups( > dev, "bfperf", pmc, pmc->groups); > > Need to add: > if (IS_ERR(pmc->hwmon_dev)) > return PTR_ERR(pmc->hwmon_dev); > > Sharvan, David, > Could you, please, have look? > >> >> On Thu, 30 Nov 2023, Kunwu Chan wrote: >> >>> Thanks for your reply. >>> >>> Cause i don't know how to deal with in some scenario，such as in >>> 'mlxbf_pmc_init_perftype_counter', when 'attr->dev_attr.attr.name' is >>> null, should return '-ENOMEM' or 'continue' the loop? >> >> I'd have thought returning -ENOMEM would be safe because it just ends up >> failing probe()? ...And it's not that likely to occur in the first place. >> >> -- >> i. >> >>> >>> So I'm going to solve it one by one. >>> >>> Thanks again, >>> Kunwu >>> >>> On 2023/11/28 17:51, Ilpo Järvinen wrote: >>>> On Mon, 27 Nov 2023, Kunwu Chan wrote: >>>> >>>>> devm_kasprintf() returns a pointer to dynamically allocated memory >>>>> which can be NULL upon failure. >>>>> >>>>> Fixes: 1a218d312e65 ("platform/mellanox: mlxbf-pmc: Add Mellanox >>>>> BlueField PMC driver") >>>>> Signed-off-by: Kunwu Chan <chentao@kylinos.cn> >>>>> --- >>>>> drivers/platform/mellanox/mlxbf-pmc.c | 2 ++ >>>>> 1 file changed, 2 insertions(+) >>>>> >>>>> diff --git a/drivers/platform/mellanox/mlxbf-pmc.c >>>>> b/drivers/platform/mellanox/mlxbf-pmc.c >>>>> index 0b427fc24a96..59bbe5e13f6b 100644 >>>>> --- a/drivers/platform/mellanox/mlxbf-pmc.c >>>>> +++ b/drivers/platform/mellanox/mlxbf-pmc.c >>>>> @@ -1882,6 +1882,8 @@ static int mlxbf_pmc_create_groups(struct >>>>> device *dev, int blk_num) >>>>> pmc->block[blk_num].block_attr_grp.attrs = >>>>> pmc->block[blk_num].block_attr; >>>>> pmc->block[blk_num].block_attr_grp.name = devm_kasprintf( >>>>> dev, GFP_KERNEL, pmc->block_name[blk_num]); >>>>> + if (!pmc->block[blk_num].block_attr_grp.name) >>>>> + return -ENOMEM; >>>>> pmc->groups[pmc->group_num] = &pmc- >>> block[blk_num].block_attr_grp; >>>>> pmc->group_num++; >>>> >>>> I'm totally lost, why did you fix only one devm_kasprintf() location? >>>> Don't all of them need this check? >>>> >>>
diff --git a/drivers/platform/mellanox/mlxbf-pmc.c b/drivers/platform/mellanox/mlxbf-pmc.c index 0b427fc24a96..59bbe5e13f6b 100644 --- a/drivers/platform/mellanox/mlxbf-pmc.c +++ b/drivers/platform/mellanox/mlxbf-pmc.c @@ -1882,6 +1882,8 @@ static int mlxbf_pmc_create_groups(struct device *dev, int blk_num) pmc->block[blk_num].block_attr_grp.attrs = pmc->block[blk_num].block_attr; pmc->block[blk_num].block_attr_grp.name = devm_kasprintf( dev, GFP_KERNEL, pmc->block_name[blk_num]); + if (!pmc->block[blk_num].block_attr_grp.name) + return -ENOMEM; pmc->groups[pmc->group_num] = &pmc->block[blk_num].block_attr_grp; pmc->group_num++;
devm_kasprintf() returns a pointer to dynamically allocated memory which can be NULL upon failure. Fixes: 1a218d312e65 ("platform/mellanox: mlxbf-pmc: Add Mellanox BlueField PMC driver") Signed-off-by: Kunwu Chan <chentao@kylinos.cn> --- drivers/platform/mellanox/mlxbf-pmc.c | 2 ++ 1 file changed, 2 insertions(+)