From: Jakub Sitnicki <jakub@cloudflare.com>
To: Cong Wang <xiyou.wangcong@gmail.com>
Cc: netdev@vger.kernel.org, bpf@vger.kernel.org,
duanxiongchun@bytedance.com, wangdongdong.6@bytedance.com,
jiang.wang@bytedance.com, Cong Wang <cong.wang@bytedance.com>,
Daniel Borkmann <daniel@iogearbox.net>,
Lorenz Bauer <lmb@cloudflare.com>,
John Fastabend <john.fastabend@gmail.com>
Subject: Re: [Patch bpf-next v6 4/8] skmsg: move sk_redir from TCP_SKB_CB to skb
Date: Mon, 22 Feb 2021 13:20:55 +0100 [thread overview]
Message-ID: <87eeh847ko.fsf@cloudflare.com> (raw)
In-Reply-To: <20210220052924.106599-5-xiyou.wangcong@gmail.com>
On Sat, Feb 20, 2021 at 06:29 AM CET, Cong Wang wrote:
> From: Cong Wang <cong.wang@bytedance.com>
>
> Currently TCP_SKB_CB() is hard-coded in skmsg code, it certainly
> does not work for any other non-TCP protocols. We can move them to
> skb ext, but it introduces a memory allocation on fast path.
>
> Fortunately, we only need to a word-size to store all the information,
> because the flags actually only contains 1 bit so can be just packed
> into the lowest bit of the "pointer", which is stored as unsigned
> long.
>
> Inside struct sk_buff, '_skb_refdst' can be reused because skb dst is
> no longer needed after ->sk_data_ready() so we can just drop it.
>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> Cc: Jakub Sitnicki <jakub@cloudflare.com>
> Cc: Lorenz Bauer <lmb@cloudflare.com>
> Acked-by: John Fastabend <john.fastabend@gmail.com>
> Signed-off-by: Cong Wang <cong.wang@bytedance.com>
> ---
LGTM. I have some questions (below) that would help me confirm if I
understand the changes, and what could be improved, if anything.
Acked-by: Jakub Sitnicki <jakub@cloudflare.com>
> include/linux/skbuff.h | 3 +++
> include/linux/skmsg.h | 35 +++++++++++++++++++++++++++++++++++
> include/net/tcp.h | 19 -------------------
> net/core/skmsg.c | 32 ++++++++++++++++++++------------
> net/core/sock_map.c | 8 ++------
> 5 files changed, 60 insertions(+), 37 deletions(-)
>
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 6d0a33d1c0db..bd84f799c952 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -755,6 +755,9 @@ struct sk_buff {
> void (*destructor)(struct sk_buff *skb);
> };
> struct list_head tcp_tsorted_anchor;
> +#ifdef CONFIG_NET_SOCK_MSG
> + unsigned long _sk_redir;
> +#endif
> };
>
> #if defined(CONFIG_NF_CONNTRACK) || defined(CONFIG_NF_CONNTRACK_MODULE)
> diff --git a/include/linux/skmsg.h b/include/linux/skmsg.h
> index e3bb712af257..fc234d507fd7 100644
> --- a/include/linux/skmsg.h
> +++ b/include/linux/skmsg.h
> @@ -459,4 +459,39 @@ static inline bool sk_psock_strp_enabled(struct sk_psock *psock)
> return false;
> return !!psock->saved_data_ready;
> }
> +
> +#if IS_ENABLED(CONFIG_NET_SOCK_MSG)
> +static inline bool skb_bpf_ingress(const struct sk_buff *skb)
> +{
> + unsigned long sk_redir = skb->_sk_redir;
> +
> + return sk_redir & BPF_F_INGRESS;
> +}
> +
> +static inline void skb_bpf_set_ingress(struct sk_buff *skb)
> +{
> + skb->_sk_redir |= BPF_F_INGRESS;
> +}
> +
> +static inline void skb_bpf_set_redir(struct sk_buff *skb, struct sock *sk_redir,
> + bool ingress)
> +{
> + skb->_sk_redir = (unsigned long)sk_redir;
> + if (ingress)
> + skb->_sk_redir |= BPF_F_INGRESS;
> +}
> +
> +static inline struct sock *skb_bpf_redirect_fetch(const struct sk_buff *skb)
> +{
> + unsigned long sk_redir = skb->_sk_redir;
> +
> + sk_redir &= ~0x1UL;
We're using the enum when setting the bit flag, but a hardcoded constant
when masking it. ~BPF_F_INGRESS would be more consistent here.
> + return (struct sock *)sk_redir;
> +}
> +
> +static inline void skb_bpf_redirect_clear(struct sk_buff *skb)
> +{
> + skb->_sk_redir = 0;
> +}
> +#endif /* CONFIG_NET_SOCK_MSG */
> #endif /* _LINUX_SKMSG_H */
> diff --git a/include/net/tcp.h b/include/net/tcp.h
> index 947ef5da6867..075de26f449d 100644
> --- a/include/net/tcp.h
> +++ b/include/net/tcp.h
> @@ -883,30 +883,11 @@ struct tcp_skb_cb {
> struct inet6_skb_parm h6;
> #endif
> } header; /* For incoming skbs */
> - struct {
> - __u32 flags;
> - struct sock *sk_redir;
> - } bpf;
> };
> };
>
> #define TCP_SKB_CB(__skb) ((struct tcp_skb_cb *)&((__skb)->cb[0]))
>
> -static inline bool tcp_skb_bpf_ingress(const struct sk_buff *skb)
> -{
> - return TCP_SKB_CB(skb)->bpf.flags & BPF_F_INGRESS;
> -}
> -
> -static inline struct sock *tcp_skb_bpf_redirect_fetch(struct sk_buff *skb)
> -{
> - return TCP_SKB_CB(skb)->bpf.sk_redir;
> -}
> -
> -static inline void tcp_skb_bpf_redirect_clear(struct sk_buff *skb)
> -{
> - TCP_SKB_CB(skb)->bpf.sk_redir = NULL;
> -}
> -
> extern const struct inet_connection_sock_af_ops ipv4_specific;
>
> #if IS_ENABLED(CONFIG_IPV6)
> diff --git a/net/core/skmsg.c b/net/core/skmsg.c
> index 2d8bbb3fd87c..05b5af09ff42 100644
> --- a/net/core/skmsg.c
> +++ b/net/core/skmsg.c
> @@ -494,6 +494,8 @@ static int sk_psock_skb_ingress_self(struct sk_psock *psock, struct sk_buff *skb
> static int sk_psock_handle_skb(struct sk_psock *psock, struct sk_buff *skb,
> u32 off, u32 len, bool ingress)
> {
> + skb_bpf_redirect_clear(skb);
This is called to avoid leaking state in skb->_skb_refdst. Correct?
I'm wondering why we're doing it every time sk_psock_handle_skb() gets
invoked from the do/while loop in sk_psock_backlog(), instead of doing
it once after reading ingress flag with skb_bpf_ingress()?
> +
> if (!ingress) {
> if (!sock_writeable(psock->sk))
> return -EAGAIN;
> @@ -525,7 +527,7 @@ static void sk_psock_backlog(struct work_struct *work)
> len = skb->len;
> off = 0;
> start:
> - ingress = tcp_skb_bpf_ingress(skb);
> + ingress = skb_bpf_ingress(skb);
> do {
> ret = -EIO;
> if (likely(psock->sk->sk_socket))
> @@ -631,7 +633,12 @@ void __sk_psock_purge_ingress_msg(struct sk_psock *psock)
>
> static void sk_psock_zap_ingress(struct sk_psock *psock)
> {
> - __skb_queue_purge(&psock->ingress_skb);
> + struct sk_buff *skb;
> +
> + while ((skb = __skb_dequeue(&psock->ingress_skb)) != NULL) {
> + skb_bpf_redirect_clear(skb);
I believe we clone the skb before enqueuing it psock->ingress_skb.
Clone happens either in sk_psock_verdict_recv() or in __strp_recv().
There are not other users holding a ref, so clearing the redirect seems
unneeded. Unless I'm missing something?
> + kfree_skb(skb);
> + }
> __sk_psock_purge_ingress_msg(psock);
> }
>
> @@ -752,7 +759,7 @@ static void sk_psock_skb_redirect(struct sk_buff *skb)
> struct sk_psock *psock_other;
> struct sock *sk_other;
>
> - sk_other = tcp_skb_bpf_redirect_fetch(skb);
> + sk_other = skb_bpf_redirect_fetch(skb);
> /* This error is a buggy BPF program, it returned a redirect
> * return code, but then didn't set a redirect interface.
> */
> @@ -802,9 +809,10 @@ int sk_psock_tls_strp_read(struct sk_psock *psock, struct sk_buff *skb)
> * TLS context.
> */
> skb->sk = psock->sk;
> - tcp_skb_bpf_redirect_clear(skb);
> + skb_dst_drop(skb);
> + skb_bpf_redirect_clear(skb);
After skb_dst_drop(), skb->_skb_refdst is clear. So it seems the
redirect_clear() is not needed. But I'm guessing it is being invoked
to communicate the intention?
> ret = sk_psock_bpf_run(psock, prog, skb);
> - ret = sk_psock_map_verd(ret, tcp_skb_bpf_redirect_fetch(skb));
> + ret = sk_psock_map_verd(ret, skb_bpf_redirect_fetch(skb));
> skb->sk = NULL;
> }
> sk_psock_tls_verdict_apply(skb, psock->sk, ret);
> @@ -816,7 +824,6 @@ EXPORT_SYMBOL_GPL(sk_psock_tls_strp_read);
> static void sk_psock_verdict_apply(struct sk_psock *psock,
> struct sk_buff *skb, int verdict)
> {
> - struct tcp_skb_cb *tcp;
> struct sock *sk_other;
> int err = -EIO;
>
> @@ -828,8 +835,7 @@ static void sk_psock_verdict_apply(struct sk_psock *psock,
> goto out_free;
> }
>
> - tcp = TCP_SKB_CB(skb);
> - tcp->bpf.flags |= BPF_F_INGRESS;
> + skb_bpf_set_ingress(skb);
>
> /* If the queue is empty then we can submit directly
> * into the msg queue. If its not empty we have to
> @@ -890,9 +896,10 @@ static void sk_psock_strp_read(struct strparser *strp, struct sk_buff *skb)
> skb_set_owner_r(skb, sk);
> prog = READ_ONCE(psock->progs.skb_verdict);
> if (likely(prog)) {
> - tcp_skb_bpf_redirect_clear(skb);
> + skb_dst_drop(skb);
> + skb_bpf_redirect_clear(skb);
> ret = sk_psock_bpf_run(psock, prog, skb);
> - ret = sk_psock_map_verd(ret, tcp_skb_bpf_redirect_fetch(skb));
> + ret = sk_psock_map_verd(ret, skb_bpf_redirect_fetch(skb));
> }
> sk_psock_verdict_apply(psock, skb, ret);
> out:
> @@ -1005,9 +1012,10 @@ static int sk_psock_verdict_recv(read_descriptor_t *desc, struct sk_buff *skb,
> skb_set_owner_r(skb, sk);
> prog = READ_ONCE(psock->progs.skb_verdict);
> if (likely(prog)) {
> - tcp_skb_bpf_redirect_clear(skb);
> + skb_dst_drop(skb);
> + skb_bpf_redirect_clear(skb);
> ret = sk_psock_bpf_run(psock, prog, skb);
> - ret = sk_psock_map_verd(ret, tcp_skb_bpf_redirect_fetch(skb));
> + ret = sk_psock_map_verd(ret, skb_bpf_redirect_fetch(skb));
> }
> sk_psock_verdict_apply(psock, skb, ret);
> out:
> diff --git a/net/core/sock_map.c b/net/core/sock_map.c
> index 1a28a5c2c61e..dbfcd7006338 100644
> --- a/net/core/sock_map.c
> +++ b/net/core/sock_map.c
> @@ -657,7 +657,6 @@ const struct bpf_func_proto bpf_sock_map_update_proto = {
> BPF_CALL_4(bpf_sk_redirect_map, struct sk_buff *, skb,
> struct bpf_map *, map, u32, key, u64, flags)
> {
> - struct tcp_skb_cb *tcb = TCP_SKB_CB(skb);
> struct sock *sk;
>
> if (unlikely(flags & ~(BPF_F_INGRESS)))
> @@ -667,8 +666,7 @@ BPF_CALL_4(bpf_sk_redirect_map, struct sk_buff *, skb,
> if (unlikely(!sk || !sock_map_redirect_allowed(sk)))
> return SK_DROP;
>
> - tcb->bpf.flags = flags;
> - tcb->bpf.sk_redir = sk;
> + skb_bpf_set_redir(skb, sk, flags & BPF_F_INGRESS);
> return SK_PASS;
> }
>
> @@ -1250,7 +1248,6 @@ const struct bpf_func_proto bpf_sock_hash_update_proto = {
> BPF_CALL_4(bpf_sk_redirect_hash, struct sk_buff *, skb,
> struct bpf_map *, map, void *, key, u64, flags)
> {
> - struct tcp_skb_cb *tcb = TCP_SKB_CB(skb);
> struct sock *sk;
>
> if (unlikely(flags & ~(BPF_F_INGRESS)))
> @@ -1260,8 +1257,7 @@ BPF_CALL_4(bpf_sk_redirect_hash, struct sk_buff *, skb,
> if (unlikely(!sk || !sock_map_redirect_allowed(sk)))
> return SK_DROP;
>
> - tcb->bpf.flags = flags;
> - tcb->bpf.sk_redir = sk;
> + skb_bpf_set_redir(skb, sk, flags & BPF_F_INGRESS);
> return SK_PASS;
> }
next prev parent reply other threads:[~2021-02-22 12:23 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-02-20 5:29 [Patch bpf-next v6 0/8] sock_map: clean up and refactor code for BPF_SK_SKB_VERDICT Cong Wang
2021-02-20 5:29 ` [Patch bpf-next v6 1/8] bpf: clean up sockmap related Kconfigs Cong Wang
2021-02-22 8:51 ` Jakub Sitnicki
2021-02-22 23:23 ` Cong Wang
2021-02-20 5:29 ` [Patch bpf-next v6 2/8] skmsg: get rid of struct sk_psock_parser Cong Wang
2021-02-20 5:29 ` [Patch bpf-next v6 3/8] bpf: compute data_end dynamically with JIT code Cong Wang
2021-02-20 5:29 ` [Patch bpf-next v6 4/8] skmsg: move sk_redir from TCP_SKB_CB to skb Cong Wang
2021-02-22 12:20 ` Jakub Sitnicki [this message]
2021-02-22 19:27 ` Cong Wang
2021-02-23 17:52 ` Jakub Sitnicki
2021-02-23 18:04 ` Cong Wang
2021-02-23 18:36 ` Jakub Sitnicki
2021-02-20 5:29 ` [Patch bpf-next v6 5/8] sock_map: rename skb_parser and skb_verdict Cong Wang
2021-02-22 12:28 ` Jakub Sitnicki
2021-02-22 19:32 ` Cong Wang
2021-02-20 5:29 ` [Patch bpf-next v6 6/8] sock_map: make sock_map_prog_update() static Cong Wang
2021-02-22 12:29 ` Jakub Sitnicki
2021-02-20 5:29 ` [Patch bpf-next v6 7/8] skmsg: make __sk_psock_purge_ingress_msg() static Cong Wang
2021-02-22 12:30 ` Jakub Sitnicki
2021-02-20 5:29 ` [Patch bpf-next v6 8/8] skmsg: get rid of sk_psock_bpf_run() Cong Wang
2021-02-22 12:31 ` Jakub Sitnicki
2021-02-22 12:32 ` [Patch bpf-next v6 0/8] sock_map: clean up and refactor code for BPF_SK_SKB_VERDICT Jakub Sitnicki
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87eeh847ko.fsf@cloudflare.com \
--to=jakub@cloudflare.com \
--cc=bpf@vger.kernel.org \
--cc=cong.wang@bytedance.com \
--cc=daniel@iogearbox.net \
--cc=duanxiongchun@bytedance.com \
--cc=jiang.wang@bytedance.com \
--cc=john.fastabend@gmail.com \
--cc=lmb@cloudflare.com \
--cc=netdev@vger.kernel.org \
--cc=wangdongdong.6@bytedance.com \
--cc=xiyou.wangcong@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.