[PATCH net-next] 802: fix a possible race condition

[PATCH net-next] 802: fix a possible race condition

[Cong Wang]

From: Cong Wang <amwang@redhat.com>

(Resend with a better changelog)

garp_pdu_queue() should ways be called with this spin lock.
garp_uninit_applicant() only holds rtnl lock which is not
enough here.  A possible race can happen as garp_pdu_rcv()
is called in BH context:

	garp_pdu_rcv()
	  |->garp_pdu_parse_msg()
	    |->garp_pdu_parse_attr()
	      |-> garp_gid_event()

Found by code inspection.

Cc: Eric Dumazet <eric.dumazet@gmail.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: David Ward <david.ward@ll.mit.edu>
Cc: "Jorge Boncompte [DTI2]" <jorge@dti2.net>
Signed-off-by: Cong Wang <amwang@redhat.com>
---
 net/802/garp.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/net/802/garp.c b/net/802/garp.c
index 8456f5d..5d9630a 100644
--- a/net/802/garp.c
+++ b/net/802/garp.c
@@ -609,8 +609,12 @@ void garp_uninit_applicant(struct net_device *dev, struct garp_application *appl
 	/* Delete timer and generate a final TRANSMIT_PDU event to flush out
 	 * all pending messages before the applicant is gone. */
 	del_timer_sync(&app->join_timer);
+
+	spin_lock_bh(&app->lock);
 	garp_gid_event(app, GARP_EVENT_TRANSMIT_PDU);
 	garp_pdu_queue(app);
+	spin_unlock_bh(&app->lock);
+
 	garp_queue_xmit(app);
 
 	dev_mc_del(dev, appl->proto.group_address);
-- 
1.7.7.6

Re: [PATCH net-next] 802: fix a possible race condition

[David Miller]

From: Cong Wang <amwang@redhat.com>
Date: Wed,  3 Apr 2013 15:52:40 +0800

> From: Cong Wang <amwang@redhat.com>
> 
> (Resend with a better changelog)
> 
> garp_pdu_queue() should ways be called with this spin lock.
> garp_uninit_applicant() only holds rtnl lock which is not
> enough here.  A possible race can happen as garp_pdu_rcv()
> is called in BH context:
> 
> 	garp_pdu_rcv()
> 	  |->garp_pdu_parse_msg()
> 	    |->garp_pdu_parse_attr()
> 	      |-> garp_gid_event()
> 
> Found by code inspection.
> 
> Cc: Eric Dumazet <eric.dumazet@gmail.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: David Ward <david.ward@ll.mit.edu>
> Cc: "Jorge Boncompte [DTI2]" <jorge@dti2.net>
> Signed-off-by: Cong Wang <amwang@redhat.com>

Applied.

[Patch net-next] ip_gre: increase inner ip header ID during segmentation

[Cong Wang]

From: Cong Wang <amwang@redhat.com>

According to the previous discussion [1] on netdev list, DaveM insists
we should increase the IP header ID for each segmented packets.
This patch fixes it.

Cc: Pravin B Shelar <pshelar@nicira.com>
Cc: Eric Dumazet <edumazet@google.com>
Cc: "David S. Miller" <davem@davemloft.net>
Signed-off-by: Cong Wang <amwang@redhat.com>

1. http://marc.info/?t=136384172700001&r=1&w=2

---
diff --git a/net/ipv4/gre.c b/net/ipv4/gre.c
index 7a4c710..3cf20a4 100644
--- a/net/ipv4/gre.c
+++ b/net/ipv4/gre.c
@@ -125,9 +125,11 @@ static struct sk_buff *gre_gso_segment(struct sk_buff *skb,
 	netdev_features_t enc_features;
 	int ghl = GRE_HEADER_SECTION;
 	struct gre_base_hdr *greh;
+	struct iphdr *iph;
 	int mac_len = skb->mac_len;
 	int tnl_hlen;
 	bool csum;
+	__be16 id;
 
 	if (unlikely(skb_shinfo(skb)->gso_type &
 				~(SKB_GSO_TCPV4 |
@@ -170,6 +172,8 @@ static struct sk_buff *gre_gso_segment(struct sk_buff *skb,
 	skb_set_network_header(skb, skb_inner_network_offset(skb));
 	skb->mac_len = skb_inner_network_offset(skb);
 
+	iph = (struct iphdr *)skb->data;
+	id = iph->id;
 	/* segment inner packet. */
 	enc_features = skb->dev->hw_enc_features & netif_skb_features(skb);
 	segs = skb_mac_gso_segment(skb, enc_features);
@@ -179,6 +183,8 @@ static struct sk_buff *gre_gso_segment(struct sk_buff *skb,
 	skb = segs;
 	tnl_hlen = skb_tnl_header_len(skb);
 	do {
+		iph = (struct iphdr *)skb->data;
+		iph->id = id++;
 		__skb_push(skb, ghl);
 		if (csum) {
 			__be32 *pcsum;

[PATCH net-next] 8021q: fix a potential use-after-free

[Cong Wang]

From: Cong Wang <amwang@redhat.com>

vlan_vid_del() could possibly free ->vlan_info after a RCU grace
period, however, we may still refer to the freed memory area
by 'grp' pointer. Found by code inspection.

This patch moves vlan_vid_del() as behind as possible.

Cc: Patrick McHardy <kaber@trash.net>
Cc: "David S. Miller" <davem@davemloft.net>
Signed-off-by: Cong Wang <amwang@redhat.com>
---
 net/8021q/vlan.c |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/net/8021q/vlan.c b/net/8021q/vlan.c
index a187144..85addcd 100644
--- a/net/8021q/vlan.c
+++ b/net/8021q/vlan.c
@@ -86,13 +86,6 @@ void unregister_vlan_dev(struct net_device *dev, struct list_head *head)
 
 	grp = &vlan_info->grp;
 
-	/* Take it out of our own structures, but be sure to interlock with
-	 * HW accelerating devices or SW vlan input packet processing if
-	 * VLAN is not 0 (leave it there for 802.1p).
-	 */
-	if (vlan_id)
-		vlan_vid_del(real_dev, vlan_id);
-
 	grp->nr_vlan_devs--;
 
 	if (vlan->flags & VLAN_FLAG_MVRP)
@@ -114,6 +107,13 @@ void unregister_vlan_dev(struct net_device *dev, struct list_head *head)
 		vlan_gvrp_uninit_applicant(real_dev);
 	}
 
+	/* Take it out of our own structures, but be sure to interlock with
+	 * HW accelerating devices or SW vlan input packet processing if
+	 * VLAN is not 0 (leave it there for 802.1p).
+	 */
+	if (vlan_id)
+		vlan_vid_del(real_dev, vlan_id);
+
 	/* Get rid of the vlan's reference to real_dev */
 	dev_put(real_dev);
 }
-- 
1.7.7.6

[PATCH net-next] 802: fix a possible race condition

[Cong Wang]

From: Cong Wang <amwang@redhat.com>

garp_pdu_queue() should ways be called with this spin lock.
garp_uninit_applicant() only holds rtnl lock which is not
enough here.

Found by code inspection.

Cc: "David S. Miller" <davem@davemloft.net>
Cc: David Ward <david.ward@ll.mit.edu>
Cc: "Jorge Boncompte [DTI2]" <jorge@dti2.net>
Signed-off-by: Cong Wang <amwang@redhat.com>
---
 net/802/garp.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/net/802/garp.c b/net/802/garp.c
index 8456f5d..5d9630a 100644
--- a/net/802/garp.c
+++ b/net/802/garp.c
@@ -609,8 +609,12 @@ void garp_uninit_applicant(struct net_device *dev, struct garp_application *appl
 	/* Delete timer and generate a final TRANSMIT_PDU event to flush out
 	 * all pending messages before the applicant is gone. */
 	del_timer_sync(&app->join_timer);
+
+	spin_lock_bh(&app->lock);
 	garp_gid_event(app, GARP_EVENT_TRANSMIT_PDU);
 	garp_pdu_queue(app);
+	spin_unlock_bh(&app->lock);
+
 	garp_queue_xmit(app);
 
 	dev_mc_del(dev, appl->proto.group_address);
-- 
1.7.7.6

Re: [PATCH net-next] 802: fix a possible race condition

[David Miller]

From: Cong Wang <amwang@redhat.com>
Date: Sat, 23 Mar 2013 13:14:08 +0800

> From: Cong Wang <amwang@redhat.com>
> 
> garp_pdu_queue() should ways be called with this spin lock.
> garp_uninit_applicant() only holds rtnl lock which is not
> enough here.
> 
> Found by code inspection.
> 
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: David Ward <david.ward@ll.mit.edu>
> Cc: "Jorge Boncompte [DTI2]" <jorge@dti2.net>
> Signed-off-by: Cong Wang <amwang@redhat.com>

Under what conditions can entries be removed or added to
these RB-trees without the RTNL being held?

If such events cannot happen, then no locking is needed.

Even if your change is correct and necessary, the answer to my
needs to be added to your commit message.

Thanks.

Re: [PATCH net-next] 802: fix a possible race condition

[Cong Wang]

On Sun, 2013-03-24 at 17:24 -0400, David Miller wrote:
> From: Cong Wang <amwang@redhat.com>
> Date: Sat, 23 Mar 2013 13:14:08 +0800
> 
> > From: Cong Wang <amwang@redhat.com>
> > 
> > garp_pdu_queue() should ways be called with this spin lock.
> > garp_uninit_applicant() only holds rtnl lock which is not
> > enough here.
> > 
> > Found by code inspection.
> > 
> > Cc: "David S. Miller" <davem@davemloft.net>
> > Cc: David Ward <david.ward@ll.mit.edu>
> > Cc: "Jorge Boncompte [DTI2]" <jorge@dti2.net>
> > Signed-off-by: Cong Wang <amwang@redhat.com>
> 
> Under what conditions can entries be removed or added to
> these RB-trees without the RTNL being held?

At least garp_join_timer() calls garp_pdu_queue() in a timer:

static void garp_join_timer(unsigned long data)
{
        struct garp_applicant *app = (struct garp_applicant *)data;

        spin_lock(&app->lock);
        garp_gid_event(app, GARP_EVENT_TRANSMIT_PDU);
        garp_pdu_queue(app);
        spin_unlock(&app->lock);

        garp_queue_xmit(app);
        garp_join_timer_arm(app);
}

which I don't think can hold RTNL lock possibly.

> 
> If such events cannot happen, then no locking is needed.
> 
> Even if your change is correct and necessary, the answer to my
> needs to be added to your commit message.

Ok, I will.

Thanks.

Re: [PATCH net-next] 802: fix a possible race condition

[Eric Dumazet]

On Mon, 2013-03-25 at 21:32 +0800, Cong Wang wrote:

> At least garp_join_timer() calls garp_pdu_queue() in a timer:
> 
> static void garp_join_timer(unsigned long data)
> {
>         struct garp_applicant *app = (struct garp_applicant *)data;
> 
>         spin_lock(&app->lock);
>         garp_gid_event(app, GARP_EVENT_TRANSMIT_PDU);
>         garp_pdu_queue(app);
>         spin_unlock(&app->lock);
> 
>         garp_queue_xmit(app);
>         garp_join_timer_arm(app);
> }
> 
> which I don't think can hold RTNL lock possibly.
> 

But timer wont possibly run because of the previous :

del_timer_sync(&app->join_timer);

Re: [PATCH net-next] 802: fix a possible race condition

[Cong Wang]

On Mon, 2013-03-25 at 07:08 -0700, Eric Dumazet wrote:
> On Mon, 2013-03-25 at 21:32 +0800, Cong Wang wrote:
> 
> > At least garp_join_timer() calls garp_pdu_queue() in a timer:
> > 
> > static void garp_join_timer(unsigned long data)
> > {
> >         struct garp_applicant *app = (struct garp_applicant *)data;
> > 
> >         spin_lock(&app->lock);
> >         garp_gid_event(app, GARP_EVENT_TRANSMIT_PDU);
> >         garp_pdu_queue(app);
> >         spin_unlock(&app->lock);
> > 
> >         garp_queue_xmit(app);
> >         garp_join_timer_arm(app);
> > }
> > 
> > which I don't think can hold RTNL lock possibly.
> > 
> 
> But timer wont possibly run because of the previous :
> 
> del_timer_sync(&app->join_timer);

Yeah, but in the following callchain:

garp_pdu_rcv() -> garp_pdu_parse_msg() -> garp_pdu_parse_attr() ->
garp_gid_event()

the race can happen too as garp_pdu_rcv() is called in BH context.