[patch] ixgbevf: potential NULL dereference on allocation failure

[patch] ixgbevf: potential NULL dereference on allocation failure

[Dan Carpenter]

If "rx_ring" is NULL then it will oops when we try:

	memcpy(rx_ring, adapter->rx_ring,
		adapter->num_rx_queues * sizeof(struct ixgbevf_ring));

Signed-off-by: Dan Carpenter <error27@gmail.com>
---
To be honest, I'm not sure why the check for need_tx_update is there.
This change has only been compile tested.

diff --git a/drivers/net/ixgbevf/ethtool.c b/drivers/net/ixgbevf/ethtool.c
index 4680b06..7f194aa 100644
--- a/drivers/net/ixgbevf/ethtool.c
+++ b/drivers/net/ixgbevf/ethtool.c
@@ -385,7 +385,7 @@ static int ixgbevf_set_ringparam(struct net_device *netdev,
 	if (new_rx_count != adapter->rx_ring_count) {
 		rx_ring = kcalloc(adapter->num_rx_queues,
 				  sizeof(struct ixgbevf_ring), GFP_KERNEL);
-		if ((!rx_ring) && (need_tx_update)) {
+		if (!rx_ring) {
 			err = -ENOMEM;
 			goto err_rx_setup;
 		}

Re: [patch] ixgbevf: potential NULL dereference on allocation failure

[David Miller]

From: Dan Carpenter <error27@gmail.com>
Date: Fri, 10 Sep 2010 13:52:34 +0200

> If "rx_ring" is NULL then it will oops when we try:
> 
> 	memcpy(rx_ring, adapter->rx_ring,
> 		adapter->num_rx_queues * sizeof(struct ixgbevf_ring));
> 
> Signed-off-by: Dan Carpenter <error27@gmail.com>
> ---
> To be honest, I'm not sure why the check for need_tx_update is there.
> This change has only been compile tested.

It's trying to optimize out the "down/up" of the device, which needs
to be done if we allocated a new TX ring.

It also adjusts the semantics of the error return,  in that if the
TX ring re-sizing went OK but the RX resizing failed, it returns
success.

That's kind of crummy semantics, if any part fails we should unwind
and return an error.  So just do the necessary memory allocations
first, and don't make any changes unless they all succeed.

This code also seems to be incredibly racy.  It allocates the new RING
structure, and copies the existing entries over.  Meanwhile the chip
is still running and we're potentially processing these same ring
entries, so by the time we actually assign adapter->{rx,tx}_ring
pointers the contents could have changed.

Probably the simplest thing to do is to structure this such that the
chip is quiesced around the entire ring set operation, so something
like:

	tx_ring = kcalloc();
	if (!tx_ring)
		goto do_err;
	rx_ring = kcalloc();
	if (!rx_ring)
		goto rx_ring_free_err;

	ixgbevf_down(adapter);

	err = setup_tx_ring(adapter, tx_ring);
	if (err)
		goto device_up_err;
	err = setup_rx_ring(adapter, rx_ring);
	if (err)
		goto device_up_err;

	ixgbevf_up(adapter);

	return 0;

device_up_err:
	tear_down_tx_ring(adapter, tx_ring);
	tear_down_rx_ring(adapter, rx_ring);
	kfree(tx_ring);
rx_ring_free_err:
	kfree(rx_ring);
do_err:
	return err;

RE: [patch] ixgbevf: potential NULL dereference on allocation failure

[Rose, Gregory V]

>-----Original Message-----
>From: David Miller [mailto:davem@davemloft.net]
>Sent: Friday, September 10, 2010 1:22 PM
>To: error27@gmail.com
>Cc: Rose, Gregory V; Kirsher, Jeffrey T; netdev@vger.kernel.org; kernel-
>janitors@vger.kernel.org
>Subject: Re: [patch] ixgbevf: potential NULL dereference on allocation
>failure
>
>It's trying to optimize out the "down/up" of the device, which needs
>to be done if we allocated a new TX ring.
>
>It also adjusts the semantics of the error return,  in that if the
>TX ring re-sizing went OK but the RX resizing failed, it returns
>success.
>
>That's kind of crummy semantics, if any part fails we should unwind
>and return an error.  So just do the necessary memory allocations
>first, and don't make any changes unless they all succeed.
>
>This code also seems to be incredibly racy.  It allocates the new RING
>structure, and copies the existing entries over.  Meanwhile the chip
>is still running and we're potentially processing these same ring
>entries, so by the time we actually assign adapter->{rx,tx}_ring
>pointers the contents could have changed.
>
>Probably the simplest thing to do is to structure this such that the
>chip is quiesced around the entire ring set operation, so something
>like:

[snip]

I'll take this up and see what I can do to fix it up a bit and address your concerns.  Thanks for the suggestions.

- Greg

RE: [patch] ixgbevf: potential NULL dereference on allocation failure

[Rose, Gregory V]

>-----Original Message-----
>From: David Miller [mailto:davem@davemloft.net]
>Sent: Friday, September 10, 2010 1:22 PM
>To: error27@gmail.com
>Cc: Rose, Gregory V; Kirsher, Jeffrey T; netdev@vger.kernel.org; kernel-
>janitors@vger.kernel.org
>Subject: Re: [patch] ixgbevf: potential NULL dereference on allocation
>failure
>
>
>That's kind of crummy semantics, if any part fails we should unwind
>and return an error.  So just do the necessary memory allocations
>first, and don't make any changes unless they all succeed.
>
>This code also seems to be incredibly racy.  It allocates the new RING
>structure, and copies the existing entries over.  Meanwhile the chip
>is still running and we're potentially processing these same ring
>entries, so by the time we actually assign adapter->{rx,tx}_ring
>pointers the contents could have changed.
>

I've taken up your suggestions and implemented them (roughly) as suggested below.  After looking at the code I had to agree that it would be very confusing for a user to set new ring parameters, have the call partially succeed but get no error and then look at the parameters again and not see what he expected.  Now the code will do as suggested and just unwind all prior allocations and return an error if the new ring sizing didn't work.  The user will be left with the prior ring size allocations which is probably what he would expect.

The patch is going to be posted internally and after it goes through our review process it will be posted to netdev.

Regards and thanks for the suggestions,

- Greg

>Probably the simplest thing to do is to structure this such that the
>chip is quiesced around the entire ring set operation, so something
>like:
>
>	tx_ring = kcalloc();
>	if (!tx_ring)
>		goto do_err;
>	rx_ring = kcalloc();
>	if (!rx_ring)
>		goto rx_ring_free_err;
>
>	ixgbevf_down(adapter);
>
>	err = setup_tx_ring(adapter, tx_ring);
>	if (err)
>		goto device_up_err;
>	err = setup_rx_ring(adapter, rx_ring);
>	if (err)
>		goto device_up_err;
>
>	ixgbevf_up(adapter);
>
>	return 0;
>
>device_up_err:
>	tear_down_tx_ring(adapter, tx_ring);
>	tear_down_rx_ring(adapter, rx_ring);
>	kfree(tx_ring);
>rx_ring_free_err:
>	kfree(rx_ring);
>do_err:
>	return err;

Re: [patch] ixgbevf: potential NULL dereference on allocation failure

[David Miller]

From: "Rose, Gregory V" <gregory.v.rose@intel.com>
Date: Tue, 14 Sep 2010 09:57:46 -0700

> I've taken up your suggestions and implemented them (roughly) as
> suggested below.  After looking at the code I had to agree that it
> would be very confusing for a user to set new ring parameters, have
> the call partially succeed but get no error and then look at the
> parameters again and not see what he expected.  Now the code will do
> as suggested and just unwind all prior allocations and return an
> error if the new ring sizing didn't work.  The user will be left
> with the prior ring size allocations which is probably what he would
> expect.
>
> The patch is going to be posted internally and after it goes through
> our review process it will be posted to netdev.

Thanks for doing this work Greg.