On Fri, Jan 14, 2022 at 1:18 PM Maciej Żenczykowski <maze@xxxxxxxxxx> wrote: > > > > > > This is wrong. > > > > > CGROUP_INET_EGRESS bpf prog cannot arbitrary change packet data. > > I agree with this sentiment, which is why the original proposal was > simply to add a helper which is only capable of modifying the > tos/tclass/dscp field, and not any arbitrary bytes. (note: there > already is such a helper to set the ECN congestion notification bits, > so there's somewhat of a precedent) True. bpf_skb_ecn_set_ce() is available to cg_skb progs. An arbitrary tos rewriting helper would screw it up. > > > > > The networking stack populated the IP header at that point. > > > > > If the prog changes it to something else it will be confusing other > > > > > layers of stack. neigh(L2) will be wrong, etc. > > > > > We can still change certain things in the packet, but not arbitrary bytes. > > > > > > > > > > We cannot change the DS field directly in the packet either. > > This part I won't agree with. In most cases there is no DSCP based > routing decision, in which case it seems perfectly reasonable to > change the DSCP bits here. Indeed last I checked (though this was a > few years ago) the ipv4 tos routing code wasn't even capable of making > sane decisions, because it looks at the bottom 4 bits of the TOS > field, instead of the top 6 bits, ie. you can route on ECN bits, but > you can't route on the full DSCP field. Additionally afaik the ipv6 > tclass routing simply wasn't implemented. However, I last had to deal > with this probably half a decade ago, on even older kernels, so > perhaps the situation has changed. > > Additionally DSCP bits may affect transmit queue selection (for > something like wifi qos / traffic prioritization across multiple > transmit queues with different air-time behaviours - which can use > dscp), so ideally we need dscp to be set *before* the mq qdisc / > dispatch. I think this implies it needs to happen before tc (though > again, I'm not too certain of the ordering here). and that's where tc bpf progs are running. Right before qdiscs. They can change any byte of the packet. So I still don't see a use case of adding a specific tos/tclass/dscp rewriting helper to cg_skb when tc bpf prog are already capable.
