Re: [PATCH] drivers/perf: Fix some null pointer dereference issues in thunderx2_pmu.c

[Kunwu Chan]

Thanks for your reply.

After read tx2_uncore_pmu_register again.
From the defination: 'char *name = tx2_pmu->name;',
we could know 'tx2_pmu->pmu.name' equals 'tx2_pmu->name'

The difference is that a new memory space is allocated for 
'tx2_pmu->pmu.name'.

If 'tx2_pmu->pmu.name' is always the same as 'tx2_pmu->name', whether we 
should use 'tx2_pmu->pmu.name =  tx2_pmu->name;'
to replace the 'devm_kasprintf'.

I'm not sure it's appropriate to do that.

Thanks again.


On 2023/12/12 17:25, Will Deacon wrote:
> On Mon, Dec 11, 2023 at 05:03:47PM +0800, Kunwu Chan wrote:
> > devm_kasprintf() returns a pointer to dynamically allocated memory
> > which can be NULL upon failure.
> >
> > Fixes: 69c32972d593 ("drivers/perf: Add Cavium ThunderX2 SoC UNCORE PMU driver")
> > Cc: Kunwu Chan <kunwu.chan@xxxxxxxxxxx>
> > Signed-off-by: Kunwu Chan <chentao@xxxxxxxxxx>
> > ---
> >   drivers/perf/thunderx2_pmu.c | 7 +++++++
> >   1 file changed, 7 insertions(+)
> >
> > diff --git a/drivers/perf/thunderx2_pmu.c b/drivers/perf/thunderx2_pmu.c
> > index 1edb9c03704f..07edb174a0d7 100644
> > --- a/drivers/perf/thunderx2_pmu.c
> > +++ b/drivers/perf/thunderx2_pmu.c
> > @@ -742,6 +742,8 @@ static int tx2_uncore_pmu_register(
> >   
> >   	tx2_pmu->pmu.name = devm_kasprintf(dev, GFP_KERNEL,
> >   			"%s", name);
> > +	if (!tx2_pmu->pmu.name)
> > +		return -ENOMEM;
> >   
> >   	return perf_pmu_register(&tx2_pmu->pmu, tx2_pmu->pmu.name, -1);
>
> AFAICT, perf_pmu_register() will WARN and return NULL, so I'm not sure what
> we gain from the additional check.
>
> >   }
> > @@ -881,6 +883,11 @@ static struct tx2_uncore_pmu *tx2_uncore_pmu_init_dev(struct device *dev,
> >   		return NULL;
> >   	}
> >   
> > +	if (!tx2_pmu->name) {
> > +		dev_err(dev, "PMU type %d: Fail to allocate memory\n", type);
> > +		devm_kfree(dev, tx2_pmu);
> > +		return NULL;
> > +	}
>
> In the _highly_ unlikely even that devm_kasprintf() failed to allocate,
> shouldn't we get a splat from the allocator? I don't think it's useful
> to print another message.
>
> Will