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);
 }
 
-

[ message continues ]

From: 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.
-

[ message continues ]

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

[ message continues ]

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

[ message continues ]

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

[ message continues ]

From: "Pekka Enberg" <penberg@cs.helsinki.fi>

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

[ message continues ]

From: Jarek Poplawski <jarkao2@o2.pl>

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

[ message continues ]

Good stuff.

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

[ message continues ]

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

Jarek P.
-

[ message continues ]