[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.

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

[Pekka J Enberg]

On 3/9/07, David Miller <davem@davemloft.net> wrote:
> The whole cahce-multipath subsystem has to have it's guts revamped for
> proper error handling.

(Untested patch follows.)

From: Amit Choudhary <amit2030@gmail.com>

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

[penberg@cs.helsinki.fi: return error status to caller.]
Signed-off-by: Amit Choudhary <amit2030@gmail.com>
Signed-off-by: Pekka Enberg <penberg@cs.helsinki.fi>
---
 include/net/ip_mp_alg.h      |    8 +++++---
 net/ipv4/multipath_wrandom.c |   19 +++++++++++++++----
 net/ipv4/route.c             |    9 +++++++--
 3 files changed, 27 insertions(+), 9 deletions(-)

Index: 2.6/include/net/ip_mp_alg.h
===================================================================
--- 2.6.orig/include/net/ip_mp_alg.h	2007-03-12 14:00:13.000000000 +0200
+++ 2.6/include/net/ip_mp_alg.h	2007-03-12 14:03:10.000000000 +0200
@@ -17,7 +17,7 @@ struct ip_mp_alg_ops {
 	void	(*mp_alg_select_route)(const struct flowi *flp,
 				       struct rtable *rth, struct rtable **rp);
 	void	(*mp_alg_flush)(void);
-	void	(*mp_alg_set_nhinfo)(__be32 network, __be32 netmask,
+	int	(*mp_alg_set_nhinfo)(__be32 network, __be32 netmask,
 				     unsigned char prefixlen,
 				     const struct fib_nh *nh);
 	void	(*mp_alg_remove)(struct rtable *rth);
@@ -58,17 +58,19 @@ static inline void multipath_flush(void)
 #endif
 }
 
-static inline void multipath_set_nhinfo(struct rtable *rth,
+static inline int multipath_set_nhinfo(struct rtable *rth,
 					__be32 network, __be32 netmask,
 					unsigned char prefixlen,
 					const struct fib_nh *nh)
 {
+	int err = 0;
 #ifdef CONFIG_IP_ROUTE_MULTIPATH_CACHED
 	struct ip_mp_alg_ops *ops = ip_mp_alg_table[rth->rt_multipath_alg];
 
 	if (ops && ops->mp_alg_set_nhinfo)
-		ops->mp_alg_set_nhinfo(network, netmask, prefixlen, nh);
+		err = ops->mp_alg_set_nhinfo(network, netmask, prefixlen, nh);
 #endif
+	return err;
 }
 
 static inline void multipath_remove(struct rtable *rth)
Index: 2.6/net/ipv4/multipath_wrandom.c
===================================================================
--- 2.6.orig/net/ipv4/multipath_wrandom.c	2007-03-12 14:00:33.000000000 +0200
+++ 2.6/net/ipv4/multipath_wrandom.c	2007-03-12 14:02:17.000000000 +0200
@@ -216,14 +216,15 @@ 	last_power = 0;
 	*rp = decision;
 }
 
-static void wrandom_set_nhinfo(__be32 network,
-			       __be32 netmask,
-			       unsigned char prefixlen,
-			       const struct fib_nh *nh)
+static int wrandom_set_nhinfo(__be32 network,
+			      __be32 netmask,
+			      unsigned char prefixlen,
+			      const struct fib_nh *nh)
 {
 	const int state_idx = nh->nh_oif % MULTIPATH_STATE_SIZE;
 	struct multipath_route *r, *target_route = NULL;
 	struct multipath_dest *d, *target_dest = NULL;
+	int err = 0;
 
 	/* store the weight information for a certain route */
 	spin_lock_bh(&state[state_idx].lock);
@@ -240,6 +241,10 @@ static void wrandom_set_nhinfo(__be32 ne
 		const size_t size_rt = sizeof(struct multipath_route);
 		target_route = (struct multipath_route *)
 			kmalloc(size_rt, GFP_ATOMIC);
+		if (!target_route) {
+			err = -ENOMEM;
+			goto error;
+		}
 
 		target_route->gw = nh->nh_gw;
 		target_route->oif = nh->nh_oif;
@@ -262,6 +267,10 @@ 		memset(&target_route->rcu, 0, sizeof(s
 		target_dest = (struct multipath_dest*)
 			kmalloc(size_dst, GFP_ATOMIC);
 
+		if (!target_dest) {
+			err = -ENOMEM;
+			goto error;
+		}
 		target_dest->nh_info = nh;
 		target_dest->network = network;
 		target_dest->netmask = netmask;
@@ -275,6 +284,8 @@ 		memset(&target_dest->rcu, 0, sizeof(st
 	 */
 
 	spin_unlock_bh(&state[state_idx].lock);
+  error:
+	return err;
 }
 
 static void __multipath_free(struct rcu_head *head)
Index: 2.6/net/ipv4/route.c
===================================================================
--- 2.6.orig/net/ipv4/route.c	2007-03-12 14:03:24.000000000 +0200
+++ 2.6/net/ipv4/route.c	2007-03-12 14:04:16.000000000 +0200
@@ -1880,11 +1880,13 @@ 	for (hop = 0; hop < hopcount; hop++) {
 			return err;
 
 		/* forward hop information to multipath impl. */
-		multipath_set_nhinfo(rth,
+		err = multipath_set_nhinfo(rth,
 				     FIB_RES_NETWORK(*res),
 				     FIB_RES_NETMASK(*res),
 				     res->prefixlen,
 				     &FIB_RES_NH(*res));
+		if (err)
+			return err;
 	}
 	skb->dst = &rtres->u.dst;
 	return err;
@@ -2334,8 +2336,11 @@ 			if (err != 0)
 					oldflp->oif);
 			err = rt_intern_hash(hash, rth, rp);
 
+			if (err != 0)
+				goto cleanup;
+
 			/* forward hop information to multipath impl. */
-			multipath_set_nhinfo(rth,
+			err = multipath_set_nhinfo(rth,
 					     FIB_RES_NETWORK(*res),
 					     FIB_RES_NETMASK(*res),
 					     res->prefixlen,

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

[David Miller]

From: Pekka J Enberg <penberg@cs.helsinki.fi>
Date: Mon, 12 Mar 2007 14:15:16 +0200 (EET)

> On 3/9/07, David Miller <davem@davemloft.net> wrote:
> > The whole cahce-multipath subsystem has to have it's guts revamped for
> > proper error handling.
> 
> (Untested patch follows.)

I'm not accepting untested patches, if people want to fix this
code it cannot be done in a shoddy manner any more, it will
need to be fixed and tested by people who really care about
this code.

We need real care for this code or I will remove it in 2.6.23

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

[Ravikiran G Thirumalai]

On Mon, Mar 12, 2007 at 01:56:13PM -0700, David Miller wrote:
> From: Pekka J Enberg <penberg@cs.helsinki.fi>
> Date: Mon, 12 Mar 2007 14:15:16 +0200 (EET)
> 
> > On 3/9/07, David Miller <davem@davemloft.net> wrote:
> > > The whole cahce-multipath subsystem has to have it's guts revamped for
> > > proper error handling.
> > 
> > (Untested patch follows.)
> 
> I'm not accepting untested patches, if people want to fix this
> code it cannot be done in a shoddy manner any more, it will
> need to be fixed and tested by people who really care about
> this code.

Hi Dave,
We do care for multipath cached option.  We do use it often with node
aware multipath.  We haven't experienced oopsen or crashes for our use cases
since the past year.  We will test Amit and Pekka's patch with fault injection.
We will also go through the core multipath-cached code and give a try at
cleaning it up.

> 
> We need real care for this code or I will remove it in 2.6.23

Can you please point us to any open bug reports on multipath cached?  We did 
a Call for testing on netdev sometime back but did not get any bug reports.  
http://marc.info/?l=linux-netdev&m=114827370231872&w=2

The kernel bugzilla shows zaroo boogs for multipath cached as well.

Thanks,
Kiran

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

[David Miller]

From: Ravikiran G Thirumalai <kiran@scalex86.org>
Date: Wed, 14 Mar 2007 13:15:42 -0700

> The kernel bugzilla shows zaroo boogs for multipath cached as well.

Because _EVERYBODY_ turns it off because we tell them to do so!  Every
bug reported to the lists is replied to with "that code is
unbelievably buggy, just turn that off, kthx, bye".  And to reiterate
no distribution turns it on in there kernels, of course there are no
bugs reported in code nobody actually runs.

And it's been that way since day one for this code.

I also want to get rid of this code because it causes major
architectural limitations in the routing code which prevents important
work we want to do in this area.  Even the most skilled core
networking developers don't have the stomache to try and repair
what we have now, and yes they have looked at it and tried.

I'm still very much going to get rid of it, sorry.  It's unrepairable.

My deepest apologies if this inconveniences you personally, but this
is open source and you can always carry a local patch to obtain the
functionality.  In fact, it would be awesome if you took up the
project to rewrite this stuff cleanly from the start, I'd be
absolutely happy to review and integrate such patches.

But for now, what's in there has to go and there is absolutely
no question about it.