Skip to contentSkip to conversation

net/ipv4/multipath_wrandom.c: check kmalloc() return value.

PATCH In review Amit Choudhary 5 participants 9 messages Mar 9, 2007 – Mar 13, 2007

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

Amit Choudhary

author
#0 lore raw

PATCH 1/1 net/ipv4/multipath_wrandom.c: check kmalloc() return value.

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

Signed-off-by Amit Choudhary <[email protected]>
net/ipv4/multipath_wrandom.c +7 -0
index 92b0482..bcdb1f1 100644
@@ -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);
 }
maintainer
#1 lore raw

From: Amit Choudhary <[email protected]>
Date: Thu, 8 Mar 2007 23:22:15 -0800

Quoted 3 lines

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

Signed-off-by Amit Choudhary <[email protected]>

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.

#2 lore raw
Quoted 15 lines · David Miller

From: Amit Choudhary <[email protected]>
Date: Thu, 8 Mar 2007 23:22:15 -0800

Quoted 3 lines

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

Signed-off-by Amit Choudhary <[email protected]>

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.

Quoted 5 lines

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.

#3 lore raw
Quoted 2 lines · Jarek Poplawski

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.

#4 lore raw
Quoted 6 lines · Pekka Enberg
Quoted 2 lines · Jarek Poplawski

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.

maintainer
#6 lore raw

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

In reply to Jarek Poplawski

From: Jarek Poplawski <[email protected]>
Date: Mon, 12 Mar 2007 12:51:37 +0100

Quoted 4 lines

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.

maintainer
#5 lore raw
In reply to Pekka Enberg

From: "Pekka Enberg" <[email protected]>
Date: Mon, 12 Mar 2007 14:36:46 +0200

Quoted 5 lines · Jarek Poplawski
Quoted 2 lines

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.

#7 lore raw

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

In reply to David Miller
Quoted 18 lines · On Mon, 12 Mar 2007 13:53:11 -0700 (PDT) David Miller <[email protected]>…

From: Jarek Poplawski <[email protected]>
Date: Mon, 12 Mar 2007 12:51:37 +0100

Quoted 4 lines

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.

Quoted 7 lines

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.

#8 lore raw

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

Quoted 1 line · Andrew Morton
Quoted 1 line · On Mon, 12 Mar 2007 13:53:11 -0700 (PDT) David Miller <[email protected]>…

...

Quoted 9 lines
Quoted 4 lines

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.