On Tue, Sep 05, 2023 at 07:27:47AM +0200, Marion & Christophe JAILLET wrote:
>
> Some debugfs dir of file way be left around. Is it what your are talking
> about?
Yes all allocated resources should be freed on the error path.
> > The other snprintf in the same file also looks suspect.
>
> It looks correct to me.
>
> And HPRE_DBGFS_VAL_MAX_LEN being 20, it doesn't really matter. The string
> can't be truncated with just a "%u\n".
Well if you're going to go with that line of reasoning then this
case ("cluster%d") can't overflow either, no?
Cheers,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
On 2023/9/5 16:17, Herbert Xu wrote:
> On Tue, Sep 05, 2023 at 07:27:47AM +0200, Marion & Christophe JAILLET wrote:
>>
>> Some debugfs dir of file way be left around. Is it what your are talking
>> about?
>
> Yes all allocated resources should be freed on the error path.
>
>>> The other snprintf in the same file also looks suspect.
>>
>> It looks correct to me.
>>
>> And HPRE_DBGFS_VAL_MAX_LEN being 20, it doesn't really matter. The string
>> can't be truncated with just a "%u\n".
>
> Well if you're going to go with that line of reasoning then this
> case ("cluster%d") can't overflow either, no?
>
First, I checked the calling code of the snprintf function in all driver files in
the hisilicon directory. Only here is the processing of return value judgment.
This treatment is indeed problematic and needs to be modified.
Then, I don't quite agree with your modification plan.
The modification of this solution is not complete.
As Herbert said, ("cluster%d") may still have overflow problems.
In the end, my proposed modification scheme is this:
...
int ret;
u8 i;
for (i = 0; i < clusters_num; i++) {
snprintf(buf, HPRE_DBGFS_VAL_MAX_LEN, "cluster%u", i);
tmp_d = debugfs_create_dir(buf, qm->debug.debug_root);
...
}
...
Thanks,
Longfang.
> Cheers,
>