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

["Tyler Wear (QUIC)" <quic_twear@quicinc.com>]

> -----Original Message-----
> From: Tyler Wear <twear@quicinc.com>
> Sent: Wednesday, January 12, 2022 9:13 PM
> To: Alexei Starovoitov <alexei.starovoitov@gmail.com>; Tyler Wear (QUIC)
> <quic_twear@quicinc.com>
> Cc: Network Development <netdev@vger.kernel.org>; bpf
> <bpf@vger.kernel.org>; Maciej Żenczykowski <maze@google.com>;
> Yonghong Song <yhs@fb.com>; Martin KaFai Lau <kafai@fb.com>; Toke
> Høiland-Jørgensen <toke@redhat.com>; Daniel Borkmann
> <daniel@iogearbox.net>; Song Liu <song@kernel.org>
> Subject: RE: [PATCH bpf-next v6 1/2] Add skb_store_bytes() for
> BPF_PROG_TYPE_CGROUP_SKB
> 
> 
> 
> > -----Original Message-----
> > From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
> > Sent: Wednesday, January 12, 2022 6:14 PM
> > To: Tyler Wear (QUIC) <quic_twear@quicinc.com>
> > Cc: Network Development <netdev@vger.kernel.org>; bpf
> > <bpf@vger.kernel.org>; Maciej Żenczykowski <maze@google.com>;
> Yonghong
> > Song <yhs@fb.com>; Martin KaFai Lau <kafai@fb.com>; Toke
> > Høiland-Jørgensen <toke@redhat.com>; Daniel Borkmann
> > <daniel@iogearbox.net>; Song Liu <song@kernel.org>
> > Subject: Re: [PATCH bpf-next v6 1/2] Add skb_store_bytes() for
> > BPF_PROG_TYPE_CGROUP_SKB
> >
> > WARNING: This email originated from outside of Qualcomm. Please be
> > wary of any links or attachments, and do not enable macros.
> >
> > On Wed, Jan 12, 2022 at 5:15 PM Tyler Wear <quic_twear@quicinc.com>
> > wrote:
> > >
> > > Need to modify the ds field to support upcoming Wifi QoS Alliance spec.
> > > Instead of adding generic function for just modifying the ds field,
> > > add skb_store_bytes for BPF_PROG_TYPE_CGROUP_SKB.
> > > This allows other fields in the network and transport header to be
> > > modified in the future.
> > >
> > > Checksum API's also need to be added for completeness.
> > >
> > > It is not possible to use CGROUP_(SET|GET)SOCKOPT since the policy
> > > may change during runtime and would result in a large number of
> > > entries with wildcards.
> > >
> > > V4 patch fixes warnings and errors from checkpatch.
> > >
> > > The existing check for bpf_try_make_writable() should mean that
> > > skb_share_check() is not needed.
> > >
> > > Signed-off-by: Tyler Wear <quic_twear@quicinc.com>
> > > ---
> > >  net/core/filter.c | 12 ++++++++++++
> > >  1 file changed, 12 insertions(+)
> > >
> > > diff --git a/net/core/filter.c b/net/core/filter.c index
> > > 6102f093d59a..f30d939cb4cf 100644
> > > --- a/net/core/filter.c
> > > +++ b/net/core/filter.c
> > > @@ -7299,6 +7299,18 @@ cg_skb_func_proto(enum bpf_func_id
> func_id,
> > const struct bpf_prog *prog)
> > >                 return &bpf_sk_storage_delete_proto;
> > >         case BPF_FUNC_perf_event_output:
> > >                 return &bpf_skb_event_output_proto;
> > > +       case BPF_FUNC_skb_store_bytes:
> > > +               return &bpf_skb_store_bytes_proto;
> > > +       case BPF_FUNC_csum_update:
> > > +               return &bpf_csum_update_proto;
> > > +       case BPF_FUNC_csum_level:
> > > +               return &bpf_csum_level_proto;
> > > +       case BPF_FUNC_l3_csum_replace:
> > > +               return &bpf_l3_csum_replace_proto;
> > > +       case BPF_FUNC_l4_csum_replace:
> > > +               return &bpf_l4_csum_replace_proto;
> > > +       case BPF_FUNC_csum_diff:
> > > +               return &bpf_csum_diff_proto;
> >
> > This is wrong.
> > CGROUP_INET_EGRESS bpf prog cannot arbitrary change packet data.
> > 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.
> > It can only be changed by changing its value in the socket.
> 
> Why is the DS field unchangeable, but ecn is changeable?

Per spec the requirement is to modify the ds field of egress packets with DSCP value. Setting ds field on socket will not suffice here.
Another case is where device is a middle-man and needs to modify the packets of a connected tethered client with the DSCP value, using a sock will not be able to change the packet here.

Spec doc can be found at: https://www.wi-fi.org/discover-wi-fi/wi-fi-qos-management

> 
> >
> > TC layer is where packet modifications are allowed.