[PATCH v2.6.33 1/2] jme: Fix VLAN memory leak

[PATCH v2.6.33 1/2] jme: Fix VLAN memory leak

[cooldavid]

From: Guo-Fu Tseng <cooldavid@cooldavid.org>

Fix memory leak while receiving 8021q tagged packet which is not
registered by user.

Signed-off-by: Guo-Fu Tseng <cooldavid@cooldavid.org>
---
 drivers/net/jme.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/net/jme.c b/drivers/net/jme.c
index 792b88f..3da390a 100644
--- a/drivers/net/jme.c
+++ b/drivers/net/jme.c
@@ -946,6 +946,8 @@ jme_alloc_and_feed_skb(struct jme_adapter *jme, int idx)
 				jme->jme_vlan_rx(skb, jme->vlgrp,
 					le16_to_cpu(rxdesc->descwb.vlan));
 				NET_STAT(jme).rx_bytes += 4;
+			} else {
+				dev_kfree_skb(skb);
 			}
 		} else {
 			jme->jme_rx(skb);
-- 
1.6.4.4

[PATCH v2.6.33 2/2] jme: Adding lock to protect vlgrp structure.

[cooldavid]

From: Guo-Fu Tseng <cooldavid@cooldavid.org>

Adding a lock to prevent modifying the vlgrp structure while receiving
VLAN packet.

Signed-off-by: Guo-Fu Tseng <cooldavid@cooldavid.org>
---
 drivers/net/jme.c |    6 ++++++
 drivers/net/jme.h |    1 +
 2 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/drivers/net/jme.c b/drivers/net/jme.c
index 3da390a..f10d9db 100644
--- a/drivers/net/jme.c
+++ b/drivers/net/jme.c
@@ -942,11 +942,14 @@ jme_alloc_and_feed_skb(struct jme_adapter *jme, int idx)
 			skb->ip_summed = CHECKSUM_NONE;
 
 		if (rxdesc->descwb.flags & cpu_to_le16(RXWBFLAG_TAGON)) {
+			spin_lock(&jme->vlgrp_lock);
 			if (jme->vlgrp) {
 				jme->jme_vlan_rx(skb, jme->vlgrp,
 					le16_to_cpu(rxdesc->descwb.vlan));
+				spin_unlock(&jme->vlgrp_lock);
 				NET_STAT(jme).rx_bytes += 4;
 			} else {
+				spin_unlock(&jme->vlgrp_lock);
 				dev_kfree_skb(skb);
 			}
 		} else {
@@ -2092,7 +2095,9 @@ jme_vlan_rx_register(struct net_device *netdev, struct vlan_group *grp)
 {
 	struct jme_adapter *jme = netdev_priv(netdev);
 
+	spin_lock_bh(&jme->vlgrp_lock);
 	jme->vlgrp = grp;
+	spin_unlock_bh(&jme->vlgrp_lock);
 }
 
 static void
@@ -2759,6 +2764,7 @@ jme_init_one(struct pci_dev *pdev,
 	spin_lock_init(&jme->phy_lock);
 	spin_lock_init(&jme->macaddr_lock);
 	spin_lock_init(&jme->rxmcs_lock);
+	spin_lock_init(&jme->vlgrp_lock);
 
 	atomic_set(&jme->link_changing, 1);
 	atomic_set(&jme->rx_cleaning, 1);
diff --git a/drivers/net/jme.h b/drivers/net/jme.h
index 251abed..fbde5c5 100644
--- a/drivers/net/jme.h
+++ b/drivers/net/jme.h
@@ -420,6 +420,7 @@ struct jme_adapter {
 	spinlock_t		phy_lock;
 	spinlock_t		macaddr_lock;
 	spinlock_t		rxmcs_lock;
+	spinlock_t		vlgrp_lock;
 	struct tasklet_struct	rxempty_task;
 	struct tasklet_struct	rxclean_task;
 	struct tasklet_struct	txclean_task;
-- 
1.6.4.4

Re: [PATCH v2.6.33 2/2] jme: Adding lock to protect vlgrp structure.

[Laurent Chavey]

what does the spinlock protect ?


On Sun, Mar 14, 2010 at 10:15 PM,  <cooldavid@cooldavid.org> wrote:
> From: Guo-Fu Tseng <cooldavid@cooldavid.org>
>
> Adding a lock to prevent modifying the vlgrp structure while receiving
> VLAN packet.
>
> Signed-off-by: Guo-Fu Tseng <cooldavid@cooldavid.org>
> ---
>  drivers/net/jme.c |    6 ++++++
>  drivers/net/jme.h |    1 +
>  2 files changed, 7 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/net/jme.c b/drivers/net/jme.c
> index 3da390a..f10d9db 100644
> --- a/drivers/net/jme.c
> +++ b/drivers/net/jme.c
> @@ -942,11 +942,14 @@ jme_alloc_and_feed_skb(struct jme_adapter *jme, int idx)
>                        skb->ip_summed = CHECKSUM_NONE;
>
>                if (rxdesc->descwb.flags & cpu_to_le16(RXWBFLAG_TAGON)) {
> +                       spin_lock(&jme->vlgrp_lock);
>                        if (jme->vlgrp) {
>                                jme->jme_vlan_rx(skb, jme->vlgrp,
>                                        le16_to_cpu(rxdesc->descwb.vlan));
> +                               spin_unlock(&jme->vlgrp_lock);
>                                NET_STAT(jme).rx_bytes += 4;
>                        } else {
> +                               spin_unlock(&jme->vlgrp_lock);
>                                dev_kfree_skb(skb);
>                        }
>                } else {
> @@ -2092,7 +2095,9 @@ jme_vlan_rx_register(struct net_device *netdev, struct vlan_group *grp)
>  {
>        struct jme_adapter *jme = netdev_priv(netdev);
>
> +       spin_lock_bh(&jme->vlgrp_lock);
>        jme->vlgrp = grp;
> +       spin_unlock_bh(&jme->vlgrp_lock);
>  }
>
>  static void
> @@ -2759,6 +2764,7 @@ jme_init_one(struct pci_dev *pdev,
>        spin_lock_init(&jme->phy_lock);
>        spin_lock_init(&jme->macaddr_lock);
>        spin_lock_init(&jme->rxmcs_lock);
> +       spin_lock_init(&jme->vlgrp_lock);
>
>        atomic_set(&jme->link_changing, 1);
>        atomic_set(&jme->rx_cleaning, 1);
> diff --git a/drivers/net/jme.h b/drivers/net/jme.h
> index 251abed..fbde5c5 100644
> --- a/drivers/net/jme.h
> +++ b/drivers/net/jme.h
> @@ -420,6 +420,7 @@ struct jme_adapter {
>        spinlock_t              phy_lock;
>        spinlock_t              macaddr_lock;
>        spinlock_t              rxmcs_lock;
> +       spinlock_t              vlgrp_lock;
>        struct tasklet_struct   rxempty_task;
>        struct tasklet_struct   rxclean_task;
>        struct tasklet_struct   txclean_task;
> --
> 1.6.4.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH v2.6.33 2/2] jme: Adding lock to protect vlgrp structure.

[Guo-Fu Tseng]

Hi Laurent:

The vlan_rx_register is called through ioctl.
And the packet feeding is called in the tasklet.
I see no lock in register_vlan_dev(), register_vlan_device(), and vlan_ioctl_handler()
which is related to the vlan_hwaccel_receive_skb(), vlan_hwaccel_rx().

It prevents the vlgrp pointer be modified while trying to feed the packet.
Ex:
> if (jme->vlgrp) {
True.
vlan_rx_register called, vlgrp set to NULL.
>         jme->jme_vlan_rx(skb, jme->vlgrp,
>                          le16_to_cpu(rxdesc->descwb.vlan));
passing vlgrp with the value "NULL" to jme_vlan_rx.
Or it might even be modified during the vlan_hwaccel_{receive_skb|rx} call.

Please correct me if I'm wrong.

On Mon, 15 Mar 2010 11:22:48 -0700, Laurent Chavey wrote
> what does the spinlock protect ?
> 

--
Guo-Fu Tseng

Re: [PATCH v2.6.33 2/2] jme: Adding lock to protect vlgrp structure.

[David Miller]

From: "Guo-Fu Tseng" <cooldavid@cooldavid.org>
Date: Tue, 16 Mar 2010 03:13:33 +0800

> The vlan_rx_register is called through ioctl.
> And the packet feeding is called in the tasklet.
> I see no lock in register_vlan_dev(), register_vlan_device(), and vlan_ioctl_handler()
> which is related to the vlan_hwaccel_receive_skb(), vlan_hwaccel_rx().
> 
> It prevents the vlgrp pointer be modified while trying to feed the packet.

This is not how you fix this.  Adding a new lock to your hot code
path of packet receiving is the last thing you should be doing.

Instead, do what other drivers do, take down the device and bring it
back up again when changing the ->vlgrp pointer.

See drivers/net/tg3.c:tg3_vlan_rx_register() for an example.

Re: [PATCH v2.6.33 2/2] jme: Adding lock to protect vlgrp structure.

[Guo-Fu Tseng]

On Mon, 15 Mar 2010 15:53:50 -0700 (PDT), David Miller wrote
> From: "Guo-Fu Tseng" <cooldavid@cooldavid.org>
> Date: Tue, 16 Mar 2010 03:13:33 +0800
> 
> > The vlan_rx_register is called through ioctl.
> > And the packet feeding is called in the tasklet.
> > I see no lock in register_vlan_dev(), register_vlan_device(), and vlan_ioctl_handler()
> > which is related to the vlan_hwaccel_receive_skb(), vlan_hwaccel_rx().
> > 
> > It prevents the vlgrp pointer be modified while trying to feed the packet.
> 
> This is not how you fix this.  Adding a new lock to your hot code
> path of packet receiving is the last thing you should be doing.
> 
> Instead, do what other drivers do, take down the device and bring it
> back up again when changing the ->vlgrp pointer.
> 
> See drivers/net/tg3.c:tg3_vlan_rx_register() for an example.
I see. I'll work on it.

--
Guo-Fu Tseng

Re: [PATCH v2.6.33 2/2] jme: Adding lock to protect vlgrp structure.

[Eric Dumazet]

Le mardi 16 mars 2010 à 15:15 +0800, Guo-Fu Tseng a écrit :
> On Mon, 15 Mar 2010 15:53:50 -0700 (PDT), David Miller wrote
> > From: "Guo-Fu Tseng" <cooldavid@cooldavid.org>
> > Date: Tue, 16 Mar 2010 03:13:33 +0800
> > 
> > > The vlan_rx_register is called through ioctl.
> > > And the packet feeding is called in the tasklet.
> > > I see no lock in register_vlan_dev(), register_vlan_device(), and vlan_ioctl_handler()
> > > which is related to the vlan_hwaccel_receive_skb(), vlan_hwaccel_rx().
> > > 
> > > It prevents the vlgrp pointer be modified while trying to feed the packet.
> > 
> > This is not how you fix this.  Adding a new lock to your hot code
> > path of packet receiving is the last thing you should be doing.
> > 
> > Instead, do what other drivers do, take down the device and bring it
> > back up again when changing the ->vlgrp pointer.
> > 
> > See drivers/net/tg3.c:tg3_vlan_rx_register() for an example.
> I see. I'll work on it.

Then if driver is NAPI enabled, another possibility would be to rely on
RCU, since vgrp are already freed after a RCU grace period.

Completely untested patch, for ease of discussion

diff --git a/drivers/net/jme.c b/drivers/net/jme.c
index 0f31497..e19de8d 100644
--- a/drivers/net/jme.c
+++ b/drivers/net/jme.c
@@ -942,8 +942,10 @@ jme_alloc_and_feed_skb(struct jme_adapter *jme, int idx)
 			skb->ip_summed = CHECKSUM_NONE;
 
 		if (rxdesc->descwb.flags & cpu_to_le16(RXWBFLAG_TAGON)) {
-			if (jme->vlgrp) {
-				jme->jme_vlan_rx(skb, jme->vlgrp,
+			struct vlan_group *grp = rcu_dereference(jme->vlgrp);
+
+			if (grp) {
+				jme->jme_vlan_rx(skb, grp,
 					le16_to_cpu(rxdesc->descwb.vlan));
 				NET_STAT(jme).rx_bytes += 4;
 			}