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

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, 18 Jan 2022 12:37:52 -0800
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Maciej Żenczykowski <maze@...gle.com>
Cc:     "Tyler Wear (QUIC)" <quic_twear@...cinc.com>,
        Network Development <netdev@...r.kernel.org>,
        bpf <bpf@...r.kernel.org>, Yonghong Song <yhs@...com>,
        Martin KaFai Lau <kafai@...com>,
        Toke Høiland-Jørgensen <toke@...hat.com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Song Liu <song@...nel.org>
Subject: Re: [PATCH bpf-next v6 1/2] Add skb_store_bytes() for BPF_PROG_TYPE_CGROUP_SKB

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