| Message ID | 20231122090026.11728-1-chentao@kylinos.cn (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | powerpc/mm: Fix null-pointer dereference in pgtable_cache_add | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/github-powerpc_selftests | success | Successfully ran 8 jobs. |
| snowpatch_ozlabs/github-powerpc_ppctests | success | Successfully ran 8 jobs. |
| snowpatch_ozlabs/github-powerpc_sparse | success | Successfully ran 4 jobs. |
| snowpatch_ozlabs/github-powerpc_clang | success | Successfully ran 6 jobs. |
| snowpatch_ozlabs/github-powerpc_kernel_qemu | success | Successfully ran 23 jobs. |
Le 22/11/2023 à 10:00, Kunwu Chan a écrit : > [Vous ne recevez pas souvent de courriers de chentao@kylinos.cn. Découvrez pourquoi ceci est important à https://aka.ms/LearnAboutSenderIdentification ] > > kasprintf() returns a pointer to dynamically allocated memory > which can be NULL upon failure. Ensure the allocation was successful > by checking the pointer validity. Are you sure this is needed ? Did you check what happens what name is NULL ? If I followed stuff correctly, I end up in function sysfs_do_create_link_sd() which already handles the NULL name case which a big hammer warning. > > Signed-off-by: Kunwu Chan <chentao@kylinos.cn> > --- > arch/powerpc/mm/init-common.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/arch/powerpc/mm/init-common.c b/arch/powerpc/mm/init-common.c > index 119ef491f797..0884fc601c46 100644 > --- a/arch/powerpc/mm/init-common.c > +++ b/arch/powerpc/mm/init-common.c > @@ -139,6 +139,8 @@ void pgtable_cache_add(unsigned int shift) > > align = max_t(unsigned long, align, minalign); > name = kasprintf(GFP_KERNEL, "pgtable-2^%d", shift); > + if (!name) > + return; > new = kmem_cache_create(name, table_size, align, 0, ctor(shift)); > if (!new) > panic("Could not allocate pgtable cache for order %d", shift); > -- > 2.34.1 >
Hi Christophe, Thanks for your reply. It's my bad. According your reply, i read the code in sysfs_do_create_link_sd.There is a null pointer check indeed. My intention was to check null pointer after memory allocation. Whether we can add a comment here for someone like me, the null pointer check is no need here? Thanks, Kunwu On 2023/11/24 23:17, Christophe Leroy wrote: > > > Le 22/11/2023 à 10:00, Kunwu Chan a écrit : >> [Vous ne recevez pas souvent de courriers de chentao@kylinos.cn. Découvrez pourquoi ceci est important à https://aka.ms/LearnAboutSenderIdentification ] >> >> kasprintf() returns a pointer to dynamically allocated memory >> which can be NULL upon failure. Ensure the allocation was successful >> by checking the pointer validity. > > Are you sure this is needed ? Did you check what happens what name is NULL ? > > If I followed stuff correctly, I end up in function > sysfs_do_create_link_sd() which already handles the NULL name case which > a big hammer warning. > >> >> Signed-off-by: Kunwu Chan <chentao@kylinos.cn> >> --- >> arch/powerpc/mm/init-common.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/arch/powerpc/mm/init-common.c b/arch/powerpc/mm/init-common.c >> index 119ef491f797..0884fc601c46 100644 >> --- a/arch/powerpc/mm/init-common.c >> +++ b/arch/powerpc/mm/init-common.c >> @@ -139,6 +139,8 @@ void pgtable_cache_add(unsigned int shift) >> >> align = max_t(unsigned long, align, minalign); >> name = kasprintf(GFP_KERNEL, "pgtable-2^%d", shift); >> + if (!name) >> + return; >> new = kmem_cache_create(name, table_size, align, 0, ctor(shift)); >> if (!new) >> panic("Could not allocate pgtable cache for order %d", shift); >> -- >> 2.34.1 >>
Kunwu Chan <chentao@kylinos.cn> writes: > Hi Christophe, > > Thanks for your reply. > It's my bad. According your reply, i read the code in > sysfs_do_create_link_sd.There is a null pointer check indeed. > > My intention was to check null pointer after memory allocation. > Whether we can add a comment here for someone like me, the null pointer > check is no need here? I don't mind there being a NULL check for name. But the code shouldn't silently return if name can't be allocated. Notice that if we can't create the cache we *panic*. A failure to allocate name, which causes us to skip the cache creation, needs to also panic. cheers > On 2023/11/24 23:17, Christophe Leroy wrote: >> >> >> Le 22/11/2023 à 10:00, Kunwu Chan a écrit : >>> [Vous ne recevez pas souvent de courriers de chentao@kylinos.cn. Découvrez pourquoi ceci est important à https://aka.ms/LearnAboutSenderIdentification ] >>> >>> kasprintf() returns a pointer to dynamically allocated memory >>> which can be NULL upon failure. Ensure the allocation was successful >>> by checking the pointer validity. >> >> Are you sure this is needed ? Did you check what happens what name is NULL ? >> >> If I followed stuff correctly, I end up in function >> sysfs_do_create_link_sd() which already handles the NULL name case which >> a big hammer warning. >> >>> >>> Signed-off-by: Kunwu Chan <chentao@kylinos.cn> >>> --- >>> arch/powerpc/mm/init-common.c | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/arch/powerpc/mm/init-common.c b/arch/powerpc/mm/init-common.c >>> index 119ef491f797..0884fc601c46 100644 >>> --- a/arch/powerpc/mm/init-common.c >>> +++ b/arch/powerpc/mm/init-common.c >>> @@ -139,6 +139,8 @@ void pgtable_cache_add(unsigned int shift) >>> >>> align = max_t(unsigned long, align, minalign); >>> name = kasprintf(GFP_KERNEL, "pgtable-2^%d", shift); >>> + if (!name) >>> + return; >>> new = kmem_cache_create(name, table_size, align, 0, ctor(shift)); >>> if (!new) >>> panic("Could not allocate pgtable cache for order %d", shift); >>> -- >>> 2.34.1 >>>
Thanks for your reply. Ok, I know what you mean, when name is NULL. The process should be aborted and the specific reason for the error should be printed, not just return. I will update v2 patch with "panic". Thanks again, Kunwu On 2023/11/28 19:32, Michael Ellerman wrote: > Kunwu Chan <chentao@kylinos.cn> writes: >> Hi Christophe, >> >> Thanks for your reply. >> It's my bad. According your reply, i read the code in >> sysfs_do_create_link_sd.There is a null pointer check indeed. >> >> My intention was to check null pointer after memory allocation. >> Whether we can add a comment here for someone like me, the null pointer >> check is no need here? > > I don't mind there being a NULL check for name. > > But the code shouldn't silently return if name can't be allocated. > Notice that if we can't create the cache we *panic*. A failure to > allocate name, which causes us to skip the cache creation, needs to also > panic. > > cheers > >> On 2023/11/24 23:17, Christophe Leroy wrote: >>> >>> >>> Le 22/11/2023 à 10:00, Kunwu Chan a écrit : >>>> [Vous ne recevez pas souvent de courriers de chentao@kylinos.cn. Découvrez pourquoi ceci est important à https://aka.ms/LearnAboutSenderIdentification ] >>>> >>>> kasprintf() returns a pointer to dynamically allocated memory >>>> which can be NULL upon failure. Ensure the allocation was successful >>>> by checking the pointer validity. >>> >>> Are you sure this is needed ? Did you check what happens what name is NULL ? >>> >>> If I followed stuff correctly, I end up in function >>> sysfs_do_create_link_sd() which already handles the NULL name case which >>> a big hammer warning. >>> >>>> >>>> Signed-off-by: Kunwu Chan <chentao@kylinos.cn> >>>> --- >>>> arch/powerpc/mm/init-common.c | 2 ++ >>>> 1 file changed, 2 insertions(+) >>>> >>>> diff --git a/arch/powerpc/mm/init-common.c b/arch/powerpc/mm/init-common.c >>>> index 119ef491f797..0884fc601c46 100644 >>>> --- a/arch/powerpc/mm/init-common.c >>>> +++ b/arch/powerpc/mm/init-common.c >>>> @@ -139,6 +139,8 @@ void pgtable_cache_add(unsigned int shift) >>>> >>>> align = max_t(unsigned long, align, minalign); >>>> name = kasprintf(GFP_KERNEL, "pgtable-2^%d", shift); >>>> + if (!name) >>>> + return; >>>> new = kmem_cache_create(name, table_size, align, 0, ctor(shift)); >>>> if (!new) >>>> panic("Could not allocate pgtable cache for order %d", shift); >>>> -- >>>> 2.34.1 >>>>
diff --git a/arch/powerpc/mm/init-common.c b/arch/powerpc/mm/init-common.c index 119ef491f797..0884fc601c46 100644 --- a/arch/powerpc/mm/init-common.c +++ b/arch/powerpc/mm/init-common.c @@ -139,6 +139,8 @@ void pgtable_cache_add(unsigned int shift) align = max_t(unsigned long, align, minalign); name = kasprintf(GFP_KERNEL, "pgtable-2^%d", shift); + if (!name) + return; new = kmem_cache_create(name, table_size, align, 0, ctor(shift)); if (!new) panic("Could not allocate pgtable cache for order %d", shift);
kasprintf() returns a pointer to dynamically allocated memory which can be NULL upon failure. Ensure the allocation was successful by checking the pointer validity. Signed-off-by: Kunwu Chan <chentao@kylinos.cn> --- arch/powerpc/mm/init-common.c | 2 ++ 1 file changed, 2 insertions(+)