[PATCH v3] 3c59x: Fix memory leaks in vortex_open

[PATCH v3] 3c59x: Fix memory leaks in vortex_open

[Jia-Ju Bai]

For linux-3.18.0
The driver calls __netdev_alloc_skb in vortex_open but lacks dev_kfree_skb
when vortex_up is failed, so memory leaks may occur in this situation.
This patch fixes this problem.

Signed-off-by: Jia-Ju Bai <baijiaju1990@163.com>
---
 drivers/net/ethernet/3com/3c59x.c |   10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/net/ethernet/3com/3c59x.c b/drivers/net/ethernet/3com/3c59x.c
index 41095eb..d0c5bee 100644
--- a/drivers/net/ethernet/3com/3c59x.c
+++ b/drivers/net/ethernet/3com/3c59x.c
@@ -1782,6 +1782,16 @@ vortex_open(struct net_device *dev)
 	if (!retval)
 		goto out;
 
+	if (vp->full_bus_master_rx) {
+		for (i = 0; i < RX_RING_SIZE; i++) {
+			if (vp->rx_skbuff[i]) {
+				dev_kfree_skb(vp->rx_skbuff[i]);
+				vp->rx_skbuff[i] = NULL;
+			}
+		}
+		retval = -ENOMEM;
+	}
+
 err_free_irq:
 	free_irq(dev->irq, dev);
 err:
-- 
1.7.9.5

Re: [PATCH v3] 3c59x: Fix memory leaks in vortex_open

[Neil Horman]

On Tue, Dec 23, 2014 at 10:54:50AM +0800, Jia-Ju Bai wrote:
> For linux-3.18.0
> The driver calls __netdev_alloc_skb in vortex_open but lacks dev_kfree_skb
> when vortex_up is failed, so memory leaks may occur in this situation.
> This patch fixes this problem.
> 
> Signed-off-by: Jia-Ju Bai <baijiaju1990@163.com>
> ---
>  drivers/net/ethernet/3com/3c59x.c |   10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/net/ethernet/3com/3c59x.c b/drivers/net/ethernet/3com/3c59x.c
> index 41095eb..d0c5bee 100644
> --- a/drivers/net/ethernet/3com/3c59x.c
> +++ b/drivers/net/ethernet/3com/3c59x.c
> @@ -1782,6 +1782,16 @@ vortex_open(struct net_device *dev)
>  	if (!retval)
>  		goto out;
>  
> +	if (vp->full_bus_master_rx) {
> +		for (i = 0; i < RX_RING_SIZE; i++) {
> +			if (vp->rx_skbuff[i]) {
> +				dev_kfree_skb(vp->rx_skbuff[i]);
> +				vp->rx_skbuff[i] = NULL;
> +			}
> +		}
> +		retval = -ENOMEM;
> +	}
> +
>  err_free_irq:
>  	free_irq(dev->irq, dev);
>  err:
> -- 
> 1.7.9.5
> 
> 
> 
This doesn't make sense.  We free all the skbs in vortex_open if we don't
allocate all of them (the if (i != RX_RING_SIZE) check), the only place we miss
is if vortex_up fails, and you didn't remove the if (!retval) goto out check, so
this code won't get run appropriately.

That said, it does seem we need to clean up if vortex_up fails, but it would
seem to me to be easier to just call vortex_close if it does, since that will do
all of the approriate cleanup.

Neil

Re: [PATCH v3] 3c59x: Fix memory leaks in vortex_open

[Jia-Ju Bai]

Thanks for the reply!

> This doesn't make sense.  We free all the skbs in vortex_open if we don't
> allocate all of them (the if (i != RX_RING_SIZE) check), the only place we miss
> is if vortex_up fails, and you didn't remove the if (!retval) goto out check, so
> this code won't get run appropriately.

In the code, when vortex_up is failed and does not returns 0,
"if (!retval)" is failed and "goto out" is not executed, so error 
handling code
below is executed, including my added code.
Is it right?

>
> That said, it does seem we need to clean up if vortex_up fails, but it would
> seem to me to be easier to just call vortex_close if it does, since that will do
> all of the approriate cleanup.
>
> Neil
>

In the code, vortex_close does too many releasing operations, such as free
vp->tx_skbuff, but vortex_open only allocates memory for vp->rx_skbuff 
before
vortex_up is called.
So I think it is enough to just free the memory of vp->rx_skbuff when 
vortex_up
is failed.

Re: [PATCH v3] 3c59x: Fix memory leaks in vortex_open

[Neil Horman]

On Tue, Dec 23, 2014 at 11:00:01PM +0800, Jia-Ju Bai wrote:
> Thanks for the reply!
> 
> >This doesn't make sense.  We free all the skbs in vortex_open if we don't
> >allocate all of them (the if (i != RX_RING_SIZE) check), the only place we miss
> >is if vortex_up fails, and you didn't remove the if (!retval) goto out check, so
> >this code won't get run appropriately.
> 
> In the code, when vortex_up is failed and does not returns 0,
> "if (!retval)" is failed and "goto out" is not executed, so error handling
> code
> below is executed, including my added code.
> Is it right?
> 
Yup, you're right, I missed the sense of the check.

