netdev - Re: [Bloat] shaper team forming up

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 15 Mar 2011 05:42:46 +0100
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	Jonathan Morton <chromatix99@...il.com>,
	David Miller <davem@...emloft.net>
Cc:	Dave Täht <d@...t.net>,
	netdev <netdev@...r.kernel.org>
Subject: Re: [Bloat] shaper team forming up

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,


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