* [Patch bpf-next v4 1/5] bpf: clean up sockmap related Kconfigs
2021-02-16 6:42 [Patch bpf-next v4 0/5] sock_map: clean up and refactor code for BPF_SK_SKB_VERDICT Cong Wang
@ 2021-02-16 6:42 ` Cong Wang
2021-02-19 18:25 ` Jakub Sitnicki
2021-02-16 6:42 ` [Patch bpf-next v4 2/5] skmsg: get rid of struct sk_psock_parser Cong Wang
` (3 subsequent siblings)
4 siblings, 1 reply; 12+ messages in thread
From: Cong Wang @ 2021-02-16 6:42 UTC (permalink / raw)
To: netdev
Cc: bpf, duanxiongchun, wangdongdong.6, jiang.wang, Cong Wang,
Daniel Borkmann, Jakub Sitnicki, Lorenz Bauer, John Fastabend
From: Cong Wang <cong.wang@bytedance.com>
As suggested by John, clean up sockmap related Kconfigs:
Reduce the scope of CONFIG_BPF_STREAM_PARSER down to TCP stream
parser, to reflect its name.
Make the rest sockmap code simply depend on CONFIG_BPF_SYSCALL.
And leave CONFIG_NET_SOCK_MSG untouched, as it is used by
non-sockmap cases.
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Jakub Sitnicki <jakub@cloudflare.com>
Reviewed-by: Lorenz Bauer <lmb@cloudflare.com>
Acked-by: John Fastabend <john.fastabend@gmail.com>
Signed-off-by: Cong Wang <cong.wang@bytedance.com>
---
include/linux/bpf.h | 20 +++---
include/linux/bpf_types.h | 2 -
include/linux/skmsg.h | 22 ++++++
include/net/tcp.h | 13 ++--
include/net/udp.h | 4 +-
init/Kconfig | 1 +
net/Kconfig | 6 +-
net/core/Makefile | 2 +-
net/core/skmsg.c | 139 ++++++++++++++++++++------------------
net/core/sock_map.c | 2 +
net/ipv4/Makefile | 2 +-
net/ipv4/tcp_bpf.c | 4 +-
net/ipv4/udp_bpf.c | 2 +
13 files changed, 121 insertions(+), 98 deletions(-)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index cccaef1088ea..e29600f01585 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1741,6 +1741,14 @@ static inline bool bpf_map_is_dev_bound(struct bpf_map *map)
struct bpf_map *bpf_map_offload_map_alloc(union bpf_attr *attr);
void bpf_map_offload_map_free(struct bpf_map *map);
+
+int sock_map_prog_update(struct bpf_map *map, struct bpf_prog *prog,
+ struct bpf_prog *old, u32 which);
+int sock_map_get_from_fd(const union bpf_attr *attr, struct bpf_prog *prog);
+int sock_map_prog_detach(const union bpf_attr *attr, enum bpf_prog_type ptype);
+int sock_map_update_elem_sys(struct bpf_map *map, void *key, void *value, u64 flags);
+void sock_map_unhash(struct sock *sk);
+void sock_map_close(struct sock *sk, long timeout);
#else
static inline int bpf_prog_offload_init(struct bpf_prog *prog,
union bpf_attr *attr)
@@ -1766,17 +1774,7 @@ static inline struct bpf_map *bpf_map_offload_map_alloc(union bpf_attr *attr)
static inline void bpf_map_offload_map_free(struct bpf_map *map)
{
}
-#endif /* CONFIG_NET && CONFIG_BPF_SYSCALL */
-#if defined(CONFIG_BPF_STREAM_PARSER)
-int sock_map_prog_update(struct bpf_map *map, struct bpf_prog *prog,
- struct bpf_prog *old, u32 which);
-int sock_map_get_from_fd(const union bpf_attr *attr, struct bpf_prog *prog);
-int sock_map_prog_detach(const union bpf_attr *attr, enum bpf_prog_type ptype);
-int sock_map_update_elem_sys(struct bpf_map *map, void *key, void *value, u64 flags);
-void sock_map_unhash(struct sock *sk);
-void sock_map_close(struct sock *sk, long timeout);
-#else
static inline int sock_map_prog_update(struct bpf_map *map,
struct bpf_prog *prog,
struct bpf_prog *old, u32 which)
@@ -1801,7 +1799,7 @@ static inline int sock_map_update_elem_sys(struct bpf_map *map, void *key, void
{
return -EOPNOTSUPP;
}
-#endif /* CONFIG_BPF_STREAM_PARSER */
+#endif /* CONFIG_NET && CONFIG_BPF_SYSCALL */
#if defined(CONFIG_INET) && defined(CONFIG_BPF_SYSCALL)
void bpf_sk_reuseport_detach(struct sock *sk);
diff --git a/include/linux/bpf_types.h b/include/linux/bpf_types.h
index 99f7fd657d87..d6b4a657c885 100644
--- a/include/linux/bpf_types.h
+++ b/include/linux/bpf_types.h
@@ -103,10 +103,8 @@ BPF_MAP_TYPE(BPF_MAP_TYPE_HASH_OF_MAPS, htab_of_maps_map_ops)
BPF_MAP_TYPE(BPF_MAP_TYPE_DEVMAP, dev_map_ops)
BPF_MAP_TYPE(BPF_MAP_TYPE_DEVMAP_HASH, dev_map_hash_ops)
BPF_MAP_TYPE(BPF_MAP_TYPE_SK_STORAGE, sk_storage_map_ops)
-#if defined(CONFIG_BPF_STREAM_PARSER)
BPF_MAP_TYPE(BPF_MAP_TYPE_SOCKMAP, sock_map_ops)
BPF_MAP_TYPE(BPF_MAP_TYPE_SOCKHASH, sock_hash_ops)
-#endif
#ifdef CONFIG_BPF_LSM
BPF_MAP_TYPE(BPF_MAP_TYPE_INODE_STORAGE, inode_storage_map_ops)
BPF_MAP_TYPE(BPF_MAP_TYPE_TASK_STORAGE, task_storage_map_ops)
diff --git a/include/linux/skmsg.h b/include/linux/skmsg.h
index 8edbbf5f2f93..041faef00937 100644
--- a/include/linux/skmsg.h
+++ b/include/linux/skmsg.h
@@ -71,7 +71,9 @@ struct sk_psock_link {
};
struct sk_psock_parser {
+#if IS_ENABLED(CONFIG_BPF_STREAM_PARSER)
struct strparser strp;
+#endif
bool enabled;
void (*saved_data_ready)(struct sock *sk);
};
@@ -305,9 +307,29 @@ static inline void sk_psock_report_error(struct sk_psock *psock, int err)
struct sk_psock *sk_psock_init(struct sock *sk, int node);
+#if IS_ENABLED(CONFIG_BPF_STREAM_PARSER)
int sk_psock_init_strp(struct sock *sk, struct sk_psock *psock);
void sk_psock_start_strp(struct sock *sk, struct sk_psock *psock);
void sk_psock_stop_strp(struct sock *sk, struct sk_psock *psock);
+void sk_psock_done_strp(struct sk_psock *psock);
+#else
+static inline int sk_psock_init_strp(struct sock *sk, struct sk_psock *psock)
+{
+ return -EOPNOTSUPP;
+}
+
+static inline void sk_psock_start_strp(struct sock *sk, struct sk_psock *psock)
+{
+}
+
+static inline void sk_psock_stop_strp(struct sock *sk, struct sk_psock *psock)
+{
+}
+
+static inline void sk_psock_done_strp(struct sk_psock *psock)
+{
+}
+#endif
void sk_psock_start_verdict(struct sock *sk, struct sk_psock *psock);
void sk_psock_stop_verdict(struct sock *sk, struct sk_psock *psock);
diff --git a/include/net/tcp.h b/include/net/tcp.h
index 4bb42fb19711..99fdbf03aeee 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -2204,23 +2204,20 @@ void tcp_update_ulp(struct sock *sk, struct proto *p,
__MODULE_INFO(alias, alias_userspace, name); \
__MODULE_INFO(alias, alias_tcp_ulp, "tcp-ulp-" name)
+#ifdef CONFIG_NET_SOCK_MSG
struct sk_msg;
struct sk_psock;
-
-#ifdef CONFIG_BPF_STREAM_PARSER
struct proto *tcp_bpf_get_proto(struct sock *sk, struct sk_psock *psock);
void tcp_bpf_clone(const struct sock *sk, struct sock *newsk);
-#else
-static inline void tcp_bpf_clone(const struct sock *sk, struct sock *newsk)
-{
-}
-#endif /* CONFIG_BPF_STREAM_PARSER */
-#ifdef CONFIG_NET_SOCK_MSG
int tcp_bpf_sendmsg_redir(struct sock *sk, struct sk_msg *msg, u32 bytes,
int flags);
int __tcp_bpf_recvmsg(struct sock *sk, struct sk_psock *psock,
struct msghdr *msg, int len, int flags);
+#else
+static inline void tcp_bpf_clone(const struct sock *sk, struct sock *newsk)
+{
+}
#endif /* CONFIG_NET_SOCK_MSG */
#ifdef CONFIG_CGROUP_BPF
diff --git a/include/net/udp.h b/include/net/udp.h
index 877832bed471..fcc73cc50007 100644
--- a/include/net/udp.h
+++ b/include/net/udp.h
@@ -511,9 +511,9 @@ static inline struct sk_buff *udp_rcv_segment(struct sock *sk,
return segs;
}
-#ifdef CONFIG_BPF_STREAM_PARSER
+#ifdef CONFIG_NET_SOCK_MSG
struct sk_psock;
struct proto *udp_bpf_get_proto(struct sock *sk, struct sk_psock *psock);
-#endif /* BPF_STREAM_PARSER */
+#endif /* CONFIG_NET_SOCK_MSG */
#endif /* _UDP_H */
diff --git a/init/Kconfig b/init/Kconfig
index b77c60f8b963..163d76019f59 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1703,6 +1703,7 @@ config BPF_SYSCALL
select BPF
select IRQ_WORK
select TASKS_TRACE_RCU
+ select NET_SOCK_MSG if NET
default n
help
Enable the bpf() system call that allows to manipulate eBPF
diff --git a/net/Kconfig b/net/Kconfig
index f4c32d982af6..a4f60d0c630f 100644
--- a/net/Kconfig
+++ b/net/Kconfig
@@ -313,13 +313,9 @@ config BPF_STREAM_PARSER
select STREAM_PARSER
select NET_SOCK_MSG
help
- Enabling this allows a stream parser to be used with
+ Enabling this allows a TCP stream parser to be used with
BPF_MAP_TYPE_SOCKMAP.
- BPF_MAP_TYPE_SOCKMAP provides a map type to use with network sockets.
- It can be used to enforce socket policy, implement socket redirects,
- etc.
-
config NET_FLOW_LIMIT
bool
depends on RPS
diff --git a/net/core/Makefile b/net/core/Makefile
index 3e2c378e5f31..05733689d050 100644
--- a/net/core/Makefile
+++ b/net/core/Makefile
@@ -28,10 +28,10 @@ obj-$(CONFIG_CGROUP_NET_PRIO) += netprio_cgroup.o
obj-$(CONFIG_CGROUP_NET_CLASSID) += netclassid_cgroup.o
obj-$(CONFIG_LWTUNNEL) += lwtunnel.o
obj-$(CONFIG_LWTUNNEL_BPF) += lwt_bpf.o
-obj-$(CONFIG_BPF_STREAM_PARSER) += sock_map.o
obj-$(CONFIG_DST_CACHE) += dst_cache.o
obj-$(CONFIG_HWBM) += hwbm.o
obj-$(CONFIG_NET_DEVLINK) += devlink.o
obj-$(CONFIG_GRO_CELLS) += gro_cells.o
obj-$(CONFIG_FAILOVER) += failover.o
+obj-$(CONFIG_BPF_SYSCALL) += sock_map.o
obj-$(CONFIG_BPF_SYSCALL) += bpf_sk_storage.o
diff --git a/net/core/skmsg.c b/net/core/skmsg.c
index 1261512d6807..6cb5ff6f8f9c 100644
--- a/net/core/skmsg.c
+++ b/net/core/skmsg.c
@@ -651,9 +651,7 @@ static void sk_psock_destroy_deferred(struct work_struct *gc)
/* No sk_callback_lock since already detached. */
- /* Parser has been stopped */
- if (psock->progs.skb_parser)
- strp_done(&psock->parser.strp);
+ sk_psock_done_strp(psock);
cancel_work_sync(&psock->work);
@@ -750,14 +748,6 @@ static int sk_psock_bpf_run(struct sk_psock *psock, struct bpf_prog *prog,
return bpf_prog_run_pin_on_cpu(prog, skb);
}
-static struct sk_psock *sk_psock_from_strp(struct strparser *strp)
-{
- struct sk_psock_parser *parser;
-
- parser = container_of(strp, struct sk_psock_parser, strp);
- return container_of(parser, struct sk_psock, parser);
-}
-
static void sk_psock_skb_redirect(struct sk_buff *skb)
{
struct sk_psock *psock_other;
@@ -866,6 +856,24 @@ static void sk_psock_verdict_apply(struct sk_psock *psock,
}
}
+static void sk_psock_write_space(struct sock *sk)
+{
+ struct sk_psock *psock;
+ void (*write_space)(struct sock *sk) = NULL;
+
+ rcu_read_lock();
+ psock = sk_psock(sk);
+ if (likely(psock)) {
+ if (sk_psock_test_state(psock, SK_PSOCK_TX_ENABLED))
+ schedule_work(&psock->work);
+ write_space = psock->saved_write_space;
+ }
+ rcu_read_unlock();
+ if (write_space)
+ write_space(sk);
+}
+
+#if IS_ENABLED(CONFIG_BPF_STREAM_PARSER)
static void sk_psock_strp_read(struct strparser *strp, struct sk_buff *skb)
{
struct sk_psock *psock;
@@ -897,6 +905,14 @@ static int sk_psock_strp_read_done(struct strparser *strp, int err)
return err;
}
+static struct sk_psock *sk_psock_from_strp(struct strparser *strp)
+{
+ struct sk_psock_parser *parser;
+
+ parser = container_of(strp, struct sk_psock_parser, strp);
+ return container_of(parser, struct sk_psock, parser);
+}
+
static int sk_psock_strp_parse(struct strparser *strp, struct sk_buff *skb)
{
struct sk_psock *psock = sk_psock_from_strp(strp);
@@ -933,6 +949,52 @@ static void sk_psock_strp_data_ready(struct sock *sk)
rcu_read_unlock();
}
+int sk_psock_init_strp(struct sock *sk, struct sk_psock *psock)
+{
+ static const struct strp_callbacks cb = {
+ .rcv_msg = sk_psock_strp_read,
+ .read_sock_done = sk_psock_strp_read_done,
+ .parse_msg = sk_psock_strp_parse,
+ };
+
+ psock->parser.enabled = false;
+ return strp_init(&psock->parser.strp, sk, &cb);
+}
+
+void sk_psock_start_strp(struct sock *sk, struct sk_psock *psock)
+{
+ struct sk_psock_parser *parser = &psock->parser;
+
+ if (parser->enabled)
+ return;
+
+ parser->saved_data_ready = sk->sk_data_ready;
+ sk->sk_data_ready = sk_psock_strp_data_ready;
+ sk->sk_write_space = sk_psock_write_space;
+ parser->enabled = true;
+}
+
+void sk_psock_stop_strp(struct sock *sk, struct sk_psock *psock)
+{
+ struct sk_psock_parser *parser = &psock->parser;
+
+ if (!parser->enabled)
+ return;
+
+ sk->sk_data_ready = parser->saved_data_ready;
+ parser->saved_data_ready = NULL;
+ strp_stop(&parser->strp);
+ parser->enabled = false;
+}
+
+void sk_psock_done_strp(struct sk_psock *psock)
+{
+ /* Parser has been stopped */
+ if (psock->progs.skb_parser)
+ strp_done(&psock->parser.strp);
+}
+#endif
+
static int sk_psock_verdict_recv(read_descriptor_t *desc, struct sk_buff *skb,
unsigned int offset, size_t orig_len)
{
@@ -984,35 +1046,6 @@ static void sk_psock_verdict_data_ready(struct sock *sk)
sock->ops->read_sock(sk, &desc, sk_psock_verdict_recv);
}
-static void sk_psock_write_space(struct sock *sk)
-{
- struct sk_psock *psock;
- void (*write_space)(struct sock *sk) = NULL;
-
- rcu_read_lock();
- psock = sk_psock(sk);
- if (likely(psock)) {
- if (sk_psock_test_state(psock, SK_PSOCK_TX_ENABLED))
- schedule_work(&psock->work);
- write_space = psock->saved_write_space;
- }
- rcu_read_unlock();
- if (write_space)
- write_space(sk);
-}
-
-int sk_psock_init_strp(struct sock *sk, struct sk_psock *psock)
-{
- static const struct strp_callbacks cb = {
- .rcv_msg = sk_psock_strp_read,
- .read_sock_done = sk_psock_strp_read_done,
- .parse_msg = sk_psock_strp_parse,
- };
-
- psock->parser.enabled = false;
- return strp_init(&psock->parser.strp, sk, &cb);
-}
-
void sk_psock_start_verdict(struct sock *sk, struct sk_psock *psock)
{
struct sk_psock_parser *parser = &psock->parser;
@@ -1026,32 +1059,6 @@ void sk_psock_start_verdict(struct sock *sk, struct sk_psock *psock)
parser->enabled = true;
}
-void sk_psock_start_strp(struct sock *sk, struct sk_psock *psock)
-{
- struct sk_psock_parser *parser = &psock->parser;
-
- if (parser->enabled)
- return;
-
- parser->saved_data_ready = sk->sk_data_ready;
- sk->sk_data_ready = sk_psock_strp_data_ready;
- sk->sk_write_space = sk_psock_write_space;
- parser->enabled = true;
-}
-
-void sk_psock_stop_strp(struct sock *sk, struct sk_psock *psock)
-{
- struct sk_psock_parser *parser = &psock->parser;
-
- if (!parser->enabled)
- return;
-
- sk->sk_data_ready = parser->saved_data_ready;
- parser->saved_data_ready = NULL;
- strp_stop(&parser->strp);
- parser->enabled = false;
-}
-
void sk_psock_stop_verdict(struct sock *sk, struct sk_psock *psock)
{
struct sk_psock_parser *parser = &psock->parser;
diff --git a/net/core/sock_map.c b/net/core/sock_map.c
index d758fb83c884..ee3334dd3a38 100644
--- a/net/core/sock_map.c
+++ b/net/core/sock_map.c
@@ -1461,9 +1461,11 @@ int sock_map_prog_update(struct bpf_map *map, struct bpf_prog *prog,
case BPF_SK_MSG_VERDICT:
pprog = &progs->msg_parser;
break;
+#if IS_ENABLED(CONFIG_BPF_STREAM_PARSER)
case BPF_SK_SKB_STREAM_PARSER:
pprog = &progs->skb_parser;
break;
+#endif
case BPF_SK_SKB_STREAM_VERDICT:
pprog = &progs->skb_verdict;
break;
diff --git a/net/ipv4/Makefile b/net/ipv4/Makefile
index 5b77a46885b9..49debfefcc62 100644
--- a/net/ipv4/Makefile
+++ b/net/ipv4/Makefile
@@ -62,7 +62,7 @@ obj-$(CONFIG_TCP_CONG_LP) += tcp_lp.o
obj-$(CONFIG_TCP_CONG_YEAH) += tcp_yeah.o
obj-$(CONFIG_TCP_CONG_ILLINOIS) += tcp_illinois.o
obj-$(CONFIG_NET_SOCK_MSG) += tcp_bpf.o
-obj-$(CONFIG_BPF_STREAM_PARSER) += udp_bpf.o
+obj-$(CONFIG_NET_SOCK_MSG) += udp_bpf.o
obj-$(CONFIG_NETLABEL) += cipso_ipv4.o
obj-$(CONFIG_XFRM) += xfrm4_policy.o xfrm4_state.o xfrm4_input.o \
diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
index bc7d2a586e18..b2c4865eb39b 100644
--- a/net/ipv4/tcp_bpf.c
+++ b/net/ipv4/tcp_bpf.c
@@ -229,7 +229,6 @@ int tcp_bpf_sendmsg_redir(struct sock *sk, struct sk_msg *msg,
}
EXPORT_SYMBOL_GPL(tcp_bpf_sendmsg_redir);
-#ifdef CONFIG_BPF_STREAM_PARSER
static bool tcp_bpf_stream_read(const struct sock *sk)
{
struct sk_psock *psock;
@@ -561,8 +560,10 @@ static void tcp_bpf_rebuild_protos(struct proto prot[TCP_BPF_NUM_CFGS],
struct proto *base)
{
prot[TCP_BPF_BASE] = *base;
+#if defined(CONFIG_BPF_SYSCALL)
prot[TCP_BPF_BASE].unhash = sock_map_unhash;
prot[TCP_BPF_BASE].close = sock_map_close;
+#endif
prot[TCP_BPF_BASE].recvmsg = tcp_bpf_recvmsg;
prot[TCP_BPF_BASE].stream_memory_read = tcp_bpf_stream_read;
@@ -629,4 +630,3 @@ void tcp_bpf_clone(const struct sock *sk, struct sock *newsk)
if (prot == &tcp_bpf_prots[family][TCP_BPF_BASE])
newsk->sk_prot = sk->sk_prot_creator;
}
-#endif /* CONFIG_BPF_STREAM_PARSER */
diff --git a/net/ipv4/udp_bpf.c b/net/ipv4/udp_bpf.c
index 7a94791efc1a..e635ccc175ca 100644
--- a/net/ipv4/udp_bpf.c
+++ b/net/ipv4/udp_bpf.c
@@ -18,8 +18,10 @@ static struct proto udp_bpf_prots[UDP_BPF_NUM_PROTS];
static void udp_bpf_rebuild_protos(struct proto *prot, const struct proto *base)
{
*prot = *base;
+#if defined(CONFIG_BPF_SYSCALL)
prot->unhash = sock_map_unhash;
prot->close = sock_map_close;
+#endif
}
static void udp_bpf_check_v6_needs_rebuild(struct proto *ops)
--
2.25.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [Patch bpf-next v4 1/5] bpf: clean up sockmap related Kconfigs
2021-02-16 6:42 ` [Patch bpf-next v4 1/5] bpf: clean up sockmap related Kconfigs Cong Wang
@ 2021-02-19 18:25 ` Jakub Sitnicki
2021-02-19 18:46 ` Cong Wang
0 siblings, 1 reply; 12+ messages in thread
From: Jakub Sitnicki @ 2021-02-19 18:25 UTC (permalink / raw)
To: Cong Wang
Cc: netdev, bpf, duanxiongchun, wangdongdong.6, jiang.wang,
Cong Wang, Daniel Borkmann, Lorenz Bauer, John Fastabend
On Tue, Feb 16, 2021 at 07:42 AM CET, Cong Wang wrote:
> From: Cong Wang <cong.wang@bytedance.com>
>
> As suggested by John, clean up sockmap related Kconfigs:
>
> Reduce the scope of CONFIG_BPF_STREAM_PARSER down to TCP stream
> parser, to reflect its name.
>
> Make the rest sockmap code simply depend on CONFIG_BPF_SYSCALL.
> And leave CONFIG_NET_SOCK_MSG untouched, as it is used by
> non-sockmap cases.
>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> Cc: Jakub Sitnicki <jakub@cloudflare.com>
> Reviewed-by: Lorenz Bauer <lmb@cloudflare.com>
> Acked-by: John Fastabend <john.fastabend@gmail.com>
> Signed-off-by: Cong Wang <cong.wang@bytedance.com>
> ---
Sorry for the delay. There's a lot happening here. Took me a while to
dig through it.
I have a couple of nit-picks, which easily can be addressed as
follow-ups, and one comment.
sock_map_prog_update and sk_psock_done_strp are only used in
net/core/sock_map.c and can be static.
[...]
> diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
> index bc7d2a586e18..b2c4865eb39b 100644
> --- a/net/ipv4/tcp_bpf.c
> +++ b/net/ipv4/tcp_bpf.c
> @@ -229,7 +229,6 @@ int tcp_bpf_sendmsg_redir(struct sock *sk, struct sk_msg *msg,
> }
> EXPORT_SYMBOL_GPL(tcp_bpf_sendmsg_redir);
>
> -#ifdef CONFIG_BPF_STREAM_PARSER
> static bool tcp_bpf_stream_read(const struct sock *sk)
> {
> struct sk_psock *psock;
> @@ -561,8 +560,10 @@ static void tcp_bpf_rebuild_protos(struct proto prot[TCP_BPF_NUM_CFGS],
> struct proto *base)
> {
> prot[TCP_BPF_BASE] = *base;
> +#if defined(CONFIG_BPF_SYSCALL)
> prot[TCP_BPF_BASE].unhash = sock_map_unhash;
> prot[TCP_BPF_BASE].close = sock_map_close;
> +#endif
> prot[TCP_BPF_BASE].recvmsg = tcp_bpf_recvmsg;
> prot[TCP_BPF_BASE].stream_memory_read = tcp_bpf_stream_read;
>
> @@ -629,4 +630,3 @@ void tcp_bpf_clone(const struct sock *sk, struct sock *newsk)
> if (prot == &tcp_bpf_prots[family][TCP_BPF_BASE])
> newsk->sk_prot = sk->sk_prot_creator;
> }
> -#endif /* CONFIG_BPF_STREAM_PARSER */
net/core/sock_map.o now is built only when CONFIG_BPF_SYSCALL is set.
While tcp_bpf_get_proto is only called from net/core/sock_map.o.
Seems there is no sense in compiling tcp_bpf_get_proto, and everything
it depends on which was enclosed by CONFIG_BPF_STREAM_PARSER check, when
CONFIG_BPF_SYSCALL is unset.
> diff --git a/net/ipv4/udp_bpf.c b/net/ipv4/udp_bpf.c
> index 7a94791efc1a..e635ccc175ca 100644
> --- a/net/ipv4/udp_bpf.c
> +++ b/net/ipv4/udp_bpf.c
> @@ -18,8 +18,10 @@ static struct proto udp_bpf_prots[UDP_BPF_NUM_PROTS];
> static void udp_bpf_rebuild_protos(struct proto *prot, const struct proto *base)
> {
> *prot = *base;
> +#if defined(CONFIG_BPF_SYSCALL)
> prot->unhash = sock_map_unhash;
> prot->close = sock_map_close;
> +#endif
> }
>
> static void udp_bpf_check_v6_needs_rebuild(struct proto *ops)
Same situation here but for udp_bpf_get_proto.
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [Patch bpf-next v4 1/5] bpf: clean up sockmap related Kconfigs
2021-02-19 18:25 ` Jakub Sitnicki
@ 2021-02-19 18:46 ` Cong Wang
2021-02-19 22:00 ` Jakub Sitnicki
0 siblings, 1 reply; 12+ messages in thread
From: Cong Wang @ 2021-02-19 18:46 UTC (permalink / raw)
To: Jakub Sitnicki
Cc: Linux Kernel Network Developers, bpf, duanxiongchun,
Dongdong Wang, Jiang Wang, Cong Wang, Daniel Borkmann,
Lorenz Bauer, John Fastabend
On Fri, Feb 19, 2021 at 10:25 AM Jakub Sitnicki <jakub@cloudflare.com> wrote:
>
> On Tue, Feb 16, 2021 at 07:42 AM CET, Cong Wang wrote:
> > From: Cong Wang <cong.wang@bytedance.com>
> >
> > As suggested by John, clean up sockmap related Kconfigs:
> >
> > Reduce the scope of CONFIG_BPF_STREAM_PARSER down to TCP stream
> > parser, to reflect its name.
> >
> > Make the rest sockmap code simply depend on CONFIG_BPF_SYSCALL.
> > And leave CONFIG_NET_SOCK_MSG untouched, as it is used by
> > non-sockmap cases.
> >
> > Cc: Daniel Borkmann <daniel@iogearbox.net>
> > Cc: Jakub Sitnicki <jakub@cloudflare.com>
> > Reviewed-by: Lorenz Bauer <lmb@cloudflare.com>
> > Acked-by: John Fastabend <john.fastabend@gmail.com>
> > Signed-off-by: Cong Wang <cong.wang@bytedance.com>
> > ---
>
> Sorry for the delay. There's a lot happening here. Took me a while to
> dig through it.
>
> I have a couple of nit-picks, which easily can be addressed as
> follow-ups, and one comment.
No problem, it is not merged, so V5 is definitely not a problem.
>
> sock_map_prog_update and sk_psock_done_strp are only used in
> net/core/sock_map.c and can be static.
1. This seems to be unrelated to this patch? But I am still happy to
address it.
2. sk_psock_done_strp() is in skmsg.c, hence why it is non-static.
And I believe it fits in skmsg.c better than in sock_map.c, because
it operates on psock rather than sock_map itself.
So, I can make sock_map_prog_update() static in a separate patch
and carry it in V5.
>
> [...]
>
> > diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
> > index bc7d2a586e18..b2c4865eb39b 100644
> > --- a/net/ipv4/tcp_bpf.c
> > +++ b/net/ipv4/tcp_bpf.c
> > @@ -229,7 +229,6 @@ int tcp_bpf_sendmsg_redir(struct sock *sk, struct sk_msg *msg,
> > }
> > EXPORT_SYMBOL_GPL(tcp_bpf_sendmsg_redir);
> >
> > -#ifdef CONFIG_BPF_STREAM_PARSER
> > static bool tcp_bpf_stream_read(const struct sock *sk)
> > {
> > struct sk_psock *psock;
> > @@ -561,8 +560,10 @@ static void tcp_bpf_rebuild_protos(struct proto prot[TCP_BPF_NUM_CFGS],
> > struct proto *base)
> > {
> > prot[TCP_BPF_BASE] = *base;
> > +#if defined(CONFIG_BPF_SYSCALL)
> > prot[TCP_BPF_BASE].unhash = sock_map_unhash;
> > prot[TCP_BPF_BASE].close = sock_map_close;
> > +#endif
> > prot[TCP_BPF_BASE].recvmsg = tcp_bpf_recvmsg;
> > prot[TCP_BPF_BASE].stream_memory_read = tcp_bpf_stream_read;
> >
> > @@ -629,4 +630,3 @@ void tcp_bpf_clone(const struct sock *sk, struct sock *newsk)
> > if (prot == &tcp_bpf_prots[family][TCP_BPF_BASE])
> > newsk->sk_prot = sk->sk_prot_creator;
> > }
> > -#endif /* CONFIG_BPF_STREAM_PARSER */
>
> net/core/sock_map.o now is built only when CONFIG_BPF_SYSCALL is set.
> While tcp_bpf_get_proto is only called from net/core/sock_map.o.
>
> Seems there is no sense in compiling tcp_bpf_get_proto, and everything
> it depends on which was enclosed by CONFIG_BPF_STREAM_PARSER check, when
> CONFIG_BPF_SYSCALL is unset.
I can try but I am definitely not sure whether kTLS is happy about
it, clearly kTLS at least uses __tcp_bpf_recvmsg() and
tcp_bpf_sendmsg_redir().
>
> > diff --git a/net/ipv4/udp_bpf.c b/net/ipv4/udp_bpf.c
> > index 7a94791efc1a..e635ccc175ca 100644
> > --- a/net/ipv4/udp_bpf.c
> > +++ b/net/ipv4/udp_bpf.c
> > @@ -18,8 +18,10 @@ static struct proto udp_bpf_prots[UDP_BPF_NUM_PROTS];
> > static void udp_bpf_rebuild_protos(struct proto *prot, const struct proto *base)
> > {
> > *prot = *base;
> > +#if defined(CONFIG_BPF_SYSCALL)
> > prot->unhash = sock_map_unhash;
> > prot->close = sock_map_close;
> > +#endif
> > }
> >
> > static void udp_bpf_check_v6_needs_rebuild(struct proto *ops)
>
> Same situation here but for udp_bpf_get_proto.
UDP is different, as kTLS certainly doesn't and won't use it. I think
udp_bpf.c can be just put under CONFIG_BPF_SYSCALL.
Thanks.
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [Patch bpf-next v4 1/5] bpf: clean up sockmap related Kconfigs
2021-02-19 18:46 ` Cong Wang
@ 2021-02-19 22:00 ` Jakub Sitnicki
0 siblings, 0 replies; 12+ messages in thread
From: Jakub Sitnicki @ 2021-02-19 22:00 UTC (permalink / raw)
To: Cong Wang
Cc: Linux Kernel Network Developers, bpf, duanxiongchun,
Dongdong Wang, Jiang Wang, Cong Wang, Daniel Borkmann,
Lorenz Bauer, John Fastabend
On Fri, Feb 19, 2021 at 07:46 PM CET, Cong Wang wrote:
> On Fri, Feb 19, 2021 at 10:25 AM Jakub Sitnicki <jakub@cloudflare.com> wrote:
>>
>> On Tue, Feb 16, 2021 at 07:42 AM CET, Cong Wang wrote:
>> > From: Cong Wang <cong.wang@bytedance.com>
>> >
>> > As suggested by John, clean up sockmap related Kconfigs:
>> >
>> > Reduce the scope of CONFIG_BPF_STREAM_PARSER down to TCP stream
>> > parser, to reflect its name.
>> >
>> > Make the rest sockmap code simply depend on CONFIG_BPF_SYSCALL.
>> > And leave CONFIG_NET_SOCK_MSG untouched, as it is used by
>> > non-sockmap cases.
>> >
>> > Cc: Daniel Borkmann <daniel@iogearbox.net>
>> > Cc: Jakub Sitnicki <jakub@cloudflare.com>
>> > Reviewed-by: Lorenz Bauer <lmb@cloudflare.com>
>> > Acked-by: John Fastabend <john.fastabend@gmail.com>
>> > Signed-off-by: Cong Wang <cong.wang@bytedance.com>
>> > ---
>>
>> Sorry for the delay. There's a lot happening here. Took me a while to
>> dig through it.
>>
>> I have a couple of nit-picks, which easily can be addressed as
>> follow-ups, and one comment.
>
> No problem, it is not merged, so V5 is definitely not a problem.
>
>>
>> sock_map_prog_update and sk_psock_done_strp are only used in
>> net/core/sock_map.c and can be static.
>
> 1. This seems to be unrelated to this patch? But I am still happy to
> address it.
Completely unrelated. Just a nit-pick. Feel free to ignore.
> 2. sk_psock_done_strp() is in skmsg.c, hence why it is non-static.
> And I believe it fits in skmsg.c better than in sock_map.c, because
> it operates on psock rather than sock_map itself.
I wrote that sk_psock_done_strp is used only in net/core/sock_map.c,
while I should have written that it's used only in net/core/skmsg.c.
Sorry, a mistake when copying from my notes. Also, feel free to ignore.
> So, I can make sock_map_prog_update() static in a separate patch
> and carry it in V5.
Completely up to you. I don't insist.
>>
>> [...]
>>
>> > diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
>> > index bc7d2a586e18..b2c4865eb39b 100644
>> > --- a/net/ipv4/tcp_bpf.c
>> > +++ b/net/ipv4/tcp_bpf.c
>> > @@ -229,7 +229,6 @@ int tcp_bpf_sendmsg_redir(struct sock *sk, struct sk_msg *msg,
>> > }
>> > EXPORT_SYMBOL_GPL(tcp_bpf_sendmsg_redir);
>> >
>> > -#ifdef CONFIG_BPF_STREAM_PARSER
>> > static bool tcp_bpf_stream_read(const struct sock *sk)
>> > {
>> > struct sk_psock *psock;
>> > @@ -561,8 +560,10 @@ static void tcp_bpf_rebuild_protos(struct proto prot[TCP_BPF_NUM_CFGS],
>> > struct proto *base)
>> > {
>> > prot[TCP_BPF_BASE] = *base;
>> > +#if defined(CONFIG_BPF_SYSCALL)
>> > prot[TCP_BPF_BASE].unhash = sock_map_unhash;
>> > prot[TCP_BPF_BASE].close = sock_map_close;
>> > +#endif
>> > prot[TCP_BPF_BASE].recvmsg = tcp_bpf_recvmsg;
>> > prot[TCP_BPF_BASE].stream_memory_read = tcp_bpf_stream_read;
>> >
>> > @@ -629,4 +630,3 @@ void tcp_bpf_clone(const struct sock *sk, struct sock *newsk)
>> > if (prot == &tcp_bpf_prots[family][TCP_BPF_BASE])
>> > newsk->sk_prot = sk->sk_prot_creator;
>> > }
>> > -#endif /* CONFIG_BPF_STREAM_PARSER */
>>
>> net/core/sock_map.o now is built only when CONFIG_BPF_SYSCALL is set.
>> While tcp_bpf_get_proto is only called from net/core/sock_map.o.
>>
>> Seems there is no sense in compiling tcp_bpf_get_proto, and everything
>> it depends on which was enclosed by CONFIG_BPF_STREAM_PARSER check, when
>> CONFIG_BPF_SYSCALL is unset.
>
> I can try but I am definitely not sure whether kTLS is happy about
> it, clearly kTLS at least uses __tcp_bpf_recvmsg() and
> tcp_bpf_sendmsg_redir().
I think kTLS will be fine because that's the situation today.
__tcp_bpf_recvmsg and tcp_bpf_sendmsg_redir need to be left out,
naturally, is it is now.
(Although I think they could event be stubbed out. But that would be
unrelated to this change.)
After all how would we end up on code path in kTLS that utilizes sockmap
API, if sockmap can't be created when CONFIG_BPF_SYSCALL is unset.
>
>>
>> > diff --git a/net/ipv4/udp_bpf.c b/net/ipv4/udp_bpf.c
>> > index 7a94791efc1a..e635ccc175ca 100644
>> > --- a/net/ipv4/udp_bpf.c
>> > +++ b/net/ipv4/udp_bpf.c
>> > @@ -18,8 +18,10 @@ static struct proto udp_bpf_prots[UDP_BPF_NUM_PROTS];
>> > static void udp_bpf_rebuild_protos(struct proto *prot, const struct proto *base)
>> > {
>> > *prot = *base;
>> > +#if defined(CONFIG_BPF_SYSCALL)
>> > prot->unhash = sock_map_unhash;
>> > prot->close = sock_map_close;
>> > +#endif
>> > }
>> >
>> > static void udp_bpf_check_v6_needs_rebuild(struct proto *ops)
>>
>> Same situation here but for udp_bpf_get_proto.
>
> UDP is different, as kTLS certainly doesn't and won't use it. I think
> udp_bpf.c can be just put under CONFIG_BPF_SYSCALL.
>
> Thanks.
^ permalink raw reply [flat|nested] 12+ messages in thread
* [Patch bpf-next v4 2/5] skmsg: get rid of struct sk_psock_parser
2021-02-16 6:42 [Patch bpf-next v4 0/5] sock_map: clean up and refactor code for BPF_SK_SKB_VERDICT Cong Wang
2021-02-16 6:42 ` [Patch bpf-next v4 1/5] bpf: clean up sockmap related Kconfigs Cong Wang
@ 2021-02-16 6:42 ` Cong Wang
2021-02-16 6:42 ` [Patch bpf-next v4 3/5] bpf: compute data_end dynamically with JIT code Cong Wang
` (2 subsequent siblings)
4 siblings, 0 replies; 12+ messages in thread
From: Cong Wang @ 2021-02-16 6:42 UTC (permalink / raw)
To: netdev
Cc: bpf, duanxiongchun, wangdongdong.6, jiang.wang, Cong Wang,
Daniel Borkmann, Lorenz Bauer, John Fastabend, Jakub Sitnicki
From: Cong Wang <cong.wang@bytedance.com>
struct sk_psock_parser is embedded in sk_psock, it is
unnecessary as skb verdict also uses ->saved_data_ready.
We can simply fold these fields into sk_psock, and get rid
of ->enabled.
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Lorenz Bauer <lmb@cloudflare.com>
Acked-by: John Fastabend <john.fastabend@gmail.com>
Acked-by: Jakub Sitnicki <jakub@cloudflare.com>
Signed-off-by: Cong Wang <cong.wang@bytedance.com>
---
include/linux/skmsg.h | 19 ++++++----------
net/core/skmsg.c | 53 +++++++++++++------------------------------
net/core/sock_map.c | 8 +++----
3 files changed, 27 insertions(+), 53 deletions(-)
diff --git a/include/linux/skmsg.h b/include/linux/skmsg.h
index 041faef00937..e3bb712af257 100644
--- a/include/linux/skmsg.h
+++ b/include/linux/skmsg.h
@@ -70,14 +70,6 @@ struct sk_psock_link {
void *link_raw;
};
-struct sk_psock_parser {
-#if IS_ENABLED(CONFIG_BPF_STREAM_PARSER)
- struct strparser strp;
-#endif
- bool enabled;
- void (*saved_data_ready)(struct sock *sk);
-};
-
struct sk_psock_work_state {
struct sk_buff *skb;
u32 len;
@@ -92,7 +84,9 @@ struct sk_psock {
u32 eval;
struct sk_msg *cork;
struct sk_psock_progs progs;
- struct sk_psock_parser parser;
+#if IS_ENABLED(CONFIG_BPF_STREAM_PARSER)
+ struct strparser strp;
+#endif
struct sk_buff_head ingress_skb;
struct list_head ingress_msg;
unsigned long state;
@@ -102,6 +96,7 @@ struct sk_psock {
void (*saved_unhash)(struct sock *sk);
void (*saved_close)(struct sock *sk, long timeout);
void (*saved_write_space)(struct sock *sk);
+ void (*saved_data_ready)(struct sock *sk);
struct proto *sk_proto;
struct sk_psock_work_state work_state;
struct work_struct work;
@@ -422,8 +417,8 @@ static inline void sk_psock_put(struct sock *sk, struct sk_psock *psock)
static inline void sk_psock_data_ready(struct sock *sk, struct sk_psock *psock)
{
- if (psock->parser.enabled)
- psock->parser.saved_data_ready(sk);
+ if (psock->saved_data_ready)
+ psock->saved_data_ready(sk);
else
sk->sk_data_ready(sk);
}
@@ -462,6 +457,6 @@ static inline bool sk_psock_strp_enabled(struct sk_psock *psock)
{
if (!psock)
return false;
- return psock->parser.enabled;
+ return !!psock->saved_data_ready;
}
#endif /* _LINUX_SKMSG_H */
diff --git a/net/core/skmsg.c b/net/core/skmsg.c
index 6cb5ff6f8f9c..7f400d044cda 100644
--- a/net/core/skmsg.c
+++ b/net/core/skmsg.c
@@ -905,17 +905,9 @@ static int sk_psock_strp_read_done(struct strparser *strp, int err)
return err;
}
-static struct sk_psock *sk_psock_from_strp(struct strparser *strp)
-{
- struct sk_psock_parser *parser;
-
- parser = container_of(strp, struct sk_psock_parser, strp);
- return container_of(parser, struct sk_psock, parser);
-}
-
static int sk_psock_strp_parse(struct strparser *strp, struct sk_buff *skb)
{
- struct sk_psock *psock = sk_psock_from_strp(strp);
+ struct sk_psock *psock = container_of(strp, struct sk_psock, strp);
struct bpf_prog *prog;
int ret = skb->len;
@@ -939,10 +931,10 @@ static void sk_psock_strp_data_ready(struct sock *sk)
psock = sk_psock(sk);
if (likely(psock)) {
if (tls_sw_has_ctx_rx(sk)) {
- psock->parser.saved_data_ready(sk);
+ psock->saved_data_ready(sk);
} else {
write_lock_bh(&sk->sk_callback_lock);
- strp_data_ready(&psock->parser.strp);
+ strp_data_ready(&psock->strp);
write_unlock_bh(&sk->sk_callback_lock);
}
}
@@ -957,41 +949,34 @@ int sk_psock_init_strp(struct sock *sk, struct sk_psock *psock)
.parse_msg = sk_psock_strp_parse,
};
- psock->parser.enabled = false;
- return strp_init(&psock->parser.strp, sk, &cb);
+ return strp_init(&psock->strp, sk, &cb);
}
void sk_psock_start_strp(struct sock *sk, struct sk_psock *psock)
{
- struct sk_psock_parser *parser = &psock->parser;
-
- if (parser->enabled)
+ if (psock->saved_data_ready)
return;
- parser->saved_data_ready = sk->sk_data_ready;
+ psock->saved_data_ready = sk->sk_data_ready;
sk->sk_data_ready = sk_psock_strp_data_ready;
sk->sk_write_space = sk_psock_write_space;
- parser->enabled = true;
}
void sk_psock_stop_strp(struct sock *sk, struct sk_psock *psock)
{
- struct sk_psock_parser *parser = &psock->parser;
-
- if (!parser->enabled)
+ if (!psock->saved_data_ready)
return;
- sk->sk_data_ready = parser->saved_data_ready;
- parser->saved_data_ready = NULL;
- strp_stop(&parser->strp);
- parser->enabled = false;
+ sk->sk_data_ready = psock->saved_data_ready;
+ psock->saved_data_ready = NULL;
+ strp_stop(&psock->strp);
}
void sk_psock_done_strp(struct sk_psock *psock)
{
/* Parser has been stopped */
if (psock->progs.skb_parser)
- strp_done(&psock->parser.strp);
+ strp_done(&psock->strp);
}
#endif
@@ -1048,25 +1033,19 @@ static void sk_psock_verdict_data_ready(struct sock *sk)
void sk_psock_start_verdict(struct sock *sk, struct sk_psock *psock)
{
- struct sk_psock_parser *parser = &psock->parser;
-
- if (parser->enabled)
+ if (psock->saved_data_ready)
return;
- parser->saved_data_ready = sk->sk_data_ready;
+ psock->saved_data_ready = sk->sk_data_ready;
sk->sk_data_ready = sk_psock_verdict_data_ready;
sk->sk_write_space = sk_psock_write_space;
- parser->enabled = true;
}
void sk_psock_stop_verdict(struct sock *sk, struct sk_psock *psock)
{
- struct sk_psock_parser *parser = &psock->parser;
-
- if (!parser->enabled)
+ if (!psock->saved_data_ready)
return;
- sk->sk_data_ready = parser->saved_data_ready;
- parser->saved_data_ready = NULL;
- parser->enabled = false;
+ sk->sk_data_ready = psock->saved_data_ready;
+ psock->saved_data_ready = NULL;
}
diff --git a/net/core/sock_map.c b/net/core/sock_map.c
index ee3334dd3a38..1a28a5c2c61e 100644
--- a/net/core/sock_map.c
+++ b/net/core/sock_map.c
@@ -148,9 +148,9 @@ static void sock_map_del_link(struct sock *sk,
struct bpf_map *map = link->map;
struct bpf_stab *stab = container_of(map, struct bpf_stab,
map);
- if (psock->parser.enabled && stab->progs.skb_parser)
+ if (psock->saved_data_ready && stab->progs.skb_parser)
strp_stop = true;
- if (psock->parser.enabled && stab->progs.skb_verdict)
+ if (psock->saved_data_ready && stab->progs.skb_verdict)
verdict_stop = true;
list_del(&link->list);
sk_psock_free_link(link);
@@ -283,14 +283,14 @@ static int sock_map_link(struct bpf_map *map, struct sk_psock_progs *progs,
goto out_drop;
write_lock_bh(&sk->sk_callback_lock);
- if (skb_parser && skb_verdict && !psock->parser.enabled) {
+ if (skb_parser && skb_verdict && !psock->saved_data_ready) {
ret = sk_psock_init_strp(sk, psock);
if (ret)
goto out_unlock_drop;
psock_set_prog(&psock->progs.skb_verdict, skb_verdict);
psock_set_prog(&psock->progs.skb_parser, skb_parser);
sk_psock_start_strp(sk, psock);
- } else if (!skb_parser && skb_verdict && !psock->parser.enabled) {
+ } else if (!skb_parser && skb_verdict && !psock->saved_data_ready) {
psock_set_prog(&psock->progs.skb_verdict, skb_verdict);
sk_psock_start_verdict(sk,psock);
}
--
2.25.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* [Patch bpf-next v4 3/5] bpf: compute data_end dynamically with JIT code
2021-02-16 6:42 [Patch bpf-next v4 0/5] sock_map: clean up and refactor code for BPF_SK_SKB_VERDICT Cong Wang
2021-02-16 6:42 ` [Patch bpf-next v4 1/5] bpf: clean up sockmap related Kconfigs Cong Wang
2021-02-16 6:42 ` [Patch bpf-next v4 2/5] skmsg: get rid of struct sk_psock_parser Cong Wang
@ 2021-02-16 6:42 ` Cong Wang
2021-02-16 6:42 ` [Patch bpf-next v4 4/5] skmsg: move sk_redir from TCP_SKB_CB to skb Cong Wang
2021-02-16 6:42 ` [Patch bpf-next v4 5/5] sock_map: rename skb_parser and skb_verdict Cong Wang
4 siblings, 0 replies; 12+ messages in thread
From: Cong Wang @ 2021-02-16 6:42 UTC (permalink / raw)
To: netdev
Cc: bpf, duanxiongchun, wangdongdong.6, jiang.wang, Cong Wang,
Jakub Sitnicki, Daniel Borkmann, Lorenz Bauer, John Fastabend
From: Cong Wang <cong.wang@bytedance.com>
Currently, we compute ->data_end with a compile-time constant
offset of skb. But as Jakub pointed out, we can actually compute
it in eBPF JIT code at run-time, so that we can competely get
rid of ->data_end. This is similar to skb_shinfo(skb) computation
in bpf_convert_shinfo_access().
Suggested-by: Jakub Sitnicki <jakub@cloudflare.com>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Lorenz Bauer <lmb@cloudflare.com>
Acked-by: John Fastabend <john.fastabend@gmail.com>
Acked-by: Jakub Sitnicki <jakub@cloudflare.com>
Signed-off-by: Cong Wang <cong.wang@bytedance.com>
---
include/net/tcp.h | 6 ------
net/core/filter.c | 48 +++++++++++++++++++++++++++--------------------
net/core/skmsg.c | 1 -
3 files changed, 28 insertions(+), 27 deletions(-)
diff --git a/include/net/tcp.h b/include/net/tcp.h
index 99fdbf03aeee..697712178eff 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -885,18 +885,12 @@ struct tcp_skb_cb {
struct {
__u32 flags;
struct sock *sk_redir;
- void *data_end;
} bpf;
};
};
#define TCP_SKB_CB(__skb) ((struct tcp_skb_cb *)&((__skb)->cb[0]))
-static inline void bpf_compute_data_end_sk_skb(struct sk_buff *skb)
-{
- TCP_SKB_CB(skb)->bpf.data_end = skb->data + skb_headlen(skb);
-}
-
static inline bool tcp_skb_bpf_ingress(const struct sk_buff *skb)
{
return TCP_SKB_CB(skb)->bpf.flags & BPF_F_INGRESS;
diff --git a/net/core/filter.c b/net/core/filter.c
index 7059cf604d94..38c4996e48bf 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -1863,10 +1863,7 @@ static const struct bpf_func_proto bpf_sk_fullsock_proto = {
static inline int sk_skb_try_make_writable(struct sk_buff *skb,
unsigned int write_len)
{
- int err = __bpf_try_make_writable(skb, write_len);
-
- bpf_compute_data_end_sk_skb(skb);
- return err;
+ return __bpf_try_make_writable(skb, write_len);
}
BPF_CALL_2(sk_skb_pull_data, struct sk_buff *, skb, u32, len)
@@ -3577,7 +3574,6 @@ BPF_CALL_4(sk_skb_adjust_room, struct sk_buff *, skb, s32, len_diff,
return -ENOMEM;
__skb_pull(skb, len_diff_abs);
}
- bpf_compute_data_end_sk_skb(skb);
if (tls_sw_has_ctx_rx(skb->sk)) {
struct strp_msg *rxm = strp_msg(skb);
@@ -3742,10 +3738,7 @@ static const struct bpf_func_proto bpf_skb_change_tail_proto = {
BPF_CALL_3(sk_skb_change_tail, struct sk_buff *, skb, u32, new_len,
u64, flags)
{
- int ret = __bpf_skb_change_tail(skb, new_len, flags);
-
- bpf_compute_data_end_sk_skb(skb);
- return ret;
+ return __bpf_skb_change_tail(skb, new_len, flags);
}
static const struct bpf_func_proto sk_skb_change_tail_proto = {
@@ -3808,10 +3801,7 @@ static const struct bpf_func_proto bpf_skb_change_head_proto = {
BPF_CALL_3(sk_skb_change_head, struct sk_buff *, skb, u32, head_room,
u64, flags)
{
- int ret = __bpf_skb_change_head(skb, head_room, flags);
-
- bpf_compute_data_end_sk_skb(skb);
- return ret;
+ return __bpf_skb_change_head(skb, head_room, flags);
}
static const struct bpf_func_proto sk_skb_change_head_proto = {
@@ -9657,22 +9647,40 @@ static u32 sock_ops_convert_ctx_access(enum bpf_access_type type,
return insn - insn_buf;
}
+/* data_end = skb->data + skb_headlen() */
+static struct bpf_insn *bpf_convert_data_end_access(const struct bpf_insn *si,
+ struct bpf_insn *insn)
+{
+ /* si->dst_reg = skb->data */
+ *insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(struct sk_buff, data),
+ si->dst_reg, si->src_reg,
+ offsetof(struct sk_buff, data));
+ /* AX = skb->len */
+ *insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(struct sk_buff, len),
+ BPF_REG_AX, si->src_reg,
+ offsetof(struct sk_buff, len));
+ /* si->dst_reg = skb->data + skb->len */
+ *insn++ = BPF_ALU64_REG(BPF_ADD, si->dst_reg, BPF_REG_AX);
+ /* AX = skb->data_len */
+ *insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(struct sk_buff, data_len),
+ BPF_REG_AX, si->src_reg,
+ offsetof(struct sk_buff, data_len));
+ /* si->dst_reg = skb->data + skb->len - skb->data_len */
+ *insn++ = BPF_ALU64_REG(BPF_SUB, si->dst_reg, BPF_REG_AX);
+
+ return insn;
+}
+
static u32 sk_skb_convert_ctx_access(enum bpf_access_type type,
const struct bpf_insn *si,
struct bpf_insn *insn_buf,
struct bpf_prog *prog, u32 *target_size)
{
struct bpf_insn *insn = insn_buf;
- int off;
switch (si->off) {
case offsetof(struct __sk_buff, data_end):
- off = si->off;
- off -= offsetof(struct __sk_buff, data_end);
- off += offsetof(struct sk_buff, cb);
- off += offsetof(struct tcp_skb_cb, bpf.data_end);
- *insn++ = BPF_LDX_MEM(BPF_SIZEOF(void *), si->dst_reg,
- si->src_reg, off);
+ insn = bpf_convert_data_end_access(si, insn);
break;
default:
return bpf_convert_ctx_access(type, si, insn_buf, prog,
diff --git a/net/core/skmsg.c b/net/core/skmsg.c
index 7f400d044cda..2d8bbb3fd87c 100644
--- a/net/core/skmsg.c
+++ b/net/core/skmsg.c
@@ -744,7 +744,6 @@ EXPORT_SYMBOL_GPL(sk_psock_msg_verdict);
static int sk_psock_bpf_run(struct sk_psock *psock, struct bpf_prog *prog,
struct sk_buff *skb)
{
- bpf_compute_data_end_sk_skb(skb);
return bpf_prog_run_pin_on_cpu(prog, skb);
}
--
2.25.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* [Patch bpf-next v4 4/5] skmsg: move sk_redir from TCP_SKB_CB to skb
2021-02-16 6:42 [Patch bpf-next v4 0/5] sock_map: clean up and refactor code for BPF_SK_SKB_VERDICT Cong Wang
` (2 preceding siblings ...)
2021-02-16 6:42 ` [Patch bpf-next v4 3/5] bpf: compute data_end dynamically with JIT code Cong Wang
@ 2021-02-16 6:42 ` Cong Wang
2021-02-17 18:40 ` John Fastabend
2021-02-16 6:42 ` [Patch bpf-next v4 5/5] sock_map: rename skb_parser and skb_verdict Cong Wang
4 siblings, 1 reply; 12+ messages in thread
From: Cong Wang @ 2021-02-16 6:42 UTC (permalink / raw)
To: netdev
Cc: bpf, duanxiongchun, wangdongdong.6, jiang.wang, Cong Wang,
John Fastabend, Daniel Borkmann, Jakub Sitnicki, Lorenz Bauer
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: John Fastabend <john.fastabend@gmail.com>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Jakub Sitnicki <jakub@cloudflare.com>
Cc: Lorenz Bauer <lmb@cloudflare.com>
Signed-off-by: Cong Wang <cong.wang@bytedance.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 46f901adf1a8..9f09fb393971 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;
+ 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 697712178eff..e35881f837b2 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -882,30 +882,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);
+
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);
+ 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);
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;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* RE: [Patch bpf-next v4 4/5] skmsg: move sk_redir from TCP_SKB_CB to skb
2021-02-16 6:42 ` [Patch bpf-next v4 4/5] skmsg: move sk_redir from TCP_SKB_CB to skb Cong Wang
@ 2021-02-17 18:40 ` John Fastabend
2021-02-17 18:49 ` Cong Wang
0 siblings, 1 reply; 12+ messages in thread
From: John Fastabend @ 2021-02-17 18:40 UTC (permalink / raw)
To: Cong Wang, netdev
Cc: bpf, duanxiongchun, wangdongdong.6, jiang.wang, Cong Wang,
John Fastabend, Daniel Borkmann, Jakub Sitnicki, Lorenz Bauer
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: John Fastabend <john.fastabend@gmail.com>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> Cc: Jakub Sitnicki <jakub@cloudflare.com>
> Cc: Lorenz Bauer <lmb@cloudflare.com>
> Signed-off-by: Cong Wang <cong.wang@bytedance.com>
> ---
Great, its likely we have more space in sk_buff we can use if needed
as well. queue_mapping, skb_iif, vlan*, fields come to mind. Couple
comments inline, but looks good to me.
Acked-by: John Fastabend <john.fastabend@gmail.com>
[...]
> --- 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);
> +
> 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);
> + 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);
Do we really need the skb_dst_drop() I thought we would have already dropped this here
but I've not had time to check yet.
> 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);
Same here.
> + 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);
And here.
> + 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);
Thanks for doing this.
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [Patch bpf-next v4 4/5] skmsg: move sk_redir from TCP_SKB_CB to skb
2021-02-17 18:40 ` John Fastabend
@ 2021-02-17 18:49 ` Cong Wang
2021-02-17 20:12 ` John Fastabend
0 siblings, 1 reply; 12+ messages in thread
From: Cong Wang @ 2021-02-17 18:49 UTC (permalink / raw)
To: John Fastabend
Cc: Linux Kernel Network Developers, bpf, duanxiongchun,
Dongdong Wang, Jiang Wang, Cong Wang, Daniel Borkmann,
Jakub Sitnicki, Lorenz Bauer
On Wed, Feb 17, 2021 at 10:40 AM John Fastabend
<john.fastabend@gmail.com> wrote:
> > @@ -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);
>
> Do we really need the skb_dst_drop() I thought we would have already dropped this here
> but I've not had time to check yet.
Yes, I got some serious complaints from dst_release() when I didn't
add skb_dst_drop().
Thanks.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Patch bpf-next v4 4/5] skmsg: move sk_redir from TCP_SKB_CB to skb
2021-02-17 18:49 ` Cong Wang
@ 2021-02-17 20:12 ` John Fastabend
0 siblings, 0 replies; 12+ messages in thread
From: John Fastabend @ 2021-02-17 20:12 UTC (permalink / raw)
To: Cong Wang, John Fastabend
Cc: Linux Kernel Network Developers, bpf, duanxiongchun,
Dongdong Wang, Jiang Wang, Cong Wang, Daniel Borkmann,
Jakub Sitnicki, Lorenz Bauer
Cong Wang wrote:
> On Wed, Feb 17, 2021 at 10:40 AM John Fastabend
> <john.fastabend@gmail.com> wrote:
> > > @@ -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);
> >
> > Do we really need the skb_dst_drop() I thought we would have already dropped this here
> > but I've not had time to check yet.
>
> Yes, I got some serious complaints from dst_release() when I didn't
> add skb_dst_drop().
>
> Thanks.
OK thanks.
^ permalink raw reply [flat|nested] 12+ messages in thread
* [Patch bpf-next v4 5/5] sock_map: rename skb_parser and skb_verdict
2021-02-16 6:42 [Patch bpf-next v4 0/5] sock_map: clean up and refactor code for BPF_SK_SKB_VERDICT Cong Wang
` (3 preceding siblings ...)
2021-02-16 6:42 ` [Patch bpf-next v4 4/5] skmsg: move sk_redir from TCP_SKB_CB to skb Cong Wang
@ 2021-02-16 6:42 ` Cong Wang
4 siblings, 0 replies; 12+ messages in thread
From: Cong Wang @ 2021-02-16 6:42 UTC (permalink / raw)
To: netdev
Cc: bpf, duanxiongchun, wangdongdong.6, jiang.wang, Cong Wang,
Daniel Borkmann, Jakub Sitnicki, Lorenz Bauer, John Fastabend
From: Cong Wang <cong.wang@bytedance.com>
These two eBPF programs are tied to BPF_SK_SKB_STREAM_PARSER
and BPF_SK_SKB_STREAM_VERDICT, rename them to reflect the fact
they are only used for TCP. And save the name 'skb_verdict' for
general use later.
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Jakub Sitnicki <jakub@cloudflare.com>
Reviewed-by: Lorenz Bauer <lmb@cloudflare.com>
Acked-by: John Fastabend <john.fastabend@gmail.com>
Signed-off-by: Cong Wang <cong.wang@bytedance.com>
---
include/linux/skmsg.h | 8 +--
net/core/skmsg.c | 14 ++---
net/core/sock_map.c | 60 +++++++++----------
.../selftests/bpf/prog_tests/sockmap_listen.c | 8 +--
.../selftests/bpf/progs/test_sockmap_listen.c | 4 +-
5 files changed, 47 insertions(+), 47 deletions(-)
diff --git a/include/linux/skmsg.h b/include/linux/skmsg.h
index fc234d507fd7..ab3f3f2c426f 100644
--- a/include/linux/skmsg.h
+++ b/include/linux/skmsg.h
@@ -56,8 +56,8 @@ struct sk_msg {
struct sk_psock_progs {
struct bpf_prog *msg_parser;
- struct bpf_prog *skb_parser;
- struct bpf_prog *skb_verdict;
+ struct bpf_prog *stream_parser;
+ struct bpf_prog *stream_verdict;
};
enum sk_psock_state_bits {
@@ -447,8 +447,8 @@ static inline int psock_replace_prog(struct bpf_prog **pprog,
static inline void psock_progs_drop(struct sk_psock_progs *progs)
{
psock_set_prog(&progs->msg_parser, NULL);
- psock_set_prog(&progs->skb_parser, NULL);
- psock_set_prog(&progs->skb_verdict, NULL);
+ psock_set_prog(&progs->stream_parser, NULL);
+ psock_set_prog(&progs->stream_verdict, NULL);
}
int sk_psock_tls_strp_read(struct sk_psock *psock, struct sk_buff *skb);
diff --git a/net/core/skmsg.c b/net/core/skmsg.c
index 05b5af09ff42..dbb176427c14 100644
--- a/net/core/skmsg.c
+++ b/net/core/skmsg.c
@@ -690,9 +690,9 @@ void sk_psock_drop(struct sock *sk, struct sk_psock *psock)
write_lock_bh(&sk->sk_callback_lock);
sk_psock_restore_proto(sk, psock);
rcu_assign_sk_user_data(sk, NULL);
- if (psock->progs.skb_parser)
+ if (psock->progs.stream_parser)
sk_psock_stop_strp(sk, psock);
- else if (psock->progs.skb_verdict)
+ else if (psock->progs.stream_verdict)
sk_psock_stop_verdict(sk, psock);
write_unlock_bh(&sk->sk_callback_lock);
sk_psock_clear_state(psock, SK_PSOCK_TX_ENABLED);
@@ -802,7 +802,7 @@ int sk_psock_tls_strp_read(struct sk_psock *psock, struct sk_buff *skb)
int ret = __SK_PASS;
rcu_read_lock();
- prog = READ_ONCE(psock->progs.skb_verdict);
+ prog = READ_ONCE(psock->progs.stream_verdict);
if (likely(prog)) {
/* We skip full set_owner_r here because if we do a SK_PASS
* or SK_DROP we can skip skb memory accounting and use the
@@ -894,7 +894,7 @@ static void sk_psock_strp_read(struct strparser *strp, struct sk_buff *skb)
goto out;
}
skb_set_owner_r(skb, sk);
- prog = READ_ONCE(psock->progs.skb_verdict);
+ prog = READ_ONCE(psock->progs.stream_verdict);
if (likely(prog)) {
skb_dst_drop(skb);
skb_bpf_redirect_clear(skb);
@@ -918,7 +918,7 @@ static int sk_psock_strp_parse(struct strparser *strp, struct sk_buff *skb)
int ret = skb->len;
rcu_read_lock();
- prog = READ_ONCE(psock->progs.skb_parser);
+ prog = READ_ONCE(psock->progs.stream_parser);
if (likely(prog)) {
skb->sk = psock->sk;
ret = sk_psock_bpf_run(psock, prog, skb);
@@ -981,7 +981,7 @@ void sk_psock_stop_strp(struct sock *sk, struct sk_psock *psock)
void sk_psock_done_strp(struct sk_psock *psock)
{
/* Parser has been stopped */
- if (psock->progs.skb_parser)
+ if (psock->progs.stream_parser)
strp_done(&psock->strp);
}
#endif
@@ -1010,7 +1010,7 @@ static int sk_psock_verdict_recv(read_descriptor_t *desc, struct sk_buff *skb,
goto out;
}
skb_set_owner_r(skb, sk);
- prog = READ_ONCE(psock->progs.skb_verdict);
+ prog = READ_ONCE(psock->progs.stream_verdict);
if (likely(prog)) {
skb_dst_drop(skb);
skb_bpf_redirect_clear(skb);
diff --git a/net/core/sock_map.c b/net/core/sock_map.c
index dbfcd7006338..69785070f02d 100644
--- a/net/core/sock_map.c
+++ b/net/core/sock_map.c
@@ -148,9 +148,9 @@ static void sock_map_del_link(struct sock *sk,
struct bpf_map *map = link->map;
struct bpf_stab *stab = container_of(map, struct bpf_stab,
map);
- if (psock->saved_data_ready && stab->progs.skb_parser)
+ if (psock->saved_data_ready && stab->progs.stream_parser)
strp_stop = true;
- if (psock->saved_data_ready && stab->progs.skb_verdict)
+ if (psock->saved_data_ready && stab->progs.stream_verdict)
verdict_stop = true;
list_del(&link->list);
sk_psock_free_link(link);
@@ -224,23 +224,23 @@ static struct sk_psock *sock_map_psock_get_checked(struct sock *sk)
static int sock_map_link(struct bpf_map *map, struct sk_psock_progs *progs,
struct sock *sk)
{
- struct bpf_prog *msg_parser, *skb_parser, *skb_verdict;
+ struct bpf_prog *msg_parser, *stream_parser, *stream_verdict;
struct sk_psock *psock;
int ret;
- skb_verdict = READ_ONCE(progs->skb_verdict);
- if (skb_verdict) {
- skb_verdict = bpf_prog_inc_not_zero(skb_verdict);
- if (IS_ERR(skb_verdict))
- return PTR_ERR(skb_verdict);
+ stream_verdict = READ_ONCE(progs->stream_verdict);
+ if (stream_verdict) {
+ stream_verdict = bpf_prog_inc_not_zero(stream_verdict);
+ if (IS_ERR(stream_verdict))
+ return PTR_ERR(stream_verdict);
}
- skb_parser = READ_ONCE(progs->skb_parser);
- if (skb_parser) {
- skb_parser = bpf_prog_inc_not_zero(skb_parser);
- if (IS_ERR(skb_parser)) {
- ret = PTR_ERR(skb_parser);
- goto out_put_skb_verdict;
+ stream_parser = READ_ONCE(progs->stream_parser);
+ if (stream_parser) {
+ stream_parser = bpf_prog_inc_not_zero(stream_parser);
+ if (IS_ERR(stream_parser)) {
+ ret = PTR_ERR(stream_parser);
+ goto out_put_stream_verdict;
}
}
@@ -249,7 +249,7 @@ static int sock_map_link(struct bpf_map *map, struct sk_psock_progs *progs,
msg_parser = bpf_prog_inc_not_zero(msg_parser);
if (IS_ERR(msg_parser)) {
ret = PTR_ERR(msg_parser);
- goto out_put_skb_parser;
+ goto out_put_stream_parser;
}
}
@@ -261,8 +261,8 @@ static int sock_map_link(struct bpf_map *map, struct sk_psock_progs *progs,
if (psock) {
if ((msg_parser && READ_ONCE(psock->progs.msg_parser)) ||
- (skb_parser && READ_ONCE(psock->progs.skb_parser)) ||
- (skb_verdict && READ_ONCE(psock->progs.skb_verdict))) {
+ (stream_parser && READ_ONCE(psock->progs.stream_parser)) ||
+ (stream_verdict && READ_ONCE(psock->progs.stream_verdict))) {
sk_psock_put(sk, psock);
ret = -EBUSY;
goto out_progs;
@@ -283,15 +283,15 @@ static int sock_map_link(struct bpf_map *map, struct sk_psock_progs *progs,
goto out_drop;
write_lock_bh(&sk->sk_callback_lock);
- if (skb_parser && skb_verdict && !psock->saved_data_ready) {
+ if (stream_parser && stream_verdict && !psock->saved_data_ready) {
ret = sk_psock_init_strp(sk, psock);
if (ret)
goto out_unlock_drop;
- psock_set_prog(&psock->progs.skb_verdict, skb_verdict);
- psock_set_prog(&psock->progs.skb_parser, skb_parser);
+ psock_set_prog(&psock->progs.stream_verdict, stream_verdict);
+ psock_set_prog(&psock->progs.stream_parser, stream_parser);
sk_psock_start_strp(sk, psock);
- } else if (!skb_parser && skb_verdict && !psock->saved_data_ready) {
- psock_set_prog(&psock->progs.skb_verdict, skb_verdict);
+ } else if (!stream_parser && stream_verdict && !psock->saved_data_ready) {
+ psock_set_prog(&psock->progs.stream_verdict, stream_verdict);
sk_psock_start_verdict(sk,psock);
}
write_unlock_bh(&sk->sk_callback_lock);
@@ -303,12 +303,12 @@ static int sock_map_link(struct bpf_map *map, struct sk_psock_progs *progs,
out_progs:
if (msg_parser)
bpf_prog_put(msg_parser);
-out_put_skb_parser:
- if (skb_parser)
- bpf_prog_put(skb_parser);
-out_put_skb_verdict:
- if (skb_verdict)
- bpf_prog_put(skb_verdict);
+out_put_stream_parser:
+ if (stream_parser)
+ bpf_prog_put(stream_parser);
+out_put_stream_verdict:
+ if (stream_verdict)
+ bpf_prog_put(stream_verdict);
return ret;
}
@@ -1459,11 +1459,11 @@ int sock_map_prog_update(struct bpf_map *map, struct bpf_prog *prog,
break;
#if IS_ENABLED(CONFIG_BPF_STREAM_PARSER)
case BPF_SK_SKB_STREAM_PARSER:
- pprog = &progs->skb_parser;
+ pprog = &progs->stream_parser;
break;
#endif
case BPF_SK_SKB_STREAM_VERDICT:
- pprog = &progs->skb_verdict;
+ pprog = &progs->stream_verdict;
break;
default:
return -EOPNOTSUPP;
diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
index d7d65a700799..c26e6bf05e49 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
+++ b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
@@ -1014,8 +1014,8 @@ static void test_skb_redir_to_connected(struct test_sockmap_listen *skel,
struct bpf_map *inner_map, int family,
int sotype)
{
- int verdict = bpf_program__fd(skel->progs.prog_skb_verdict);
- int parser = bpf_program__fd(skel->progs.prog_skb_parser);
+ int verdict = bpf_program__fd(skel->progs.prog_stream_verdict);
+ int parser = bpf_program__fd(skel->progs.prog_stream_parser);
int verdict_map = bpf_map__fd(skel->maps.verdict_map);
int sock_map = bpf_map__fd(inner_map);
int err;
@@ -1125,8 +1125,8 @@ static void test_skb_redir_to_listening(struct test_sockmap_listen *skel,
struct bpf_map *inner_map, int family,
int sotype)
{
- int verdict = bpf_program__fd(skel->progs.prog_skb_verdict);
- int parser = bpf_program__fd(skel->progs.prog_skb_parser);
+ int verdict = bpf_program__fd(skel->progs.prog_stream_verdict);
+ int parser = bpf_program__fd(skel->progs.prog_stream_parser);
int verdict_map = bpf_map__fd(skel->maps.verdict_map);
int sock_map = bpf_map__fd(inner_map);
int err;
diff --git a/tools/testing/selftests/bpf/progs/test_sockmap_listen.c b/tools/testing/selftests/bpf/progs/test_sockmap_listen.c
index a3a366c57ce1..fa221141e9c1 100644
--- a/tools/testing/selftests/bpf/progs/test_sockmap_listen.c
+++ b/tools/testing/selftests/bpf/progs/test_sockmap_listen.c
@@ -31,13 +31,13 @@ struct {
static volatile bool test_sockmap; /* toggled by user-space */
SEC("sk_skb/stream_parser")
-int prog_skb_parser(struct __sk_buff *skb)
+int prog_stream_parser(struct __sk_buff *skb)
{
return skb->len;
}
SEC("sk_skb/stream_verdict")
-int prog_skb_verdict(struct __sk_buff *skb)
+int prog_stream_verdict(struct __sk_buff *skb)
{
unsigned int *count;
__u32 zero = 0;
--
2.25.1
^ permalink raw reply related [flat|nested] 12+ messages in thread