[PATCH net] ipv6: skip __ipv6_select_ident when there is no skb_dst

[PATCH net] ipv6: skip __ipv6_select_ident when there is no skb_dst

[Sabrina Dubroca]

Matt Grant reported frequent crashes in ipv6_select_ident when
udp6_ufo_fragment is called from openvswitch on a skb that doesn't
have a dst_entry set.

Skip __ipv6_select_ident in case of a NULL rt.

Fixes: 0508c07f5e0c ("ipv6: Select fragment id during UFO segmentation if not set.")
Cc: Vladislav Yasevich <vyasevic@redhat.com>
Reported-by: Matt Grant <matt@mattgrant.net.nz>
Tested-by: Matt Grant <matt@mattgrant.net.nz>
Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
---
David, can you queue it for stable (3.19)?

 net/ipv6/output_core.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/net/ipv6/output_core.c b/net/ipv6/output_core.c
index 74581f706c4d..52b1bc76d5c5 100644
--- a/net/ipv6/output_core.c
+++ b/net/ipv6/output_core.c
@@ -62,12 +62,14 @@ EXPORT_SYMBOL_GPL(ipv6_proxy_select_ident);
 void ipv6_select_ident(struct frag_hdr *fhdr, struct rt6_info *rt)
 {
 	static u32 ip6_idents_hashrnd __read_mostly;
-	u32 id;
+	u32 id = 0;
 
 	net_get_random_once(&ip6_idents_hashrnd, sizeof(ip6_idents_hashrnd));
 
-	id = __ipv6_select_ident(ip6_idents_hashrnd, &rt->rt6i_dst.addr,
-				 &rt->rt6i_src.addr);
+	if (rt)
+		id = __ipv6_select_ident(ip6_idents_hashrnd, &rt->rt6i_dst.addr,
+					 &rt->rt6i_src.addr);
+
 	fhdr->identification = htonl(id);
 }
 EXPORT_SYMBOL(ipv6_select_ident);
-- 
2.3.3

Re: [PATCH net] ipv6: skip __ipv6_select_ident when there is no skb_dst

[Vlad Yasevich]

On 03/18/2015 09:52 AM, Sabrina Dubroca wrote:
> Matt Grant reported frequent crashes in ipv6_select_ident when
> udp6_ufo_fragment is called from openvswitch on a skb that doesn't
> have a dst_entry set.
> 
> Skip __ipv6_select_ident in case of a NULL rt.
> 
> Fixes: 0508c07f5e0c ("ipv6: Select fragment id during UFO segmentation if not set.")
> Cc: Vladislav Yasevich <vyasevic@redhat.com>
> Reported-by: Matt Grant <matt@mattgrant.net.nz>
> Tested-by: Matt Grant <matt@mattgrant.net.nz>
> Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
> ---
> David, can you queue it for stable (3.19)?
> 
>  net/ipv6/output_core.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/net/ipv6/output_core.c b/net/ipv6/output_core.c
> index 74581f706c4d..52b1bc76d5c5 100644
> --- a/net/ipv6/output_core.c
> +++ b/net/ipv6/output_core.c
> @@ -62,12 +62,14 @@ EXPORT_SYMBOL_GPL(ipv6_proxy_select_ident);
>  void ipv6_select_ident(struct frag_hdr *fhdr, struct rt6_info *rt)
>  {
>  	static u32 ip6_idents_hashrnd __read_mostly;
> -	u32 id;
> +	u32 id = 0;
>  
>  	net_get_random_once(&ip6_idents_hashrnd, sizeof(ip6_idents_hashrnd));
>  
> -	id = __ipv6_select_ident(ip6_idents_hashrnd, &rt->rt6i_dst.addr,
> -				 &rt->rt6i_src.addr);
> +	if (rt)
> +		id = __ipv6_select_ident(ip6_idents_hashrnd, &rt->rt6i_dst.addr,
> +					 &rt->rt6i_src.addr);
> +
>  	fhdr->identification = htonl(id);
>  }
>  EXPORT_SYMBOL(ipv6_select_ident);
> 

Hi Sabrina

This would result in us using id 0 which is not what we want to do.

In this case, udp6_ufo_fragment() should be calling ipv6_proxy_select_ident() so that
the fragment id is properly generated.

-vlad

Re: [PATCH net] ipv6: skip __ipv6_select_ident when there is no skb_dst

[Sabrina Dubroca]

