netdev - Re: Flow classifier proto-dst and TOS (and proto-src)

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] [day] [month] [year] [list]

Message-ID: <1319428931.2517.28.camel@edumazet-laptop>
Date:	Mon, 24 Oct 2011 06:02:11 +0200
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	Dan Siemon <dan@...erfire.com>
Cc:	netdev <netdev@...r.kernel.org>
Subject: Re: Flow classifier proto-dst and TOS (and proto-src)

Le dimanche 23 octobre 2011 à 21:03 -0400, Dan Siemon a écrit :
> On Mon, 2011-10-17 at 08:09 +0200, Eric Dumazet wrote:
> > Le samedi 15 octobre 2011 à 12:51 -0400, Dan Siemon a écrit :
> > > cls_flow.c: flow_get_proto_dst()
> > > 
> > > The proto-dst key returns the destination port for UDP, TCP and a few
> > > other protocols [see proto_ports_offset()]. For ICMP and IPIP it falls
> > > back to:
> > > 
> > > return addr_fold(skb_dst(skb)) ^ (__force u16)skb->protocol;
> > > 
> > > Since Linux maintains a dst_entry for each TOS value this causes the
> > > returned value to be affected by the TOS which is unexpected and
> > > probably broken.
> > 
> > Hi Dan
> > 
> > I think Patrick did this on purpose, because of of the lack of
> > perturbation in cls_flow.c : If all these frames were mapped to a single
> > flow, they might interfere with an other regular flow and hurt it.
> > 
> > I dont qualify existing code as buggy. Its about fallback behavior
> > anyway (I dont think its even documented)
> 
> Thanks for the review Eric.
> 
> Won't virtually all uses of proto-dst also use the dst key anyway? In
> which case this fallback does nothing except make the TOS effect the
> hash output because the dst will be the same and dst_entry would be the
> same if it wasn't for the different TOS (by far the common case). I
> don't see the value of the unintuitive behavior.
> 
> I'm not certain this is a problem but also note that including TOS will
> mean that packets within a tunnel will be reordered if 'tos inherit' is
> set on the tunnel and only the typical src,dst,proto,proto-src,proto-dst
> is used. Again, probably not expected.

Hmm, it seems cls_flow now supports "perturbation", so my prior argument
is not true anymore. I guess your patch would be fine, but I prefer
waiting Patrick review on it ;)



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