Re: [PATCHv3] netns: oops in ip[6]_frag_reasm incrementing stats

[Jarek Poplawski <jarkao2@gmail.com>]

On 17-03-2009 14:54, Jorge Boncompte [DTI2] wrote:
> Jarek Poplawski escribió:
>> On Tue, Mar 17, 2009 at 12:55:42PM +0100, Jorge Boncompte [DTI2] wrote:
>>> dev can be NULL in ip[6]_frag_reasm for skb's coming from RAW sockets.
>>>
>>> Quagga's OSPFD sends fragmented packets on a RAW socket, when netfilter
>>> conntrack reassembles them on the OUTPUT path you hit this code path.
>>>
>>> You can test it with something like "hping2 -0 -d 2000 -f AA.BB.CC.DD"
>>>
>>> Changes from v2: (address comments from Jarek Poplawski)
>>> 	- Patch reworked to get the net pointer with container_of()
>>> 	  instead of passing it to function calls.
>>> 	- Fix IPv6 code
>>> Changes from v1:
>>> 	- Fixed description
>> I guess David will be interested only with the final state of changes,
>> so v1 & v2 are not necessary here...
>>
>> Anyway, ipv4 looks OK to me, but ipv6 looks like something is
>> different:
>>> +	IP6_INC_STATS_BH(net, __in6_dev_get(dev), IPSTATS_MIB_REASMFAILS);
>> It still depends on dev != NULL in __in6_dev_get(). I see there
>> is also used skb->dst for similar things in ip6_frag_queue(), so I
>> don't know: it needs rethinking, and maybe these patches should be
>> separated if you prefer.
> 
> 	Not my day! :-) I should not look at code at 2 am and write patches
> the day after, I confused _idev and idev in the check for != NULL in _DEVINC.
> 
> 	I think this bug was first introduced by patch "[IPV6]: Per-interface
> statistics support." from YOSHIFUJI Hideaki on Nov 4, 2006.
> 
> 	If someone with more knowledge could confirm that using something like...
> 
> "(skb->dev ? skb->dev : skb->dst->dev)"
> 
> ... here is fine I'll redo this part and resend. I do not have an IPv6 setup where
> I can test this.
> 

skb->dst->dev is used in ip6_frag_queue anyway, so it shouldn't be
worse if you do this similarly in ip6_frag_reasm. If you send it as
a separate patch, and write it's not tested, David will decide if he
wants it. Otherwise you can resend this ipv4 patch only.

Jarek P.