Re: [PATCH bpf-next v6 1/2] Add skb_store_bytes() for BPF_PROG_TYPE_CGROUP_SKB

[Alexei Starovoitov <alexei.starovoitov@gmail.com>]

On Fri, Jan 14, 2022 at 1:18 PM Maciej Żenczykowski <maze@google.com> 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.