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]

Date:   Tue, 21 Dec 2021 19:43:50 -0800
From:   Yonghong Song <yhs@...com>
To:     Tyler Wear <quic_twear@...cinc.com>, <netdev@...r.kernel.org>,
        <bpf@...r.kernel.org>
CC:     <kafai@...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.