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

[Jarek Poplawski <jarkao2@gmail.com>]

On Wed, Mar 18, 2009 at 11:26:51PM -0700, David Miller wrote:
> From: Jarek Poplawski <jarkao2@gmail.com>
> Date: Wed, 18 Mar 2009 07:26:58 +0000
> 
> > 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.
> 
> Netfilter on ipv6 handles reassembly differently and in a way
> that won't result in dev being NULL here.
> 
> I've applied Jorge's patch, thanks everyone.

My proposal is to revert the ipv6/reassembly part of this patch yet;
it doesn't fix anything, and the changelog is only misleading if it's
like you said.

Jarek P.