> -----Original Message----- > From: Alexei Starovoitov <alexei.starovoitov@xxxxxxxxx> > Sent: Wednesday, January 12, 2022 6:14 PM > To: Tyler Wear (QUIC) <quic_twear@xxxxxxxxxxx> > Cc: Network Development <netdev@xxxxxxxxxxxxxxx>; bpf > <bpf@xxxxxxxxxxxxxxx>; Maciej Żenczykowski <maze@xxxxxxxxxx>; > Yonghong Song <yhs@xxxxxx>; Martin KaFai Lau <kafai@xxxxxx>; Toke > Høiland-Jørgensen <toke@xxxxxxxxxx>; Daniel Borkmann > <daniel@xxxxxxxxxxxxx>; Song Liu <song@xxxxxxxxxx> > 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@xxxxxxxxxxx> > 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@xxxxxxxxxxx> > > --- > > 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? > > TC layer is where packet modifications are allowed.
