[PATCH] ipv4: nexthop: Fix deadcode issue by performing a proper NULL check

[PATCH] ipv4: nexthop: Fix deadcode issue by performing a proper NULL check

[patrickeigensatz]

From: Patrick Eigensatz <patrickeigensatz@gmail.com>

After allocating the spare nexthop group it should be tested for kzalloc()
returning NULL, instead the already used nexthop group (which cannot be
NULL at this point) had been tested so far.

Additionally, if kzalloc() fails, return ERR_PTR(-ENOMEM) instead of NULL.

Coverity-id: 1463885
Reported-by: Coverity <scan-admin@coverity.com>
Signed-off-by: Patrick Eigensatz <patrickeigensatz@gmail.com>
---
 net/ipv4/nexthop.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/nexthop.c b/net/ipv4/nexthop.c
index 563f71bcb2d7..cb9412cd5e4b 100644
--- a/net/ipv4/nexthop.c
+++ b/net/ipv4/nexthop.c
@@ -1118,10 +1118,10 @@ static struct nexthop *nexthop_create_group(struct net *net,
 
 	/* spare group used for removals */
 	nhg->spare = nexthop_grp_alloc(num_nh);
-	if (!nhg) {
+	if (!nhg->spare) {
 		kfree(nhg);
 		kfree(nh);
-		return NULL;
+		return ERR_PTR(-ENOMEM);
 	}
 	nhg->spare->spare = nhg;
 
-- 
2.26.2

Re: [PATCH] ipv4: nexthop: Fix deadcode issue by performing a proper NULL check

[Nikolay Aleksandrov]

On 01/06/2020 14:12, patrickeigensatz@gmail.com wrote:
> From: Patrick Eigensatz <patrickeigensatz@gmail.com>
> 
> After allocating the spare nexthop group it should be tested for kzalloc()
> returning NULL, instead the already used nexthop group (which cannot be
> NULL at this point) had been tested so far.
> 
> Additionally, if kzalloc() fails, return ERR_PTR(-ENOMEM) instead of NULL.
> 
> Coverity-id: 1463885
> Reported-by: Coverity <scan-admin@coverity.com>
> Signed-off-by: Patrick Eigensatz <patrickeigensatz@gmail.com>
> ---
>  net/ipv4/nexthop.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/net/ipv4/nexthop.c b/net/ipv4/nexthop.c
> index 563f71bcb2d7..cb9412cd5e4b 100644
> --- a/net/ipv4/nexthop.c
> +++ b/net/ipv4/nexthop.c
> @@ -1118,10 +1118,10 @@ static struct nexthop *nexthop_create_group(struct net *net,
>  
>  	/* spare group used for removals */
>  	nhg->spare = nexthop_grp_alloc(num_nh);
> -	if (!nhg) {
> +	if (!nhg->spare) {
>  		kfree(nhg);
>  		kfree(nh);
> -		return NULL;
> +		return ERR_PTR(-ENOMEM);
>  	}
>  	nhg->spare->spare = nhg;
>  
> 

As Colin's similar patch[1] was rejected recently, this one also fixes the issue.
This is targeted at -net.

Fixes: 90f33bffa382 ("nexthops: don't modify published nexthop groups")
Acked-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>

Thanks!

[1] https://lkml.org/lkml/2020/5/28/909

Re: [PATCH] ipv4: nexthop: Fix deadcode issue by performing a proper NULL check

[David Miller]

From: patrickeigensatz@gmail.com
Date: Mon,  1 Jun 2020 13:12:01 +0200

> From: Patrick Eigensatz <patrickeigensatz@gmail.com>
> 
> After allocating the spare nexthop group it should be tested for kzalloc()
> returning NULL, instead the already used nexthop group (which cannot be
> NULL at this point) had been tested so far.
> 
> Additionally, if kzalloc() fails, return ERR_PTR(-ENOMEM) instead of NULL.
> 
> Coverity-id: 1463885
> Reported-by: Coverity <scan-admin@coverity.com>
> Signed-off-by: Patrick Eigensatz <patrickeigensatz@gmail.com>

Applied, thank you.

Re: [PATCH] ipv4: nexthop: Fix deadcode issue by performing a proper NULL check

[Nikolay Aleksandrov]

On 01/06/2020 21:06, David Miller wrote:
> From: patrickeigensatz@gmail.com
> Date: Mon,  1 Jun 2020 13:12:01 +0200
> 
>> From: Patrick Eigensatz <patrickeigensatz@gmail.com>
>>
>> After allocating the spare nexthop group it should be tested for kzalloc()
>> returning NULL, instead the already used nexthop group (which cannot be
>> NULL at this point) had been tested so far.
>>
>> Additionally, if kzalloc() fails, return ERR_PTR(-ENOMEM) instead of NULL.
>>
>> Coverity-id: 1463885
>> Reported-by: Coverity <scan-admin@coverity.com>
>> Signed-off-by: Patrick Eigensatz <patrickeigensatz@gmail.com>
> 
> Applied, thank you.
> 

Hi Dave,
I see this patch in -net-next but it should've been in -net as I wrote in my
review[1]. This patch should go along with the recent nexthop set that fixes
a few bugs, since it could result in a null ptr deref if the spare group cannot
be allocated.
How would you like to proceed? Should it be submitted for -net as well?

Thanks,
 Nik

[1] https://lkml.org/lkml/2020/6/1/391

Re: [PATCH] ipv4: nexthop: Fix deadcode issue by performing a proper NULL check

[Nikolay Aleksandrov]

On 02/06/2020 10:23, Nikolay Aleksandrov wrote:
> On 01/06/2020 21:06, David Miller wrote:
>> From: patrickeigensatz@gmail.com
>> Date: Mon,  1 Jun 2020 13:12:01 +0200
>>
>>> From: Patrick Eigensatz <patrickeigensatz@gmail.com>
>>>
>>> After allocating the spare nexthop group it should be tested for kzalloc()
>>> returning NULL, instead the already used nexthop group (which cannot be
>>> NULL at this point) had been tested so far.
>>>
>>> Additionally, if kzalloc() fails, return ERR_PTR(-ENOMEM) instead of NULL.
>>>
>>> Coverity-id: 1463885
>>> Reported-by: Coverity <scan-admin@coverity.com>
>>> Signed-off-by: Patrick Eigensatz <patrickeigensatz@gmail.com>
>>
>> Applied, thank you.
>>
> 
> Hi Dave,
> I see this patch in -net-next but it should've been in -net as I wrote in my
> review[1]. This patch should go along with the recent nexthop set that fixes
> a few bugs, since it could result in a null ptr deref if the spare group cannot
> be allocated.

Obviously I forgot to mention in my review that it should go to -stable with the
nexthop fix set.

> How would you like to proceed? Should it be submitted for -net as well?
> 
> Thanks,
>  Nik
> 
> [1] https://lkml.org/lkml/2020/6/1/391
> 

Re: [PATCH] ipv4: nexthop: Fix deadcode issue by performing a proper NULL check

[David Miller]

From: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
Date: Tue, 2 Jun 2020 10:23:09 +0300

> On 01/06/2020 21:06, David Miller wrote:
>> From: patrickeigensatz@gmail.com
>> Date: Mon,  1 Jun 2020 13:12:01 +0200
>> 
>>> From: Patrick Eigensatz <patrickeigensatz@gmail.com>
>>>
>>> After allocating the spare nexthop group it should be tested for kzalloc()
>>> returning NULL, instead the already used nexthop group (which cannot be
>>> NULL at this point) had been tested so far.
>>>
>>> Additionally, if kzalloc() fails, return ERR_PTR(-ENOMEM) instead of NULL.
>>>
>>> Coverity-id: 1463885
>>> Reported-by: Coverity <scan-admin@coverity.com>
>>> Signed-off-by: Patrick Eigensatz <patrickeigensatz@gmail.com>
>> 
>> Applied, thank you.
>> 
> 
> Hi Dave,
> I see this patch in -net-next but it should've been in -net as I wrote in my
> review[1]. This patch should go along with the recent nexthop set that fixes
> a few bugs, since it could result in a null ptr deref if the spare group cannot
> be allocated.
> How would you like to proceed? Should it be submitted for -net as well?

When I'm leading up to the merge window I just toss everything into net-next
and still queue things to -stable as needed.

Re: [PATCH] ipv4: nexthop: Fix deadcode issue by performing a proper NULL check

[Nikolay Aleksandrov]

On 03/06/2020 00:01, David Miller wrote:
> From: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
> Date: Tue, 2 Jun 2020 10:23:09 +0300
> 
>> On 01/06/2020 21:06, David Miller wrote:
>>> From: patrickeigensatz@gmail.com
>>> Date: Mon,  1 Jun 2020 13:12:01 +0200
>>>
>>>> From: Patrick Eigensatz <patrickeigensatz@gmail.com>
>>>>
>>>> After allocating the spare nexthop group it should be tested for kzalloc()
>>>> returning NULL, instead the already used nexthop group (which cannot be
>>>> NULL at this point) had been tested so far.
>>>>
>>>> Additionally, if kzalloc() fails, return ERR_PTR(-ENOMEM) instead of NULL.
>>>>
>>>> Coverity-id: 1463885
>>>> Reported-by: Coverity <scan-admin@coverity.com>
>>>> Signed-off-by: Patrick Eigensatz <patrickeigensatz@gmail.com>
>>>
>>> Applied, thank you.
>>>
>>
>> Hi Dave,
>> I see this patch in -net-next but it should've been in -net as I wrote in my
>> review[1]. This patch should go along with the recent nexthop set that fixes
>> a few bugs, since it could result in a null ptr deref if the spare group cannot
>> be allocated.
>> How would you like to proceed? Should it be submitted for -net as well?
> 
> When I'm leading up to the merge window I just toss everything into net-next
> and still queue things to -stable as needed.
> 

Got it, in that case could you please queue the patch for -stable?

I checked https://patchwork.ozlabs.org/bundle/davem/stable/?state=* and
https://git.kernel.org/pub/scm/linux/kernel/git/stable/stable-queue.git
but didn't find it.

Thanks,
 Nik