Patchwork BUG: unable to handle kernel NULL pointer dereference at 00000000000000a0

login
register
mail settings
Submitter Timo Teräs
Date June 23, 2010, 8:34 p.m.
Message ID <4C226FCA.2030604@iki.fi>
Download mbox | patch
Permalink /patch/56715/
State RFC
Delegated to: David Miller
Headers show

Comments

Timo Teräs - June 23, 2010, 8:34 p.m.
On 06/23/2010 09:20 PM, Justin P. Mattock wrote:
> On 06/23/10 11:10, Timo Teräs wrote:
>> On 06/23/2010 08:29 PM, Eric Dumazet wrote:
>>   
>>> Le mercredi 23 juin 2010 à 10:00 -0700, Justin P. Mattock a écrit :
>>>     
>>>> o.k. the bisect is pointing to the below results..
>>>> (I tried git revert xxx but this commit is too big
>>>> so I'll(hopefully)manually revert it on the latest HEAD to
>>>> see if this is the actual problem im experiencing)
>>>>
>>>> 80c802f3073e84c956846e921e8a0b02dfa3755f is the first bad commit
>>>> commit 80c802f3073e84c956846e921e8a0b02dfa3755f
>>>> Author: Timo Teräs<timo.teras@iki.fi>
>>>> Date:   Wed Apr 7 00:30:05 2010 +0000
>>>>
>>>>       xfrm: cache bundles instead of policies for outgoing flows
>>>>
>>>>       __xfrm_lookup() is called for each packet transmitted out of
>>>>       system. The xfrm_find_bundle() does a linear search which can
>>>>       kill system performance depending on how many bundles are
>>>>       required per policy.
>>>>
>>>>       This modifies __xfrm_lookup() to store bundles directly in
>>>>       the flow cache. If we did not get a hit, we just create a new
>>>>       bundle instead of doing slow search. This means that we can now
>>>>       get multiple xfrm_dst's for same flow (on per-cpu basis).
>>>>
>>>>       Signed-off-by: Timo Teras<timo.teras@iki.fi>
>>>>       Signed-off-by: David S. Miller<davem@davemloft.net>
>>>>
>>>> :040000 040000 d8e60f5fa4c1329f450d9c7cdf98b34e6a177f22
>>>> 9f576e68e5bf4ce357d7f0305aee5f410250dfe2 M      include
>>>> :040000 040000 f2876df688ee36907af7b4123eea96592faaed3e
>>>> a3f6f6f94f0309106856cd99b38ec90b024eb016 M      net
>>>>        
>>> Thanks a lot for bisecting Jutin, this is really appreciated.
>>>
>>> crash is in xfrm_bundle_ok()
>>>
>>> if (xdst->policy_genid != atomic_read(&xdst->pols[0]->genid))
>>>     return 0;
>>>
>>> xdst->pols[0] contains a NULL pointer
>>>      
>> That does not really make sense, if we get this far; there's a valid
>> xfrm_state with the bundle. This means that there existed a policy with
>> it too.
>>
>> I'll take a deeper look at this tomorrow. Would it be possible to see
>> your xfrm policies?
>>
>> - Timo
>>
>>    
> 
> @Eric sure no problem doing the bisect...
> 
> as for the xfrm policy here is the link that I used to setup ipsec:
> http://www.linuxfromscratch.org/hints/downloads/files/ipsec.txt
> (just change the keys if doing real world work..(but for me just testing).
> 
> below is a temporary fix for me to get this working, tcpdump reports
> everything is doing what it should be
> 11:16:32.496166 IP xxxxx > xxxxx: AH(spi=0x00000200,seq=0x1090):
> ESP(spi=0x00000201,seq=0x1090), length 56
> 11:16:32.496212 IP xxxxx > xxxxx: AH(spi=0x00000200,seq=0x1091):
> ESP(spi=0x00000201,seq=0x1091), length 56
> 11:16:32.496259 IP xxxxx > xxxxx: AH(spi=0x00000200,seq=0x1092):
> ESP(spi=0x00000201,seq=0x1092), length 56
> 
> (tested a few mins ago, but not the right fix..)

Yes, that would break some other obscure scenarios.

Looks like it's ah inside esp. So you get chain of bundles. And only the
first bundle gets a policy. Should have thought of that. Does the below
fix it for you?

 		if (strict && fl &&

--
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
Justin Mattock - June 23, 2010, 9:44 p.m.
On 06/23/2010 01:34 PM, Timo Teräs wrote:
> On 06/23/2010 09:20 PM, Justin P. Mattock wrote:
>> On 06/23/10 11:10, Timo Teräs wrote:
>>> On 06/23/2010 08:29 PM, Eric Dumazet wrote:
>>>
>>>> Le mercredi 23 juin 2010 à 10:00 -0700, Justin P. Mattock a écrit :
>>>>
>>>>> o.k. the bisect is pointing to the below results..
>>>>> (I tried git revert xxx but this commit is too big
>>>>> so I'll(hopefully)manually revert it on the latest HEAD to
>>>>> see if this is the actual problem im experiencing)
>>>>>
>>>>> 80c802f3073e84c956846e921e8a0b02dfa3755f is the first bad commit
>>>>> commit 80c802f3073e84c956846e921e8a0b02dfa3755f
>>>>> Author: Timo Teräs<timo.teras@iki.fi>
>>>>> Date:   Wed Apr 7 00:30:05 2010 +0000
>>>>>
>>>>>        xfrm: cache bundles instead of policies for outgoing flows
>>>>>
>>>>>        __xfrm_lookup() is called for each packet transmitted out of
>>>>>        system. The xfrm_find_bundle() does a linear search which can
>>>>>        kill system performance depending on how many bundles are
>>>>>        required per policy.
>>>>>
>>>>>        This modifies __xfrm_lookup() to store bundles directly in
>>>>>        the flow cache. If we did not get a hit, we just create a new
>>>>>        bundle instead of doing slow search. This means that we can now
>>>>>        get multiple xfrm_dst's for same flow (on per-cpu basis).
>>>>>
>>>>>        Signed-off-by: Timo Teras<timo.teras@iki.fi>
>>>>>        Signed-off-by: David S. Miller<davem@davemloft.net>
>>>>>
>>>>> :040000 040000 d8e60f5fa4c1329f450d9c7cdf98b34e6a177f22
>>>>> 9f576e68e5bf4ce357d7f0305aee5f410250dfe2 M      include
>>>>> :040000 040000 f2876df688ee36907af7b4123eea96592faaed3e
>>>>> a3f6f6f94f0309106856cd99b38ec90b024eb016 M      net
>>>>>
>>>> Thanks a lot for bisecting Jutin, this is really appreciated.
>>>>
>>>> crash is in xfrm_bundle_ok()
>>>>
>>>> if (xdst->policy_genid != atomic_read(&xdst->pols[0]->genid))
>>>>      return 0;
>>>>
>>>> xdst->pols[0] contains a NULL pointer
>>>>
>>> That does not really make sense, if we get this far; there's a valid
>>> xfrm_state with the bundle. This means that there existed a policy with
>>> it too.
>>>
>>> I'll take a deeper look at this tomorrow. Would it be possible to see
>>> your xfrm policies?
>>>
>>> - Timo
>>>
>>>
>>
>> @Eric sure no problem doing the bisect...
>>
>> as for the xfrm policy here is the link that I used to setup ipsec:
>> http://www.linuxfromscratch.org/hints/downloads/files/ipsec.txt
>> (just change the keys if doing real world work..(but for me just testing).
>>
>> below is a temporary fix for me to get this working, tcpdump reports
>> everything is doing what it should be
>> 11:16:32.496166 IP xxxxx>  xxxxx: AH(spi=0x00000200,seq=0x1090):
>> ESP(spi=0x00000201,seq=0x1090), length 56
>> 11:16:32.496212 IP xxxxx>  xxxxx: AH(spi=0x00000200,seq=0x1091):
>> ESP(spi=0x00000201,seq=0x1091), length 56
>> 11:16:32.496259 IP xxxxx>  xxxxx: AH(spi=0x00000200,seq=0x1092):
>> ESP(spi=0x00000201,seq=0x1092), length 56
>>
>> (tested a few mins ago, but not the right fix..)
>
> Yes, that would break some other obscure scenarios.
>
> Looks like it's ah inside esp. So you get chain of bundles. And only the
> first bundle gets a policy. Should have thought of that. Does the below
> fix it for you?
>
> diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
> index 4bf27d9..af1c173 100644
> --- a/net/xfrm/xfrm_policy.c
> +++ b/net/xfrm/xfrm_policy.c
> @@ -2300,7 +2300,8 @@ int xfrm_bundle_ok(struct xfrm_policy *pol, struct
> xfrm_dst *first,
>   			return 0;
>   		if (xdst->xfrm_genid != dst->xfrm->genid)
>   			return 0;
> -		if (xdst->policy_genid != atomic_read(&xdst->pols[0]->genid))
> +		if (xdst->num_pols>  0&&
> +		    xdst->policy_genid != atomic_read(&xdst->pols[0]->genid))
>   			return 0;
>
>   		if (strict&&  fl&&
>
>


yeah this works.. I can see AH and ESP showing up with tcpdump..
looks good..

Reported-Bisected-By: Justin P. Mattock <justinmattock@gmail.com>

cheers,

Justin P. Mattock
--
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

Patch

diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 4bf27d9..af1c173 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -2300,7 +2300,8 @@  int xfrm_bundle_ok(struct xfrm_policy *pol, struct
xfrm_dst *first,
 			return 0;
 		if (xdst->xfrm_genid != dst->xfrm->genid)
 			return 0;
-		if (xdst->policy_genid != atomic_read(&xdst->pols[0]->genid))
+		if (xdst->num_pols > 0 &&
+		    xdst->policy_genid != atomic_read(&xdst->pols[0]->genid))
 			return 0;