[Bismark-devel] ECN bug found, fixed

[Bismark-devel] ECN bug found, fixed

[Dave Täht]

We think we've found and are in the process of fixing a bug in ECN
handling in Linux. I'm curious if you had ecn turned on at all in your
initial testing and what qdisc you were using.



Eric Dumazet <eric.dumazet@gmail.com> writes:

> Le lundi 14 mars 2011 à 21:24 +0100, Eric Dumazet a écrit :
>
> remove CC to bloat lists for now, adding David Miller to thread.
>
>> Le lundi 14 mars 2011 à 21:55 +0200, Jonathan Morton a écrit :
>> > On 14 Mar, 2011, at 9:26 pm, Dave Täht wrote:
>> > 
>> > > Over the weekend, Dan Siemons uncovered a possible bad interaction
>> > > between ECN and the default pfifo_fast qdisc in Linux.
>> > > 
>> > > http://www.coverfire.com/archives/2011/03/13/pfifo_fast-and-ecn/
>> > 
>> > This seems to be more complicated that it appears.  It looks as though
>> > Linux has re-used the LSB of the old TOS field for some "link local"
>> > flag which is used by routing.
>> > 
>> > It's not immediately obvious whether pfifo_fast is using this new
>> > interpretation though.  If it isn't, the fix should be to remove the
>> > RTO_ONLINK bit from the mask it's using on the tos field.  The other
>> > half of the mask correctly excludes the ECN bits from the field.
>> > 
>> 
>> CC netdev, where linux network dev can take a look.
>> 
>> I would say that this is a wrong analysis : 
>> 
>> 1) ECN uses two low order bits of TOS byte
>> 
>> 2) pfifo_fast uses skb->priority
>> 
>> 
>> skb->priority = rt_tos2priority(iph->tos);
>> 
>> #define IPTOS_TOS_MASK            0x1E
>> #define IPTOS_TOS(tos)            ((tos)&IPTOS_TOS_MASK)
>> 
>> static inline char rt_tos2priority(u8 tos)
>> {
>> 	return ip_tos2prio[IPTOS_TOS(tos)>>1];
>> }
>> 
>> No interference between two mechanisms, unless sysadmin messed up things
>> (skb_edit)
>> 
>> 
>
> David, it seems ip_tos2prio is wrong on its 2nd entry :
>
> #define TC_PRIO_BESTEFFORT              0
> #define TC_PRIO_FILLER                  1
> #define TC_PRIO_BULK                    2
> #define TC_PRIO_INTERACTIVE_BULK        4
> #define TC_PRIO_INTERACTIVE             6
> #define TC_PRIO_CONTROL                 7
>
> #define TC_PRIO_MAX                     15
>
> net/ipv4/route.c:170:#define ECN_OR_COST(class) TC_PRIO_##class
>
> const __u8 ip_tos2prio[16] = {
> 	TC_PRIO_BESTEFFORT,   /* 0 : for flow without ECN */
> 	ECN_OR_COST(FILLER), /* 1 : flow with ECN */
> 	...
> };
>
>
>
>
> This means ECN enabled flows got TC_PRIO_FILLER (what the hell is
> that ?)
>
> pfifo_fast has :
>
> static const u8 prio2band[TC_PRIO_MAX+1] =
> 	{ 1, 2, 2, 2, 1, 2, 0, 0 , 1, 1, 1, 1, 1, 1, 1, 1 };
>
> So a non ECN enabled flow goes to band 1, while an ECN enabled one is in
> band 2 (!).  Thus, ECN enabled flows have a chance being droped more
> often than non ECN flows. Thats not fair...
>
> What do you think ?
>
> Thanks
>
> diff --git a/net/ipv4/route.c b/net/ipv4/route.c
> index 6ed6603..fabfe81 100644
> --- a/net/ipv4/route.c
> +++ b/net/ipv4/route.c
> @@ -171,7 +171,7 @@ static struct dst_ops ipv4_dst_ops = {
>  
>  const __u8 ip_tos2prio[16] = {
>  	TC_PRIO_BESTEFFORT,
> -	ECN_OR_COST(FILLER),
> +	ECN_OR_COST(BESTEFFORT),
>  	TC_PRIO_BESTEFFORT,
>  	ECN_OR_COST(BESTEFFORT),
>  	TC_PRIO_BULK,
>
>

-- 
Dave Taht
http://nex-6.taht.net