Re: [Fwd: Re: [PATCH] Multicast packet reassembly can fail]

[Steve Chen <schen@mvista.com>]

On Wed, 2009-10-28 at 18:26 +0100, Eric Dumazet wrote:
> Steve Chen a écrit :
> > On Wed, 2009-10-28 at 16:32 +0100, Eric Dumazet wrote:
> >> If each fragment is received twice on host, once by eth0, once by eth1,
> >> should we deliver datagram once or twice ?
> > 
> > The application received it once.  IIRC the duplicate packet is drop in
> > the routing code.
> > 
> >> Once should be enough, even if in the non fragmented case, it will
> >> be delivered twice (kernel cannot detect duplicates, user app might do itself)
> > 
> > Routing code drops the duplicate packet for none-fragmented case as
> > well.
> 
> Really ? How so ? Receiving two copies of the same packet is legal.

I will have to double check exactly where the packet drop happens.  I
thought it was somewhere in routing, but it could be in netfilter.

> 
> > 
> >>
> >>> For this specific case, src/dst address, protocol, IP ID and fragment
> >>> offset are all identical.  The only difference is the ingress interface.
> >>> A good follow up question would be why would anyone in their right mind
> >>> multicast to the same destination?  well, I don't know.  I can not get
> >>> the people who reported the problem to tell me either.   Since someone
> >>> found the need to do this,  perhaps others may find it useful too.
> >>>
> >> Then, if a 2000 bytes message is fragmented in two packets, one coming
> >> from eth0, one coming from eth1, I suspect your patch drops the message,
> >> unless eth0/eth1 are part of a bonding device...
> > 
> > Actually, the patch tries to prevent packet drop for this exact
> > scenario.  Please consider the following scenarios
> > 1.  Packet comes in the fragment reassemble code in the following order
> > (eth0 frag1), (eth0 frag2), (eth1 frag1), (eth1 frag2)
> > Packet from both interfaces get reassembled and gets further processed.
> 
> Yes your patch does this, so each multicast application receives two copies of the
> same datagram.
> 
> > 
> > 2. Packet can some times arrive in (perhaps other orders as well)
> > (eth0 frag1), (eth1 frag1), (eth0 frag2), (eth1 frag2)
> > Without this patch, eth0 frag 1/2 are overwritten by eth1 frag1/2, and
> > packet from eth1 is dropped in the routing code.
> 
> Really ? how so ? I dont see how it can happen, unless you use RPF ?
> 
> current situation should be :
> 
> (eth0 frag1) : We create a context, store frag1 in it
> (eth1 frag1) : We find this context, and drop frag1 since we already have the data
>                   (maybe the bug is here, if we cannot cope with a duplicate ?)
> (eth0 frag2) : We find this context, store frag2 -> complete datagram and deliver it
> (eth1 frag2) : We find context, drop frag2 since datagram was completed.

Yes, this is exactly what is happening in the current code.

> 
>                (or maybe we create a new context that will timeout later, maybe this is your problem ?)
> 
> Net effect : We deliver the datagram correctly.
> 
> 
> > 
> >> That would break common routing setups, using two links to aggregate bandwidth ?
> > 
> > I don't believe it would.  The aggregate bandwidth will work the same as
> > before.  The attributes (src/dst addr, protocol, interface, etc.) should
> > generate a unique hash key.  If hash collision should happen with the
> > addition of iif << 5, the code still compare the original src addr along
> > with interface number, so there should be no issues.
> 
> What about the obvious :
> 
> (eth0 frag1),  (eth1 frag2)
> 
> Your patch creates two contexts since hashes are different,
> that will timeout and no packet delivered at all
> 
I see the point you are making.  I assumed, probably incorrectly, that
since eth0 and eth1 have different IP address.  I would get a complete
series of fragments for each interface.  Perhaps, I should really be
looking up the stack to see why packets were dropped.  Please correct me
if I'm mistaken.  The normal behavior is that application should be
receiving either 2 (scenario 1) or 1 (scenario 2) packets.

Regards,

Steve