netdev - RE: [PATCH] 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]

Message-ID: <BYAPR02MB52384D4B920EE2DB7C6D0F89AA7D9@BYAPR02MB5238.namprd02.prod.outlook.com>
Date:   Wed, 22 Dec 2021 22:49:45 +0000
From:   Tyler Wear <twear@...cinc.com>
To:     Yonghong Song <yhs@...com>,
        "Tyler Wear (QUIC)" <quic_twear@...cinc.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "bpf@...r.kernel.org" <bpf@...r.kernel.org>
CC:     "kafai@...com" <kafai@...com>, "maze@...gle.com" <maze@...gle.com>
Subject: RE: [PATCH] Add skb_store_bytes() for BPF_PROG_TYPE_CGROUP_SKB

> 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@...cinc.com>
> > ---
> >   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()