2015-03-18, 10:04:10 -0400, Vlad Yasevich wrote:
> On 03/18/2015 09:52 AM, Sabrina Dubroca wrote:
> > Matt Grant reported frequent crashes in ipv6_select_ident when
> > udp6_ufo_fragment is called from openvswitch on a skb that doesn't
> > have a dst_entry set.
> > 
> > Skip __ipv6_select_ident in case of a NULL rt.
> > 
> > Fixes: 0508c07f5e0c ("ipv6: Select fragment id during UFO segmentation if not set.")
> > Cc: Vladislav Yasevich <vyasevic@redhat.com>
> > Reported-by: Matt Grant <matt@mattgrant.net.nz>
> > Tested-by: Matt Grant <matt@mattgrant.net.nz>
> > Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
> > ---
> [...]
> 
> Hi Sabrina
> 
> This would result in us using id 0 which is not what we want to do.
> 
> In this case, udp6_ufo_fragment() should be calling ipv6_proxy_select_ident() so that
> the fragment id is properly generated.

Hi Vlad,

So, instead, something like this?  Or do you want to use
ipv6_proxy_select_ident even when we have a skb_dst?


diff --git a/net/ipv6/udp_offload.c b/net/ipv6/udp_offload.c
index ab889bb16b3c..01c41122ddd7 100644
--- a/net/ipv6/udp_offload.c
+++ b/net/ipv6/udp_offload.c
@@ -112,11 +112,17 @@ static struct sk_buff *udp6_ufo_fragment(struct sk_buff *skb,
 		fptr = (struct frag_hdr *)(skb_network_header(skb) + unfrag_ip6hlen);
 		fptr->nexthdr = nexthdr;
 		fptr->reserved = 0;
-		if (skb_shinfo(skb)->ip6_frag_id)
+		if (skb_shinfo(skb)->ip6_frag_id) {
 			fptr->identification = skb_shinfo(skb)->ip6_frag_id;
-		else
-			ipv6_select_ident(fptr,
-					  (struct rt6_info *)skb_dst(skb));
+		} else {
+			struct rt6_info *rt = (struct rt6_info *)skb_dst(skb);
+			if (rt) {
+				ipv6_select_ident(fptr, rt);
+			} else {
+				ipv6_proxy_select_ident(skb);
+				fptr->identification = skb_shinfo(skb)->ip6_frag_id;
+			}
+		}
 
 		/* Fragment the skb. ipv6 header and the remaining fields of the
 		 * fragment header are updated in ipv6_gso_segment()


-- 
Sabrina

Re: [PATCH net] ipv6: skip __ipv6_select_ident when there is no skb_dst

[Vlad Yasevich]

On 03/18/2015 10:15 AM, Sabrina Dubroca wrote:
> 2015-03-18, 10:04:10 -0400, Vlad Yasevich wrote:
>> On 03/18/2015 09:52 AM, Sabrina Dubroca wrote:
>>> Matt Grant reported frequent crashes in ipv6_select_ident when
>>> udp6_ufo_fragment is called from openvswitch on a skb that doesn't
>>> have a dst_entry set.
>>>
>>> Skip __ipv6_select_ident in case of a NULL rt.
>>>
>>> Fixes: 0508c07f5e0c ("ipv6: Select fragment id during UFO segmentation if not set.")
>>> Cc: Vladislav Yasevich <vyasevic@redhat.com>
>>> Reported-by: Matt Grant <matt@mattgrant.net.nz>
>>> Tested-by: Matt Grant <matt@mattgrant.net.nz>
>>> Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
>>> ---
>> [...]
>>
>> Hi Sabrina
>>
>> This would result in us using id 0 which is not what we want to do.
>>
>> In this case, udp6_ufo_fragment() should be calling ipv6_proxy_select_ident() so that
>> the fragment id is properly generated.
> 
> Hi Vlad,
> 
> So, instead, something like this?  Or do you want to use
> ipv6_proxy_select_ident even when we have a skb_dst?
> 

Yes, this is what I was thinking...

-vlad

> 
> diff --git a/net/ipv6/udp_offload.c b/net/ipv6/udp_offload.c
> index ab889bb16b3c..01c41122ddd7 100644
> --- a/net/ipv6/udp_offload.c
> +++ b/net/ipv6/udp_offload.c
> @@ -112,11 +112,17 @@ static struct sk_buff *udp6_ufo_fragment(struct sk_buff *skb,
>  		fptr = (struct frag_hdr *)(skb_network_header(skb) + unfrag_ip6hlen);
>  		fptr->nexthdr = nexthdr;
>  		fptr->reserved = 0;
> -		if (skb_shinfo(skb)->ip6_frag_id)
> +		if (skb_shinfo(skb)->ip6_frag_id) {
>  			fptr->identification = skb_shinfo(skb)->ip6_frag_id;
> -		else
> -			ipv6_select_ident(fptr,
> -					  (struct rt6_info *)skb_dst(skb));
> +		} else {
> +			struct rt6_info *rt = (struct rt6_info *)skb_dst(skb);
> +			if (rt) {
> +				ipv6_select_ident(fptr, rt);
> +			} else {
> +				ipv6_proxy_select_ident(skb);
> +				fptr->identification = skb_shinfo(skb)->ip6_frag_id;
> +			}
> +		}
>  
>  		/* Fragment the skb. ipv6 header and the remaining fields of the
>  		 * fragment header are updated in ipv6_gso_segment()
> 
> 

Re: [PATCH net] ipv6: skip __ipv6_select_ident when there is no skb_dst

[Sabrina Dubroca]

2015-03-18, 10:25:43 -0400, Vlad Yasevich wrote:
> On 03/18/2015 10:15 AM, Sabrina Dubroca wrote:
> > 2015-03-18, 10:04:10 -0400, Vlad Yasevich wrote:
> >> On 03/18/2015 09:52 AM, Sabrina Dubroca wrote:
> >>> Matt Grant reported frequent crashes in ipv6_select_ident when
> >>> udp6_ufo_fragment is called from openvswitch on a skb that doesn't
> >>> have a dst_entry set.
> >>>
> >>> Skip __ipv6_select_ident in case of a NULL rt.
> >>>
> >>> Fixes: 0508c07f5e0c ("ipv6: Select fragment id during UFO segmentation if not set.")
> >>> Cc: Vladislav Yasevich <vyasevic@redhat.com>
> >>> Reported-by: Matt Grant <matt@mattgrant.net.nz>
> >>> Tested-by: Matt Grant <matt@mattgrant.net.nz>
> >>> Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
> >>> ---
> >> [...]
> >>
> >> Hi Sabrina
> >>
> >> This would result in us using id 0 which is not what we want to do.
> >>
> >> In this case, udp6_ufo_fragment() should be calling ipv6_proxy_select_ident() so that
> >> the fragment id is properly generated.
> > 
> > Hi Vlad,
> > 
> > So, instead, something like this?  Or do you want to use
> > ipv6_proxy_select_ident even when we have a skb_dst?
> > 
> 
> Yes, this is what I was thinking...
> 
> -vlad

Okay, so:

diff --git a/net/ipv6/udp_offload.c b/net/ipv6/udp_offload.c
index ab889bb16b3c..be2c0ba82c85 100644
--- a/net/ipv6/udp_offload.c
+++ b/net/ipv6/udp_offload.c
@@ -112,11 +112,9 @@ static struct sk_buff *udp6_ufo_fragment(struct sk_buff *skb,
 		fptr = (struct frag_hdr *)(skb_network_header(skb) + unfrag_ip6hlen);
 		fptr->nexthdr = nexthdr;
 		fptr->reserved = 0;
-		if (skb_shinfo(skb)->ip6_frag_id)
-			fptr->identification = skb_shinfo(skb)->ip6_frag_id;
-		else
-			ipv6_select_ident(fptr,
-					  (struct rt6_info *)skb_dst(skb));
+		if (!skb_shinfo(skb)->ip6_frag_id)
+			ipv6_proxy_select_ident(skb);
+		fptr->identification = skb_shinfo(skb)->ip6_frag_id;
 
 		/* Fragment the skb. ipv6 header and the remaining fields of the
 		 * fragment header are updated in ipv6_gso_segment()


-- 
Sabrina

Re: [PATCH net] ipv6: skip __ipv6_select_ident when there is no skb_dst

[Matt Grant]

Hi!

This fixes hard lock on 3.19.2 as well. Without it, machine drops dead -
hard locks - pretty quickly after boot.

Will try 4.0-rc4 tomorrow.  4.0-rc3 had same hard crash issues.

Get it out there, this is a IPv6 network Packet of Death!  We need
3.19.3 ASAP I think.  

I'll try and see if I can get a network trace of the cause on Saturday.

Regards,

Matt

On Wed, 2015-03-18 at 15:36 +0100, Sabrina Dubroca wrote:
> 2015-03-18, 10:25:43 -0400, Vlad Yasevich wrote:
> > On 03/18/2015 10:15 AM, Sabrina Dubroca wrote:
> > > 2015-03-18, 10:04:10 -0400, Vlad Yasevich wrote:
> > >> On 03/18/2015 09:52 AM, Sabrina Dubroca wrote:
> > >>> Matt Grant reported frequent crashes in ipv6_select_ident when
> > >>> udp6_ufo_fragment is called from openvswitch on a skb that doesn't
> > >>> have a dst_entry set.
> > >>>
> > >>> Skip __ipv6_select_ident in case of a NULL rt.
> > >>>
> > >>> Fixes: 0508c07f5e0c ("ipv6: Select fragment id during UFO segmentation if not set.")
> > >>> Cc: Vladislav Yasevich <vyasevic@redhat.com>
> > >>> Reported-by: Matt Grant <matt@mattgrant.net.nz>
> > >>> Tested-by: Matt Grant <matt@mattgrant.net.nz>
> > >>> Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
> > >>> ---
> > >> [...]
> > >>
> > >> Hi Sabrina
> > >>
> > >> This would result in us using id 0 which is not what we want to do.
> > >>
> > >> In this case, udp6_ufo_fragment() should be calling ipv6_proxy_select_ident() so that
> > >> the fragment id is properly generated.
> > > 
> > > Hi Vlad,
> > > 
> > > So, instead, something like this?  Or do you want to use
> > > ipv6_proxy_select_ident even when we have a skb_dst?
> > > 
> > 
> > Yes, this is what I was thinking...
> > 
> > -vlad
> 
> Okay, so:
> 
> diff --git a/net/ipv6/udp_offload.c b/net/ipv6/udp_offload.c
> index ab889bb16b3c..be2c0ba82c85 100644
> --- a/net/ipv6/udp_offload.c
> +++ b/net/ipv6/udp_offload.c
> @@ -112,11 +112,9 @@ static struct sk_buff *udp6_ufo_fragment(struct sk_buff *skb,
>  		fptr = (struct frag_hdr *)(skb_network_header(skb) + unfrag_ip6hlen);
>  		fptr->nexthdr = nexthdr;
>  		fptr->reserved = 0;
> -		if (skb_shinfo(skb)->ip6_frag_id)
> -			fptr->identification = skb_shinfo(skb)->ip6_frag_id;
> -		else
> -			ipv6_select_ident(fptr,
> -					  (struct rt6_info *)skb_dst(skb));
> +		if (!skb_shinfo(skb)->ip6_frag_id)
> +			ipv6_proxy_select_ident(skb);
> +		fptr->identification = skb_shinfo(skb)->ip6_frag_id;
>  
>  		/* Fragment the skb. ipv6 header and the remaining fields of the
>  		 * fragment header are updated in ipv6_gso_segment()
> 
> 

-- 
Matt Grant,  Debian and Linux Systems Administration and Consulting
Mobile: 021 0267 0578
Email: matt@mattgrant.net.nz

Re: [PATCH net] ipv6: skip __ipv6_select_ident when there is no skb_dst

[Sabrina Dubroca]

2015-03-19, 22:39:50 +1300, Matt Grant wrote:
> Hi!
> 
> This fixes hard lock on 3.19.2 as well. Without it, machine drops dead -
> hard locks - pretty quickly after boot.
> 
> Will try 4.0-rc4 tomorrow.  4.0-rc3 had same hard crash issues.
> 
> Get it out there, this is a IPv6 network Packet of Death!  We need
> 3.19.3 ASAP I think.  
> 
> I'll try and see if I can get a network trace of the cause on Saturday.
> 
> Regards,
> 
> Matt

Thanks for testing this, Matt.
I will submit the patch in a few minutes.

-- 
Sabrina

[PATCH net] ipv6: call ipv6_proxy_select_ident instead of ipv6_select_ident in udp6_ufo_fragment

[Sabrina Dubroca]

Matt Grant reported frequent crashes in ipv6_select_ident when
udp6_ufo_fragment is called from openvswitch on a skb that doesn't
have a dst_entry set.

ipv6_proxy_select_ident generates the frag_id without using the dst
associated with the skb.  This approach was suggested by Vladislav
Yasevich.

Fixes: 0508c07f5e0c ("ipv6: Select fragment id during UFO segmentation if not set.")
Cc: Vladislav Yasevich <vyasevic@redhat.com>
Reported-by: Matt Grant <matt@mattgrant.net.nz>
Tested-by: Matt Grant <matt@mattgrant.net.nz>
Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
---
David, can you queue this for stable/3.19?  Thanks.

 net/ipv6/udp_offload.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/net/ipv6/udp_offload.c b/net/ipv6/udp_offload.c
index ab889bb16b3c..be2c0ba82c85 100644
--- a/net/ipv6/udp_offload.c
+++ b/net/ipv6/udp_offload.c
@@ -112,11 +112,9 @@ static struct sk_buff *udp6_ufo_fragment(struct sk_buff *skb,
 		fptr = (struct frag_hdr *)(skb_network_header(skb) + unfrag_ip6hlen);
 		fptr->nexthdr = nexthdr;
 		fptr->reserved = 0;
-		if (skb_shinfo(skb)->ip6_frag_id)
-			fptr->identification = skb_shinfo(skb)->ip6_frag_id;
-		else
-			ipv6_select_ident(fptr,
-					  (struct rt6_info *)skb_dst(skb));
+		if (!skb_shinfo(skb)->ip6_frag_id)
+			ipv6_proxy_select_ident(skb);
+		fptr->identification = skb_shinfo(skb)->ip6_frag_id;
 
 		/* Fragment the skb. ipv6 header and the remaining fields of the
 		 * fragment header are updated in ipv6_gso_segment()
-- 
2.3.3

Re: [PATCH net] ipv6: call ipv6_proxy_select_ident instead of ipv6_select_ident in udp6_ufo_fragment

[Vlad Yasevich]

On 03/19/2015 06:22 AM, Sabrina Dubroca wrote:
> Matt Grant reported frequent crashes in ipv6_select_ident when
> udp6_ufo_fragment is called from openvswitch on a skb that doesn't
> have a dst_entry set.
> 
> ipv6_proxy_select_ident generates the frag_id without using the dst
> associated with the skb.  This approach was suggested by Vladislav
> Yasevich.
> 
> Fixes: 0508c07f5e0c ("ipv6: Select fragment id during UFO segmentation if not set.")
> Cc: Vladislav Yasevich <vyasevic@redhat.com>
> Reported-by: Matt Grant <matt@mattgrant.net.nz>
> Tested-by: Matt Grant <matt@mattgrant.net.nz>
> Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>

Acked-by: Vladislav Yasevich <vyasevic@redhat.com>

-vlad

> ---
> David, can you queue this for stable/3.19?  Thanks.
> 
>  net/ipv6/udp_offload.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/net/ipv6/udp_offload.c b/net/ipv6/udp_offload.c
> index ab889bb16b3c..be2c0ba82c85 100644
> --- a/net/ipv6/udp_offload.c
> +++ b/net/ipv6/udp_offload.c
> @@ -112,11 +112,9 @@ static struct sk_buff *udp6_ufo_fragment(struct sk_buff *skb,
>  		fptr = (struct frag_hdr *)(skb_network_header(skb) + unfrag_ip6hlen);
>  		fptr->nexthdr = nexthdr;
>  		fptr->reserved = 0;
> -		if (skb_shinfo(skb)->ip6_frag_id)
> -			fptr->identification = skb_shinfo(skb)->ip6_frag_id;
> -		else
> -			ipv6_select_ident(fptr,
> -					  (struct rt6_info *)skb_dst(skb));
> +		if (!skb_shinfo(skb)->ip6_frag_id)
> +			ipv6_proxy_select_ident(skb);
> +		fptr->identification = skb_shinfo(skb)->ip6_frag_id;
>  
>  		/* Fragment the skb. ipv6 header and the remaining fields of the
>  		 * fragment header are updated in ipv6_gso_segment()
> 

Re: [PATCH net] ipv6: call ipv6_proxy_select_ident instead of ipv6_select_ident in udp6_ufo_fragment

[David Miller]

From: Sabrina Dubroca <sd@queasysnail.net>
Date: Thu, 19 Mar 2015 11:22:32 +0100

> Matt Grant reported frequent crashes in ipv6_select_ident when
> udp6_ufo_fragment is called from openvswitch on a skb that doesn't
> have a dst_entry set.
> 
> ipv6_proxy_select_ident generates the frag_id without using the dst
> associated with the skb.  This approach was suggested by Vladislav
> Yasevich.
> 
> Fixes: 0508c07f5e0c ("ipv6: Select fragment id during UFO segmentation if not set.")
> Cc: Vladislav Yasevich <vyasevic@redhat.com>
> Reported-by: Matt Grant <matt@mattgrant.net.nz>
> Tested-by: Matt Grant <matt@mattgrant.net.nz>
> Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>

Applied and queued up for -stable, thanks.