Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) client-ip=23.128.96.34;
Message-ID: <9586be60-35f9-401f-b473-b16e86d13d07@kylinos.cn>
Date:   Thu, 14 Dec 2023 10:57:38 +0800
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] drivers/perf: Fix some null pointer dereference issues in
 thunderx2_pmu.c
Content-Language: en-US
To:     Will Deacon <will@kernel.org>
Cc:     mark.rutland@arm.com, Ganapatrao.Kulkarni@cavium.com,
        suzuki.poulose@arm.com, linux-arm-kernel@lists.infradead.org,
        linux-kernel@vger.kernel.org, Kunwu Chan <kunwu.chan@hotmail.com>
References: <20231211090347.265240-1-chentao@kylinos.cn>
 <20231212092523.GC28174@willie-the-truck>
From:   Kunwu Chan <chentao@kylinos.cn>
In-Reply-To: <20231212092523.GC28174@willie-the-truck>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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@hotmail.com>
>> Signed-off-by: Kunwu Chan <chentao@kylinos.cn>
>> ---
>>   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