netdev - RE: [PATCH bpf-next v2 1/7] bpf, sockmap: add BPF_F

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: <64ddba9e1df57_32c0720898@john.notmuch>
Date: Wed, 16 Aug 2023 23:13:50 -0700
From: John Fastabend <john.fastabend@...il.com>
To: Liu Jian <liujian56@...wei.com>, 
 john.fastabend@...il.com, 
 jakub@...udflare.com, 
 ast@...nel.org, 
 daniel@...earbox.net, 
 andrii@...nel.org, 
 martin.lau@...ux.dev, 
 song@...nel.org, 
 yonghong.song@...ux.dev, 
 kpsingh@...nel.org, 
 sdf@...gle.com, 
 haoluo@...gle.com, 
 jolsa@...nel.org, 
 davem@...emloft.net, 
 edumazet@...gle.com, 
 kuba@...nel.org, 
 pabeni@...hat.com, 
 dsahern@...nel.org
Cc: netdev@...r.kernel.org, 
 bpf@...r.kernel.org, 
 liujian56@...wei.com
Subject: RE: [PATCH bpf-next v2 1/7] bpf, sockmap: add BPF_F_PERMANENTLY flag
 for skmsg redirect

Liu Jian wrote:
> If the sockmap msg redirection function is used only to forward packets
> and no other operation, the execution result of the BPF_SK_MSG_VERDICT
> program is the same each time. In this case, the BPF program only needs to
> be run once. Add BPF_F_PERMANENTLY flag to bpf_msg_redirect_map() and
> bpf_msg_redirect_hash() to implement this ability.
> 

I like the use case. Did you consider using

 long bpf_msg_apply_bytes(struct sk_msg_buff *msg, u32 bytes)

This could be set to UINT32_MAX and then the BPF prog would only be run
every 0xfffffff bytes.

> Then we can enable this function in the bpf program as follows:
> bpf_msg_redirect_hash(xx, xx, xx, BPF_F_INGRESS | BPF_F_PERMANENTLY);
> 
> Test results using netperf  TCP_STREAM mode:
> for i in 1 64 128 512 1k 2k 32k 64k 100k 500k 1m;then
> netperf -T 1,2 -t TCP_STREAM -H 127.0.0.1 -l 20 -- -m $i -s 100m,100m -S 100m,100m
> done
> 
> before:
> 3.84 246.52 496.89 1885.03 3415.29 6375.03 40749.09 48764.40 51611.34 55678.26 55992.78
> after:
> 4.43 279.20 555.82 2080.79 3870.70 7105.44 41836.41 49709.75 51861.56 55211.00 54566.85

I suspect comparing against

  bpf_msg_redirect_hash(...)
  bpf_msg_apply_bytes(msg, UINT32_MAX)

the diff will be rather small. I agree the API is nicer though to simply
set the flag. Its too bad we didn't think to add a forever to apply_bytes.
I would prefer this API for example,

  bpf_msg_redirect_hash(...)
  bpf_msg_apply_bytes(msg, 0, PERMANENT);

Given we have apply_bytes is it still useful to have a PERMANENT flag
in your use case? Here we would just reset to UNINT32_MAX if we reached
max bytes.

> 
> Signed-off-by: Liu Jian <liujian56@...wei.com>
> ---
>  include/linux/skmsg.h          |  1 +
>  include/uapi/linux/bpf.h       |  7 +++++--
>  net/core/skmsg.c               |  1 +
>  net/core/sock_map.c            |  4 ++--
>  net/ipv4/tcp_bpf.c             | 21 +++++++++++++++------
>  tools/include/uapi/linux/bpf.h |  7 +++++--
>  6 files changed, 29 insertions(+), 12 deletions(-)

[...]

>  
> diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> index 70da85200695..cf622ea4f018 100644
> --- a/tools/include/uapi/linux/bpf.h
> +++ b/tools/include/uapi/linux/bpf.h
> @@ -3004,7 +3004,8 @@ union bpf_attr {
>   * 		egress interfaces can be used for redirection. The
>   * 		**BPF_F_INGRESS** value in *flags* is used to make the
>   * 		distinction (ingress path is selected if the flag is present,
> - * 		egress path otherwise). This is the only flag supported for now.
> + * 		egress path otherwise). The **BPF_F_PERMANENTLY** value in
> + *		*flags* is used to indicates whether the eBPF result is permanent.

We at least need to document what happens if PERMANENTLY and apply_bytes are
used together.

>   * 	Return
>   * 		**SK_PASS** on success, or **SK_DROP** on error.
>   *