Message-ID: <4AE87ECD.7080408@gmail.com>
Date:	Wed, 28 Oct 2009 18:26:37 +0100
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	Steve Chen <schen@...sta.com>
CC:	netdev@...r.kernel.org
Subject: Re: [Fwd: Re: [PATCH] Multicast packet reassembly can fail]

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.

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

               (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

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists