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

[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;

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

[Cong Wang]

On Fri, 2013-03-22 at 15:50 +0800, Cong Wang wrote:
> +		iph = (struct iphdr *)skb->data;
> +		iph->id = id++;

Oops, seems I missed its endian...

[PATCH net-next 1/2] 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

---
 net/ipv4/gre.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/net/ipv4/gre.c b/net/ipv4/gre.c
index 7a4c710..e20631c 100644
--- a/net/ipv4/gre.c
+++ b/net/ipv4/gre.c
@@ -125,8 +125,9 @@ 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;
+	int tnl_hlen, id;
 	bool csum;
 
 	if (unlikely(skb_shinfo(skb)->gso_type &
@@ -170,6 +171,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 = ip_hdr(skb);
+	id = ntohs(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 +182,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 = htons(id++);
 		__skb_push(skb, ghl);
 		if (csum) {
 			__be32 *pcsum;
-- 
1.7.7.6

[PATCH net-next 2/2] udp: increase inner ip header ID during segmentation

[Cong Wang]

From: Cong Wang <amwang@redhat.com>

Similar to GRE tunnel, UDP tunnel should take care of IP header ID
too.

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>
---
 net/ipv4/udp.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index 7117d14..3c362ee 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -2304,7 +2304,8 @@ static struct sk_buff *skb_udp_tunnel_segment(struct sk_buff *skb,
 	struct sk_buff *segs = ERR_PTR(-EINVAL);
 	int mac_len = skb->mac_len;
 	int tnl_hlen = skb_inner_mac_header(skb) - skb_transport_header(skb);
-	int outer_hlen;
+	int outer_hlen, id;
+	struct iphdr *iph;
 	netdev_features_t enc_features;
 
 	if (unlikely(!pskb_may_pull(skb, tnl_hlen)))
@@ -2315,6 +2316,8 @@ static struct sk_buff *skb_udp_tunnel_segment(struct sk_buff *skb,
 	skb_reset_mac_header(skb);
 	skb_set_network_header(skb, skb_inner_network_offset(skb));
 	skb->mac_len = skb_inner_network_offset(skb);
+	iph = ip_hdr(skb);
+	id = ntohs(iph->id);
 
 	/* segment inner packet. */
 	enc_features = skb->dev->hw_enc_features & netif_skb_features(skb);
@@ -2329,6 +2332,8 @@ static struct sk_buff *skb_udp_tunnel_segment(struct sk_buff *skb,
 		int udp_offset = outer_hlen - tnl_hlen;
 
 		skb->mac_len = mac_len;
+		iph = (struct iphdr *)skb_inner_network_header(skb);
+		iph->id = htons(id++);
 
 		skb_push(skb, outer_hlen);
 		skb_reset_mac_header(skb);
-- 
1.7.7.6

Re: [PATCH net-next 1/2] ip_gre: increase inner ip header ID during segmentation

[David Miller]

From: Cong Wang <amwang@redhat.com>
Date: Fri, 22 Mar 2013 18:31:31 +0800

> 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>

Applied.

Re: [PATCH net-next 2/2] udp: increase inner ip header ID during segmentation

[David Miller]

From: Cong Wang <amwang@redhat.com>
Date: Fri, 22 Mar 2013 18:31:32 +0800

> From: Cong Wang <amwang@redhat.com>
> 
> Similar to GRE tunnel, UDP tunnel should take care of IP header ID
> too.
> 
> 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>

Applied.

Re: [PATCH net-next 1/2] ip_gre: increase inner ip header ID during segmentation

[Pravin Shelar]

On Fri, Mar 22, 2013 at 3:31 AM, Cong Wang <amwang@redhat.com> wrote:
> 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.
>

Outer IP header ids are incremented in inet_gso_segment for GRE. So it
is already done. In any case I don't think we should increment
IP-Identification in GRE handler, This is layering violation.
This breaks GRE with IPV6 payload.

> 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
>
> ---
>  net/ipv4/gre.c |    7 ++++++-
>  1 files changed, 6 insertions(+), 1 deletions(-)
>
> diff --git a/net/ipv4/gre.c b/net/ipv4/gre.c
> index 7a4c710..e20631c 100644
> --- a/net/ipv4/gre.c
> +++ b/net/ipv4/gre.c
> @@ -125,8 +125,9 @@ 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;
> +       int tnl_hlen, id;
>         bool csum;
>
>         if (unlikely(skb_shinfo(skb)->gso_type &
> @@ -170,6 +171,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 = ip_hdr(skb);
> +       id = ntohs(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 +182,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 = htons(id++);
>                 __skb_push(skb, ghl);
>                 if (csum) {
>                         __be32 *pcsum;
> --
> 1.7.7.6
>

Re: [PATCH net-next 1/2] ip_gre: increase inner ip header ID during segmentation

[Cong Wang]

On Fri, 2013-03-22 at 10:12 -0700, Pravin Shelar wrote:
> On Fri, Mar 22, 2013 at 3:31 AM, Cong Wang <amwang@redhat.com> wrote:
> > 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.
> >
> 
> Outer IP header ids are incremented in inet_gso_segment for GRE. So it
> is already done. In any case I don't think we should increment
> IP-Identification in GRE handler, This is layering violation.
> This breaks GRE with IPV6 payload.

I knew, $subject mentions this increases the *inner* IP header ID, not
outer one.

But you are probably right that we should check for IPv4, I will send
patch to fix it.

Thanks.

[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] 8021q: fix a potential use-after-free

[Eric Dumazet]

On Sat, 2013-03-23 at 13:14 +0800, Cong Wang wrote:
> 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(-)

Acked-by: Eric Dumazet <edumazet@google.com>

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] 8021q: fix a potential use-after-free

[David Miller]

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

> 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>

Applied.

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.

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] 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