[PATCH] parisc: Increase the usage check of kmalloc allocated object a

[PATCH] parisc: Increase the usage check of kmalloc allocated object a

[Li zeming]

In the case of memory allocation failure, no alignment operation is
required.

Signed-off-by: Li zeming <zeming@nfschina.com>
---
 drivers/parisc/iosapic.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/parisc/iosapic.c b/drivers/parisc/iosapic.c
index 3a8c98615634..33de438916d3 100644
--- a/drivers/parisc/iosapic.c
+++ b/drivers/parisc/iosapic.c
@@ -229,7 +229,9 @@ static struct irt_entry *iosapic_alloc_irt(int num_entries)
 	 * 4-byte alignment on 32-bit kernels
 	 */
 	a = (unsigned long)kmalloc(sizeof(struct irt_entry) * num_entries + 8, GFP_KERNEL);
-	a = (a + 7UL) & ~7UL;
+	if (a)
+		a = (a + 7UL) & ~7UL;
+
 	return (struct irt_entry *)a;
 }
 
-- 
2.18.2

Re: [PATCH] parisc: Increase the usage check of kmalloc allocated object a

[Helge Deller]

On 9/14/22 08:04, Li zeming wrote:
> In the case of memory allocation failure, no alignment operation is
> required.
>
> Signed-off-by: Li zeming <zeming@nfschina.com>
> ---
>   drivers/parisc/iosapic.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/parisc/iosapic.c b/drivers/parisc/iosapic.c
> index 3a8c98615634..33de438916d3 100644
> --- a/drivers/parisc/iosapic.c
> +++ b/drivers/parisc/iosapic.c
> @@ -229,7 +229,9 @@ static struct irt_entry *iosapic_alloc_irt(int num_entries)
>   	 * 4-byte alignment on 32-bit kernels
>   	 */
>   	a = (unsigned long)kmalloc(sizeof(struct irt_entry) * num_entries + 8, GFP_KERNEL);
> -	a = (a + 7UL) & ~7UL;
> +	if (a)
> +		a = (a + 7UL) & ~7UL;
> +

As you said, the adjustment isn't required, but it's still ok.
So I think the additional "if" isn't necessary and so I'm not
applying your patch.

Anyway, thanks for your help to try to improve the code!

Helge


>   	return (struct irt_entry *)a;
>   }
>

Re: [PATCH] parisc: Increase the usage check of kmalloc allocated object a

[Rolf Eike Beer]

[-- Attachment #1: Type: text/plain, Size: 1722 bytes --]

Am Mittwoch, 14. September 2022, 08:18:19 CEST schrieb Helge Deller:
> On 9/14/22 08:04, Li zeming wrote:
> > In the case of memory allocation failure, no alignment operation is
> > required.
> > 
> > Signed-off-by: Li zeming <zeming@nfschina.com>
> > ---
> > 
> >   drivers/parisc/iosapic.c | 4 +++-
> >   1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/parisc/iosapic.c b/drivers/parisc/iosapic.c
> > index 3a8c98615634..33de438916d3 100644
> > --- a/drivers/parisc/iosapic.c
> > +++ b/drivers/parisc/iosapic.c
> > @@ -229,7 +229,9 @@ static struct irt_entry *iosapic_alloc_irt(int
> > num_entries)> 
> >   	 * 4-byte alignment on 32-bit kernels
> >   	 */
> >   	
> >   	a = (unsigned long)kmalloc(sizeof(struct irt_entry) * num_entries 
+ 8,
> >   	GFP_KERNEL);> 
> > -	a = (a + 7UL) & ~7UL;
> > +	if (a)
> > +		a = (a + 7UL) & ~7UL;
> > +
> 
> As you said, the adjustment isn't required, but it's still ok.
> So I think the additional "if" isn't necessary and so I'm not
> applying your patch.
> 
> Anyway, thanks for your help to try to improve the code!

I was about to say the same, but from looking at the code I don't think what 
is in there is correct either. The comment seems outdated, because 
__assume_kmalloc_alignment, which is __alignof__(unsigned long long). This 
code is untouched for the entire git history, so maybe we can just change the 
whole thing to

  return kcalloc(num_entries, sizeof(struct irt_entry))

now?

And these functions end up propagating an allocation error in this file and it 
will never reach kernel/setup.c, which seems bad. But I guess the only point 
where this really can go wrong if the PDC returns an absurdly large number of 
entries.

Eike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH] parisc: Increase the usage check of kmalloc allocated object a

[Helge Deller]

