[PATCH] [REVISED] net/ipv4/multipath_wrandom.c: check kmalloc() return value.

[PATCH] [REVISED] net/ipv4/multipath_wrandom.c: check kmalloc() return value.

[Amit Choudhary]

Description: Check the return value of kmalloc() in function wrandom_set_nhinfo(), in file net/ipv4/multipath_wrandom.c.

Signed-off-by: Amit Choudhary <amit2030@gmail.com>

diff --git a/net/ipv4/multipath_wrandom.c b/net/ipv4/multipath_wrandom.c
index 92b0482..bcdb1f1 100644
--- a/net/ipv4/multipath_wrandom.c
+++ b/net/ipv4/multipath_wrandom.c
@@ -242,6 +242,9 @@ static void wrandom_set_nhinfo(__be32 ne
 		target_route = (struct multipath_route *)
 			kmalloc(size_rt, GFP_ATOMIC);
 
+		if (!target_route)
+			goto error;
+
 		target_route->gw = nh->nh_gw;
 		target_route->oif = nh->nh_oif;
 		memset(&target_route->rcu, 0, sizeof(struct rcu_head));
@@ -263,6 +266,9 @@ static void wrandom_set_nhinfo(__be32 ne
 		target_dest = (struct multipath_dest*)
 			kmalloc(size_dst, GFP_ATOMIC);
 
+		if (!target_dest)
+			goto error;
+
 		target_dest->nh_info = nh;
 		target_dest->network = network;
 		target_dest->netmask = netmask;
@@ -275,6 +281,7 @@ static void wrandom_set_nhinfo(__be32 ne
 	 * we are finished
 	 */
 
+ error:
 	spin_unlock_bh(&state[state_idx].lock);
 }
 

Re: [PATCH] [REVISED] net/ipv4/multipath_wrandom.c: check kmalloc() return value.

[David Miller]

From: Amit Choudhary <amit2030@gmail.com>
Date: Thu, 8 Mar 2007 23:22:15 -0800

> Description: Check the return value of kmalloc() in function wrandom_set_nhinfo(), in file net/ipv4/multipath_wrandom.c.
> 
> Signed-off-by: Amit Choudhary <amit2030@gmail.com>

This kind of patch has been submitted several times before and it's
never accepted because you have to do much more than this to recover
from the allocation error.

There is no error status returned to the caller, so the callers assume
the operation succeeded, and will either OOPS or crash in some other
way.

Therefore, just adding some NULL pointer checks and returning is not
going to fix this bug.

The whole cahce-multipath subsystem has to have it's guts revamped for
proper error handling.

Re: [PATCH] [REVISED] net/ipv4/multipath_wrandom.c: check kmalloc() return value.

[Jarek Poplawski]

On 09-03-2007 08:29, David Miller wrote:
> From: Amit Choudhary <amit2030@gmail.com>
> Date: Thu, 8 Mar 2007 23:22:15 -0800
> 
>> Description: Check the return value of kmalloc() in function wrandom_set_nhinfo(), in file net/ipv4/multipath_wrandom.c.
>>
>> Signed-off-by: Amit Choudhary <amit2030@gmail.com>
> 
> This kind of patch has been submitted several times before and it's
> never accepted because you have to do much more than this to recover
> from the allocation error.
> 
> There is no error status returned to the caller, so the callers assume
> the operation succeeded, and will either OOPS or crash in some other
> way.
> 
> Therefore, just adding some NULL pointer checks and returning is not
> going to fix this bug.
> 
> The whole cahce-multipath subsystem has to have it's guts revamped for
> proper error handling.

But until then it'll unnecessarily spoil linux opinion as regards
stability and waste time of developers to check error messages.
So, maybe it's less evil to check those NULLs where possible and add
some WARN_ONs here and there...

Regards,
Jarek P.

Re: [PATCH] [REVISED] net/ipv4/multipath_wrandom.c: check kmalloc() return value.

[Pekka Enberg]

On 3/12/07, Jarek Poplawski <jarkao2@o2.pl> wrote:
> So, maybe it's less evil to check those NULLs where possible and add
> some WARN_ONs here and there...

No, it's much better to oops rather than paper over a bug.

Re: [PATCH] [REVISED] net/ipv4/multipath_wrandom.c: check kmalloc() return value.

[Jarek Poplawski]

On Mon, Mar 12, 2007 at 02:36:46PM +0200, Pekka Enberg wrote:
> On 3/12/07, Jarek Poplawski <jarkao2@o2.pl> wrote:
> >So, maybe it's less evil to check those NULLs where possible and add
> >some WARN_ONs here and there...
> 
> No, it's much better to oops rather than paper over a bug.
> 

I'm not sure I can understand your intentions - do you mean
always better? In my opinion oops is right only to avoid
some danger. And here is no danger - some routing info,
which is internal multipath_xxx data and can be handled
safely, will not go into its cache.

Jarek P.

Re: [PATCH] [REVISED] net/ipv4/multipath_wrandom.c: check kmalloc() return value.

[David Miller]

From: "Pekka Enberg" <penberg@cs.helsinki.fi>
Date: Mon, 12 Mar 2007 14:36:46 +0200

> On 3/12/07, Jarek Poplawski <jarkao2@o2.pl> wrote:
> > So, maybe it's less evil to check those NULLs where possible and add
> > some WARN_ONs here and there...
> 
> No, it's much better to oops rather than paper over a bug.

The caller is going to OOPS in this case if you return after an
allocation failure, so there is no improvement after the patch.

Multipath-cached just sucks, it was thrown over the wall by IBM
Germany then never had any followon maintainence at all, so it will be
removed unless someone steps up to seriously maintain and fix that
code, and I REALLY MEAN IT.

Removal of multipath cached (was Re: [PATCH] [REVISED] net/ipv4/multipath_wrandom.c: check kmalloc() return value.)

[David Miller]

From: Jarek Poplawski <jarkao2@o2.pl>
Date: Mon, 12 Mar 2007 12:51:37 +0100

> But until then it'll unnecessarily spoil linux opinion as regards
> stability and waste time of developers to check error messages.
> So, maybe it's less evil to check those NULLs where possible and add
> some WARN_ONs here and there...

It's a crash either way, so zero improvement.

And _THIS_ is my big problem with the multi-path cached code in the
kernel.

NOBODY wants to step up and fix the code, but people refuse to let it
get removed from the tree.  That is totally unacceptable, so I'm going
to FIX THIS.

I'm going to FIX IT by saying that if nobody steps up to the plate to
fix the multipath cached code by 2.6.23 IT IS GONE forver.

And there is absolutely no negotiations about this, I've held back on
this for nearly 2 years, and nothing has happened, this code is not
maintained, nobody cares enough to fix the bugs, and even no
distributions enable it because it causes crashes.

Re: Removal of multipath cached (was Re: [PATCH] [REVISED] net/ipv4/multipath_wrandom.c: check kmalloc() return value.)

[Andrew Morton]

> On Mon, 12 Mar 2007 13:53:11 -0700 (PDT) David Miller <davem@davemloft.net> wrote:
> From: Jarek Poplawski <jarkao2@o2.pl>
> Date: Mon, 12 Mar 2007 12:51:37 +0100
> 
> > But until then it'll unnecessarily spoil linux opinion as regards
> > stability and waste time of developers to check error messages.
> > So, maybe it's less evil to check those NULLs where possible and add
> > some WARN_ONs here and there...
> 
> It's a crash either way, so zero improvement.
> 
> And _THIS_ is my big problem with the multi-path cached code in the
> kernel.
> 
> NOBODY wants to step up and fix the code, but people refuse to let it
> get removed from the tree.  That is totally unacceptable, so I'm going
> to FIX THIS.
> 
> I'm going to FIX IT by saying that if nobody steps up to the plate to
> fix the multipath cached code by 2.6.23 IT IS GONE forver.
> 
> And there is absolutely no negotiations about this, I've held back on
> this for nearly 2 years, and nothing has happened, this code is not
> maintained, nobody cares enough to fix the bugs, and even no
> distributions enable it because it causes crashes.

Good stuff.

I suggest you put a big printk explaining the above into 2.6.21.

Re: Removal of multipath cached (was Re: [PATCH] [REVISED] net/ipv4/multipath_wrandom.c: check kmalloc() return value.)

[Jarek Poplawski]

On Mon, Mar 12, 2007 at 10:22:36PM -0800, Andrew Morton wrote:
> > On Mon, 12 Mar 2007 13:53:11 -0700 (PDT) David Miller <davem@davemloft.net> wrote:
...
> > And there is absolutely no negotiations about this, I've held back on
> > this for nearly 2 years, and nothing has happened, this code is not
> > maintained, nobody cares enough to fix the bugs, and even no
> > distributions enable it because it causes crashes.
> 
> Good stuff.
> 
> I suggest you put a big printk explaining the above into 2.6.21.
> 

Plus official way: Documentation/feature-remove-schedule.txt
in the next rc-git.

Jarek P.