Occasional oops with IPSec and IPv6.

On 11/18/2011 11:21 PM, Nick Bowler wrote:
> On 2011-11-18 22:06 +0200, Timo Teräs wrote:
>> It's still headroom underrun.
>>
>> I'm not too familiar with the relevant IPv6 code, but it seems to be
>> mostly modelled after the IPv4 side. Looking at the back trace offset
>> inside ipv6_fragment, I'd say it was taking the "fast path" for
>> constructing the fragments. So first guess is that the headroom check
>> for allowing fast path to happen is not right.
>>
>> Since the code seems to be treating separately hlen and struct frag_hdr,
>> I'm wondering if the following patch would be in place?
>>
>> diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
>> index 1c9bf8b..c35d9fc 100644
>> --- a/net/ipv6/ip6_output.c
>> +++ b/net/ipv6/ip6_output.c
>> @@ -675,7 +675,7 @@ int ip6_fragment(struct sk_buff *skb, int
>> (*output)(struct sk_buff *))
>>  			/* Correct geometry. */
>>  			if (frag->len > mtu ||
>>  			    ((frag->len & 7) && frag->next) ||
>> -			    skb_headroom(frag) < hlen)
>> +			    skb_headroom(frag) < hlen + sizeof(struct frag_hdr))
>>  				goto slow_path_clean;
>>
>>  			/* Partially cloned skb? */
>>
>>
>> Alternatively, we could just run the "slow path" unconditionally with
>> the test load to see if it fixes the issue. At least that'd be pretty
>> good test if it's a problem in the ipv6 fragmentation code or something
>> else.
> 
> Good call.  I replaced the "correct geometry" check with an
> unconditional "goto slow_path_clean;", and I can no longer reproduce the
> crash.  So at the very least, I have a workaround now.  (I still have
> Herbert Xu's six patches applied on top of Linus' master).

Ok, so it's most likely ipv6 code issue then. My change just happened to
trigger it.

> I then tried the smaller change above, but this does not correct the
> issue.

That's not it then (likely).

I did notice that the headroom of the main skb is never checked. So my
other suggestion is to try something like:

 		skb_walk_frags(skb, frag) {

Other than that, I hope some of the ipv6 people could take a look at it.
But the problem is that somewhere some headroom check isn't taking
place, or is checking for too little of headroom.

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

On 2011-11-19 09:36 +0200, Timo Teräs wrote:
> On 11/18/2011 11:21 PM, Nick Bowler wrote:
> > On 2011-11-18 22:06 +0200, Timo Teräs wrote:
> >> Alternatively, we could just run the "slow path" unconditionally with
> >> the test load to see if it fixes the issue. At least that'd be pretty
> >> good test if it's a problem in the ipv6 fragmentation code or something
> >> else.
> > 
> > Good call.  I replaced the "correct geometry" check with an
> > unconditional "goto slow_path_clean;", and I can no longer reproduce the
> > crash.  So at the very least, I have a workaround now.  (I still have
> > Herbert Xu's six patches applied on top of Linus' master).
> 
> Ok, so it's most likely ipv6 code issue then. My change just happened to
> trigger it.
> 
> > I then tried the smaller change above, but this does not correct the
> > issue.
> 
> That's not it then (likely).
> 
> I did notice that the headroom of the main skb is never checked. So my
> other suggestion is to try something like:
> 
> diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
> index 1c9bf8b..735c4dc 100644
> --- a/net/ipv6/ip6_output.c
> +++ b/net/ipv6/ip6_output.c
> @@ -668,7 +668,8 @@ int ip6_fragment(struct sk_buff *skb, int
> (*output)(struct sk_buff *))
> 
>  		if (first_len - hlen > mtu ||
>  		    ((first_len - hlen) & 7) ||
> -		    skb_cloned(skb))
> +		    skb_cloned(skb) ||
> +		    skb_headroom(skb) < sizeof(struct frag_hdr))
>  			goto slow_path;
> 
>  		skb_walk_frags(skb, frag) {

Tried this (still on top of Herbert Xu's patch set) to no avail; the
crash still occurs :(.

Using the unconditional slow path workaround, I ran the test over the
weekend and it did not crash, so that seems to be stable.

> Other than that, I hope some of the ipv6 people could take a look at it.
> But the problem is that somewhere some headroom check isn't taking
> place, or is checking for too little of headroom.

Thanks,