On 9/14/22 08:43, Rolf Eike Beer wrote:
> Am Mittwoch, 14. September 2022, 08:18:19 CEST schrieb Helge Deller:
>> On 9/14/22 08:04, Li zeming wrote:
>>> In the case of memory allocation failure, no alignment operation is
>>> required.
>>>
>>> Signed-off-by: Li zeming <zeming@nfschina.com>
>>> ---
>>>
>>>    drivers/parisc/iosapic.c | 4 +++-
>>>    1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/parisc/iosapic.c b/drivers/parisc/iosapic.c
>>> index 3a8c98615634..33de438916d3 100644
>>> --- a/drivers/parisc/iosapic.c
>>> +++ b/drivers/parisc/iosapic.c
>>> @@ -229,7 +229,9 @@ static struct irt_entry *iosapic_alloc_irt(int
>>> num_entries)>
>>>    	 * 4-byte alignment on 32-bit kernels
>>>    	 */
>>>
>>>    	a = (unsigned long)kmalloc(sizeof(struct irt_entry) * num_entries
> + 8,
>>>    	GFP_KERNEL);>
>>> -	a = (a + 7UL) & ~7UL;
>>> +	if (a)
>>> +		a = (a + 7UL) & ~7UL;
>>> +
>>
>> As you said, the adjustment isn't required, but it's still ok.
>> So I think the additional "if" isn't necessary and so I'm not
>> applying your patch.
>>
>> Anyway, thanks for your help to try to improve the code!
>
> I was about to say the same, but from looking at the code I don't think what
> is in there is correct either. The comment seems outdated, because
> __assume_kmalloc_alignment, which is __alignof__(unsigned long long). This
> code is untouched for the entire git history, so maybe we can just change the
> whole thing to
>
>    return kcalloc(num_entries, sizeof(struct irt_entry))
>
> now?

Yes, your proposal is good.
Anyone want to send a patch (with a small comment that kcalloc() will return
at least the required 8-byte alignment)?

> And these functions end up propagating an allocation error in this file and it
> will never reach kernel/setup.c, which seems bad.

That part I don't understand.
The return value of iosapic_alloc_irt() is checked afterwards, but you probably
meant something else?

> But I guess the only point where this really can go wrong if the PDC
> returns an absurdly large number of entries.
Helge

Re: [PATCH] parisc: Increase the usage check of kmalloc allocated object a

[Rolf Eike Beer]

[-- Attachment #1: Type: text/plain, Size: 1113 bytes --]

Am Mittwoch, 14. September 2022, 11:04:33 CEST schrieb Helge Deller:
> On 9/14/22 08:43, Rolf Eike Beer wrote:
> > Am Mittwoch, 14. September 2022, 08:18:19 CEST schrieb Helge Deller:
> >> On 9/14/22 08:04, Li zeming wrote:

> Yes, your proposal is good.
> Anyone want to send a patch (with a small comment that kcalloc() will return
> at least the required 8-byte alignment)?

Done.

> > And these functions end up propagating an allocation error in this file
> > and it will never reach kernel/setup.c, which seems bad.
> 
> That part I don't understand.
> The return value of iosapic_alloc_irt() is checked afterwards, but you
> probably meant something else?
> 
> > But I guess the only point where this really can go wrong if the PDC
> > returns an absurdly large number of entries.

What I meant was that if iosapic_alloc_irt() fails, then iosapic_load_irt() 
will return 0, which can either be "nothing to do" or "error". iosapic_init() 
is void, so even if it could detect the failure, it can't report it upwards to 
parisc_init(). Which is the same for basically all other *_init() calls in 
there.

Eike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH] parisc: Increase the usage check of kmalloc allocated object a

[Helge Deller]

On 9/14/22 16:25, Rolf Eike Beer wrote:
> Am Mittwoch, 14. September 2022, 11:04:33 CEST schrieb Helge Deller:
>> On 9/14/22 08:43, Rolf Eike Beer wrote:
>>> Am Mittwoch, 14. September 2022, 08:18:19 CEST schrieb Helge Deller:
>>>> On 9/14/22 08:04, Li zeming wrote:
>
>> Yes, your proposal is good.
>> Anyone want to send a patch (with a small comment that kcalloc() will return
>> at least the required 8-byte alignment)?
>
> Done.
>
>>> And these functions end up propagating an allocation error in this file
>>> and it will never reach kernel/setup.c, which seems bad.
>>
>> That part I don't understand.
>> The return value of iosapic_alloc_irt() is checked afterwards, but you
>> probably meant something else?
>>
>>> But I guess the only point where this really can go wrong if the PDC
>>> returns an absurdly large number of entries.
>
> What I meant was that if iosapic_alloc_irt() fails, then iosapic_load_irt()
> will return 0, which can either be "nothing to do" or "error". iosapic_init()
> is void, so even if it could detect the failure, it can't report it upwards to
> parisc_init(). Which is the same for basically all other *_init() calls in
> there.

Ok, I see.
Not sure if that needs fixing. If the allocation fails we will be in trouble anyway :-)

Helge