> >
> >That said, it does seem we need to clean up if vortex_up fails, but it would
> >seem to me to be easier to just call vortex_close if it does, since that will do
> >all of the approriate cleanup.
> >
> >Neil
> >
> 
> In the code, vortex_close does too many releasing operations, such as free
> vp->tx_skbuff, but vortex_open only allocates memory for vp->rx_skbuff
> before
> vortex_up is called.
> So I think it is enough to just free the memory of vp->rx_skbuff when
> vortex_up
> is failed.
> 
No, I don't think so.  vortex_close predicates each free with a NULL check, so
if its not been allocated, it shouldn't be freed.  vortex_close also puts the
adapter back into a known state (undoing all the setup that vortex_open does).
I really think its better to go with the proper close path than just unwinding
the allocation

Neil

> 

Re: [PATCH v3] 3c59x: Fix memory leaks in vortex_open

[Jia-Ju Bai]

On 12/23/2014 11:43 PM, Neil Horman wrote:
> No, I don't think so.  vortex_close predicates each free with a NULL check, so
> if its not been allocated, it shouldn't be freed.  vortex_close also puts the
> adapter back into a known state (undoing all the setup that vortex_open does).
> I really think its better to go with the proper close path than just unwinding
> the allocation
>
> Neil
>

Firstly, I run my match on the real hardware(3com 3c905B 100Base
PCI Ethernet Controller) and make vortex_up failed on purpose
(make "pci_enable_device" in vortex_up failed). During runtime, the driver
works well and memory leaks are fixed.

Secondly, I revise the code according to your opinion:

         retval = vortex_up(dev);
         if (!retval)
             goto out;

+      vortex_close(dev);
+      return -ENOMEM;

Then I repeat my experiment, but system hang occurs!

After adding some "printk"s into the code and running the driver, I find
the problem's source:
vortex_close calls vortex_down in runtime, and vortex_down calls
"del_timer_sync(&vp->rx_oom_timer);" in the code. However, I make
"pci_enable_device" failed in vortext_up to let vortex_up return an
error code directly, but "vp->rx_oom_timer" is initialized only by
"init_timer" after "pci_enable_device". Thus when
"del_timer_sync(&vp->rx_oom_timer);" is called in vortex_down,
a null dereference may occur.
Moreover, only "pci_enable_device" can make vortex_up failed.

Re: [PATCH v3] 3c59x: Fix memory leaks in vortex_open

[Neil Horman]

On Wed, Dec 24, 2014 at 10:12:58AM +0800, Jia-Ju Bai wrote:
> On 12/23/2014 11:43 PM, Neil Horman wrote:
> >No, I don't think so.  vortex_close predicates each free with a NULL check, so
> >if its not been allocated, it shouldn't be freed.  vortex_close also puts the
> >adapter back into a known state (undoing all the setup that vortex_open does).
> >I really think its better to go with the proper close path than just unwinding
> >the allocation
> >
> >Neil
> >
> 
> Firstly, I run my match on the real hardware(3com 3c905B 100Base
> PCI Ethernet Controller) and make vortex_up failed on purpose
> (make "pci_enable_device" in vortex_up failed). During runtime, the driver
> works well and memory leaks are fixed.
> 
> Secondly, I revise the code according to your opinion:
> 
>         retval = vortex_up(dev);
>         if (!retval)
>             goto out;
> 
> +      vortex_close(dev);
> +      return -ENOMEM;
> 
> Then I repeat my experiment, but system hang occurs!
> 
> After adding some "printk"s into the code and running the driver, I find
> the problem's source:
> vortex_close calls vortex_down in runtime, and vortex_down calls
> "del_timer_sync(&vp->rx_oom_timer);" in the code. However, I make
> "pci_enable_device" failed in vortext_up to let vortex_up return an
> error code directly, but "vp->rx_oom_timer" is initialized only by
> "init_timer" after "pci_enable_device". Thus when
> "del_timer_sync(&vp->rx_oom_timer);" is called in vortex_down,
> a null dereference may occur.
> Moreover, only "pci_enable_device" can make vortex_up failed.
> 
> 
Sooo, fix it.  Add some checks to not delete the timer if its not been
initalized.  Its really preferable to have a single teardown path and a single
bringup path if at all possible
Neil

Re: [PATCH v3] 3c59x: Fix memory leaks in vortex_open

[David Miller]

From: Neil Horman <nhorman@tuxdriver.com>
Date: Tue, 23 Dec 2014 22:27:28 -0500

> Sooo, fix it.  Add some checks to not delete the timer if its not been
> initalized.  Its really preferable to have a single teardown path and a single
> bringup path if at all possible

Or simply have vortex_up() initialize the two timers before it does
anything else.