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

[Dan Siemon <dan@coverfire.com>]

[-- Attachment #1: Type: text/plain, Size: 2279 bytes --]

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.

> If you have too many frames going to the fallback, then this classifier
> is probably not the one you should use ?

If you have significant traffic in tunnels then any 5-tuple approach is
going to present problems unless you look into the tunnel (like my other
patch :) )

> Hint : You can change your filter to use this classifier only on TCP/UDP
> trafic, and use another one on other protocols : Coupled to your qdisc
> rules, you even can limit to X percent the bandwidth allocated to this
> trafic
> 
> We could argue that if TOS value of two packets is different, then
> packets belong to different flows as well. [ It seems we currently lack
> a FLOW_KEY_TOS : that could be a usefull addition ]



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]