On Wed, Dec 22, 2021 at 10:49:45PM +0000, Tyler Wear wrote:
> > On 12/21/21 6:27 PM, Tyler Wear 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.
> >
> > Could change tag from "[PATCH]" to "[PATCH bpf-next]"?
> > Please also indicate the version of the patch, so in this case, it should be "[PATCH bpf-next v2]".
> >
> > I think you can add more contents in the commit message about why existing bpf_setsockopt() won't work and why
> > CGROUP_UDP[4|6]_SENDMSG is not preferred.
> > These have been discussed in v1 of this patch and they are valuable for people to understand full context and reasoning.
> >
> > >
> > > Signed-off-by: Tyler Wear <quic_twear@xxxxxxxxxxx>
> > > ---
> > > net/core/filter.c | 2 ++
> > > 1 file changed, 2 insertions(+)
> > >
> > > diff --git a/net/core/filter.c b/net/core/filter.c index
> > > 6102f093d59a..0c25aa2212a2 100644
> > > --- a/net/core/filter.c
> > > +++ b/net/core/filter.c
> > > @@ -7289,6 +7289,8 @@ static const struct bpf_func_proto *
> > > cg_skb_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
> > > {
> > > switch (func_id) {
> > > + case BPF_FUNC_skb_store_bytes:
> > > + return &bpf_skb_store_bytes_proto;
> >
> > Typically different 'case's are added in chronological order to people can guess what is added earlier and what is added later. Maybe
> > add the new helper after BPF_FUNC_perf_event_output?
> >
> > > case BPF_FUNC_get_local_storage:
> > > return &bpf_get_local_storage_proto;
> > > case BPF_FUNC_sk_fullsock:
> >
> > Please add a test case to exercise the new usage of
> > bpf_skb_store_bytes() helper. You may piggy back on some existing cg_skb progs if it is easier to do.
>
> Would it be sufficient to change the dscp value in tools/testing/selftests/bpf/progs/test_sock_fields.c via bpf_skb_store_bytes()
test_sock_fields focus on sk instead of skb, so it will not be a good fit.
load_bytes_relative.c may be a better fit.
The minimal is to write the dscp value by bpf_skb_store_bytes()
and be able to read it back at the receiver side (e.g.
by making a TCP connection like load_bytes_relative).
