netdev - RE: [PATCH bpf-next v5 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: <651b982a1b22a_4fa3f20854@john.notmuch>
Date: Mon, 02 Oct 2023 21:27:22 -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 v5 1/7] bpf, sockmap: add BPF_F_PERMANENT 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_PERMANENT flag to bpf_msg_redirect_map() and
> bpf_msg_redirect_hash() to implement this ability.
> 
> Then we can enable this function in the bpf program as follows:
> bpf_msg_redirect_hash(xx, xx, xx, BPF_F_INGRESS | BPF_F_PERMANENT);
> 
> 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
> 
> Signed-off-by: Liu Jian <liujian56@...wei.com>
> ---

First sorry for the delay I was thinking about this a bit. I decided it likely makes
a lot of sense if you want to build an l7 load balancer where you just read some
keys off an initial msg and then pin the rest of the connection to a specific
backend or proxy socket, etc.

>  include/linux/skmsg.h          |  1 +
>  include/uapi/linux/bpf.h       | 45 ++++++++++++++++++++++++++--------
>  net/core/skmsg.c               |  6 ++++-
>  net/core/sock_map.c            |  4 +--
>  net/ipv4/tcp_bpf.c             | 12 +++++----
>  tools/include/uapi/linux/bpf.h | 45 ++++++++++++++++++++++++++--------
>  6 files changed, 85 insertions(+), 28 deletions(-)
> 
> diff --git a/include/linux/skmsg.h b/include/linux/skmsg.h
> index c1637515a8a4..acd7de85608b 100644
> --- a/include/linux/skmsg.h
> +++ b/include/linux/skmsg.h
> @@ -83,6 +83,7 @@ struct sk_psock {
>  	u32				cork_bytes;
>  	u32				eval;
>  	bool				redir_ingress; /* undefined if sk_redir is null */
> +	bool				redir_permanent;
>  	struct sk_msg			*cork;
>  	struct sk_psock_progs		progs;
>  #if IS_ENABLED(CONFIG_BPF_STREAM_PARSER)
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 70bfa997e896..cec6c34f4486 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -3029,11 +3029,23 @@ union bpf_attr {
>   * 		socket level. If the message *msg* is allowed to pass (i.e. if
>   * 		the verdict eBPF program returns **SK_PASS**), redirect it to
>   * 		the socket referenced by *map* (of type
> - * 		**BPF_MAP_TYPE_SOCKMAP**) at index *key*. Both ingress and
> - * 		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.
> + * 		**BPF_MAP_TYPE_SOCKMAP**) at index *key*.
> + *
> + *		The following *flags* are supported:
> + *
> + *		**BPF_F_INGRESS**
> + *		        Both ingress and 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.
> + *		**BPF_F_PERMANENT**
> + *		        Indicates that redirect verdict and the target socket should be
> + *		        remembered. The verdict program will not be run for subsequent
> + *		        packets, unless an error occurs when forwarding packets.

Why clear it on error? The error is almost always because either the socket is
being torn down or there is a memory ENOMEM error that is going to be kicked
back to user.

Is the idea that you can try anopther backend possibly by picking another socket
out of the table? But, I'm not sure how that might work because you probably
don't know that this is even the beginning of a msg block.

I'm wondering if I missed some other reason or if its just simpler to pass the
error up the stack and keep the same sk_redir.

Thanks,
John