From: Gang Yan <yangang@kylinos.cn> Changelog: v2: - Patches 1 and 2 are new in this series; they address TCP_MAXSEG handling in the bpf_setsockopt() path [1]. - Patch 4 adds an early return to fix msk->sk_rcvlowat being unexpectedly modified, an issue seen in v1. - Patch 5 makes the hook safe for the non-tcp master socket: it guards bpf_sock_ops_cb_flags_set() with sk_is_tcp() to prevent out-of-bounds heap reads/writes through tcp_sk(sk)->bpf_sock_ops_cb_flags, and does not set is_locked_tcp_sock for the msk (unlike tcp_call_bpf()). That flag authorizes the verifier's direct tcp_sock-offset field accesses; since the msk is not a tcp_sock, leaving it at the default 0 is safe. v1: Link: https://patchwork.kernel.org/project/mptcp/cover/20260713095735.1222033-1-gang.yan@linux.dev/ Gang Yan (7): mptcp: drop unused @max arg of __mptcp_setsockopt_set_val mptcp: take TCP_MAXSEG handling into __mptcp_setsockopt_set_val mptcp: use sockopt_lock/release_sock in sockopt mptcp: reject sockopt requiring ssks' lock in BPF context mptcp: enable bpf_setsockopt on the master socket mptcp: add TCP_CONNECT_CB sock_ops hook selftests: bpf: verify mptcp bpf_setsockopt from TCP_CONNECT_CB include/net/mptcp.h | 9 ++ net/core/filter.c | 10 ++ net/ipv4/tcp.c | 1 + net/mptcp/protocol.c | 6 + net/mptcp/protocol.h | 29 ++++ net/mptcp/sockopt.c | 149 +++++++++--------- .../testing/selftests/bpf/prog_tests/mptcp.c | 61 +++++++ .../selftests/bpf/progs/mptcp_setsockopt.c | 32 ++++ 8 files changed, 223 insertions(+), 74 deletions(-) create mode 100644 tools/testing/selftests/bpf/progs/mptcp_setsockopt.c -- 2.43.0 From: Gang Yan <yangang@kylinos.cn> The @max argument is never read in the function body. Remove it and the MAX_TCP_KEEP* values passed by the TCP_KEEPIDLE/INTVL/KEEPCNT callers. Signed-off-by: Gang Yan <yangang@kylinos.cn> --- net/mptcp/sockopt.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c index XXXXXXX..XXXXXXX 100644 --- a/net/mptcp/sockopt.c +++ b/net/mptcp/sockopt.c @@ -XXX,XX +XXX,XX @@ static int mptcp_setsockopt_sol_tcp_congestion(struct mptcp_sock *msk, sockptr_t return ret; } -static int __mptcp_setsockopt_set_val(struct mptcp_sock *msk, int max, +static int __mptcp_setsockopt_set_val(struct mptcp_sock *msk, int (*set_val)(struct sock *, int), int *msk_val, int val) { @@ -XXX,XX +XXX,XX @@ static int mptcp_setsockopt_sol_tcp(struct mptcp_sock *msk, int optname, ret = __mptcp_setsockopt_sol_tcp_nodelay(msk, val); break; case TCP_KEEPIDLE: - ret = __mptcp_setsockopt_set_val(msk, MAX_TCP_KEEPIDLE, - &tcp_sock_set_keepidle_locked, + ret = __mptcp_setsockopt_set_val(msk, &tcp_sock_set_keepidle_locked, &msk->keepalive_idle, val); break; case TCP_KEEPINTVL: - ret = __mptcp_setsockopt_set_val(msk, MAX_TCP_KEEPINTVL, - &tcp_sock_set_keepintvl, + ret = __mptcp_setsockopt_set_val(msk, &tcp_sock_set_keepintvl, &msk->keepalive_intvl, val); break; case TCP_KEEPCNT: - ret = __mptcp_setsockopt_set_val(msk, MAX_TCP_KEEPCNT, - &tcp_sock_set_keepcnt, + ret = __mptcp_setsockopt_set_val(msk, &tcp_sock_set_keepcnt, &msk->keepalive_cnt, val); break; -- 2.43.0 From: Gang Yan <yangang@kylinos.cn> mptcp_setsockopt_all_sf is only used in 'TCP_MAXSEG', and it can be replaced with __mptcp_setsockopt_set_val. Signed-off-by: Gang Yan <yangang@kylinos.cn> --- net/ipv4/tcp.c | 1 + net/mptcp/sockopt.c | 27 ++------------------------- 2 files changed, 3 insertions(+), 25 deletions(-) diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c index XXXXXXX..XXXXXXX 100644 --- a/net/ipv4/tcp.c +++ b/net/ipv4/tcp.c @@ -XXX,XX +XXX,XX @@ int tcp_sock_set_maxseg(struct sock *sk, int val) WRITE_ONCE(tcp_sk(sk)->rx_opt.user_mss, val); return 0; } +EXPORT_SYMBOL(tcp_sock_set_maxseg); /* * Socket option code for TCP. diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c index XXXXXXX..XXXXXXX 100644 --- a/net/mptcp/sockopt.c +++ b/net/mptcp/sockopt.c @@ -XXX,XX +XXX,XX @@ static int mptcp_setsockopt_first_sf_only(struct mptcp_sock *msk, int level, int return ret; } -static int mptcp_setsockopt_all_sf(struct mptcp_sock *msk, int level, - int optname, sockptr_t optval, - unsigned int optlen) -{ - struct mptcp_subflow_context *subflow; - int ret = 0; - - mptcp_for_each_subflow(msk, subflow) { - struct sock *ssk = mptcp_subflow_tcp_sock(subflow); - int err; - - err = tcp_setsockopt(ssk, level, optname, optval, optlen); - if (err < 0 && ret == 0) - ret = err; - } - - if (!ret) - sockopt_seq_inc(msk); - - return ret; -} - static int mptcp_setsockopt_sol_tcp(struct mptcp_sock *msk, int optname, sockptr_t optval, unsigned int optlen) { @@ -XXX,XX +XXX,XX @@ static int mptcp_setsockopt_sol_tcp(struct mptcp_sock *msk, int optname, val); break; case TCP_MAXSEG: - msk->maxseg = val; - ret = mptcp_setsockopt_all_sf(msk, SOL_TCP, optname, optval, - optlen); + ret = __mptcp_setsockopt_set_val(msk, &tcp_sock_set_maxseg, + &msk->maxseg, val); break; default: ret = -ENOPROTOOPT; -- 2.43.0 From: Gang Yan <yangang@kylinos.cn> TCP and the core socket layer use sockopt_lock_sock() / sockopt_release_sock() in their setsockopt and getsockopt handlers. Switch the MPTCP socket (msk) level lock_sock()/release_sock() calls to use the BPF-aware wrappers, making the MPTCP sockopt codepaths consistent with the rest of the networking stack. Signed-off-by: Gang Yan <yangang@kylinos.cn> --- net/mptcp/sockopt.c | 84 ++++++++++++++++++++++----------------------- 1 file changed, 42 insertions(+), 42 deletions(-) diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c index XXXXXXX..XXXXXXX 100644 --- a/net/mptcp/sockopt.c +++ b/net/mptcp/sockopt.c @@ -XXX,XX +XXX,XX @@ static void mptcp_sol_socket_sync_intval(struct mptcp_sock *msk, int optname, in struct mptcp_subflow_context *subflow; struct sock *sk = (struct sock *)msk; - lock_sock(sk); + sockopt_lock_sock(sk); sockopt_seq_inc(msk); mptcp_for_each_subflow(msk, subflow) { @@ -XXX,XX +XXX,XX @@ static void mptcp_sol_socket_sync_intval(struct mptcp_sock *msk, int optname, in unlock_sock_fast(ssk, slow); } - release_sock(sk); + sockopt_release_sock(sk); } static int mptcp_sol_socket_intval(struct mptcp_sock *msk, int optname, int val) @@ -XXX,XX +XXX,XX @@ static int mptcp_setsockopt_sol_socket_tstamp(struct mptcp_sock *msk, int optnam if (ret) return ret; - lock_sock(sk); + sockopt_lock_sock(sk); mptcp_for_each_subflow(msk, subflow) { struct sock *ssk = mptcp_subflow_tcp_sock(subflow); @@ -XXX,XX +XXX,XX @@ static int mptcp_setsockopt_sol_socket_tstamp(struct mptcp_sock *msk, int optnam release_sock(ssk); } - release_sock(sk); + sockopt_release_sock(sk); return 0; } @@ -XXX,XX +XXX,XX @@ static int mptcp_setsockopt_sol_socket_timestamping(struct mptcp_sock *msk, if (ret) return ret; - lock_sock(sk); + sockopt_lock_sock(sk); mptcp_for_each_subflow(msk, subflow) { struct sock *ssk = mptcp_subflow_tcp_sock(subflow); @@ -XXX,XX +XXX,XX @@ static int mptcp_setsockopt_sol_socket_timestamping(struct mptcp_sock *msk, ret = err; } - release_sock(sk); + sockopt_release_sock(sk); return ret; } @@ -XXX,XX +XXX,XX @@ static int mptcp_setsockopt_sol_socket_linger(struct mptcp_sock *msk, sockptr_t if (ret) return ret; - lock_sock(sk); + sockopt_lock_sock(sk); sockopt_seq_inc(msk); mptcp_for_each_subflow(msk, subflow) { struct sock *ssk = mptcp_subflow_tcp_sock(subflow); @@ -XXX,XX +XXX,XX @@ static int mptcp_setsockopt_sol_socket_linger(struct mptcp_sock *msk, sockptr_t unlock_sock_fast(ssk, slow); } - release_sock(sk); + sockopt_release_sock(sk); return 0; } @@ -XXX,XX +XXX,XX @@ static int mptcp_setsockopt_sol_socket(struct mptcp_sock *msk, int optname, case SO_REUSEADDR: case SO_BINDTODEVICE: case SO_BINDTOIFINDEX: - lock_sock(sk); + sockopt_lock_sock(sk); ssk = __mptcp_nmpc_sk(msk); if (IS_ERR(ssk)) { - release_sock(sk); + sockopt_release_sock(sk); return PTR_ERR(ssk); } @@ -XXX,XX +XXX,XX @@ static int mptcp_setsockopt_sol_socket(struct mptcp_sock *msk, int optname, else if (optname == SO_BINDTOIFINDEX) sk->sk_bound_dev_if = ssk->sk_bound_dev_if; } - release_sock(sk); + sockopt_release_sock(sk); return ret; case SO_KEEPALIVE: case SO_PRIORITY: @@ -XXX,XX +XXX,XX @@ static int mptcp_setsockopt_v6(struct mptcp_sock *msk, int optname, case IPV6_V6ONLY: case IPV6_TRANSPARENT: case IPV6_FREEBIND: - lock_sock(sk); + sockopt_lock_sock(sk); ssk = __mptcp_nmpc_sk(msk); if (IS_ERR(ssk)) { - release_sock(sk); + sockopt_release_sock(sk); return PTR_ERR(ssk); } ret = tcp_setsockopt(ssk, SOL_IPV6, optname, optval, optlen); if (ret != 0) { - release_sock(sk); + sockopt_release_sock(sk); return ret; } @@ -XXX,XX +XXX,XX @@ static int mptcp_setsockopt_v6(struct mptcp_sock *msk, int optname, break; } - release_sock(sk); + sockopt_release_sock(sk); break; } @@ -XXX,XX +XXX,XX @@ static int mptcp_setsockopt_sol_tcp_congestion(struct mptcp_sock *msk, sockptr_t cap_net_admin = ns_capable(sock_net(sk)->user_ns, CAP_NET_ADMIN); ret = 0; - lock_sock(sk); + sockopt_lock_sock(sk); sockopt_seq_inc(msk); mptcp_for_each_subflow(msk, subflow) { struct sock *ssk = mptcp_subflow_tcp_sock(subflow); @@ -XXX,XX +XXX,XX @@ static int mptcp_setsockopt_sol_tcp_congestion(struct mptcp_sock *msk, sockptr_t if (ret == 0) strscpy(msk->ca_name, name, sizeof(msk->ca_name)); - release_sock(sk); + sockopt_release_sock(sk); return ret; } @@ -XXX,XX +XXX,XX @@ static int mptcp_setsockopt_sol_ip_set(struct mptcp_sock *msk, int optname, if (err != 0) return err; - lock_sock(sk); + sockopt_lock_sock(sk); ssk = __mptcp_nmpc_sk(msk); if (IS_ERR(ssk)) { - release_sock(sk); + sockopt_release_sock(sk); return PTR_ERR(ssk); } @@ -XXX,XX +XXX,XX @@ static int mptcp_setsockopt_sol_ip_set(struct mptcp_sock *msk, int optname, READ_ONCE(inet_sk(sk)->local_port_range)); break; default: - release_sock(sk); + sockopt_release_sock(sk); WARN_ON_ONCE(1); return -EOPNOTSUPP; } sockopt_seq_inc(msk); - release_sock(sk); + sockopt_release_sock(sk); return 0; } @@ -XXX,XX +XXX,XX @@ static int mptcp_setsockopt_v4_set_tos(struct mptcp_sock *msk, int optname, if (err != 0) return err; - lock_sock(sk); + sockopt_lock_sock(sk); sockopt_seq_inc(msk); val = READ_ONCE(inet_sk(sk)->tos); mptcp_for_each_subflow(msk, subflow) { @@ -XXX,XX +XXX,XX @@ static int mptcp_setsockopt_v4_set_tos(struct mptcp_sock *msk, int optname, __ip_sock_set_tos(ssk, val); unlock_sock_fast(ssk, slow); } - release_sock(sk); + sockopt_release_sock(sk); return 0; } @@ -XXX,XX +XXX,XX @@ static int mptcp_setsockopt_first_sf_only(struct mptcp_sock *msk, int level, int int ret; /* Limit to first subflow, before the connection establishment */ - lock_sock(sk); + sockopt_lock_sock(sk); ssk = __mptcp_nmpc_sk(msk); if (IS_ERR(ssk)) { ret = PTR_ERR(ssk); @@ -XXX,XX +XXX,XX @@ static int mptcp_setsockopt_first_sf_only(struct mptcp_sock *msk, int level, int ret = tcp_setsockopt(ssk, level, optname, optval, optlen); unlock: - release_sock(sk); + sockopt_release_sock(sk); return ret; } @@ -XXX,XX +XXX,XX @@ static int mptcp_setsockopt_sol_tcp(struct mptcp_sock *msk, int optname, if (ret) return ret; - lock_sock(sk); + sockopt_lock_sock(sk); switch (optname) { case TCP_INQ: if (val < 0 || val > 1) @@ -XXX,XX +XXX,XX @@ static int mptcp_setsockopt_sol_tcp(struct mptcp_sock *msk, int optname, ret = -ENOPROTOOPT; } - release_sock(sk); + sockopt_release_sock(sk); return ret; } @@ -XXX,XX +XXX,XX @@ int mptcp_setsockopt(struct sock *sk, int level, int optname, * is in TCP fallback, when TCP socket options are passed through * to the one remaining subflow. */ - lock_sock(sk); + sockopt_lock_sock(sk); ssk = __mptcp_tcp_fallback(msk); - release_sock(sk); + sockopt_release_sock(sk); if (ssk) return tcp_setsockopt(ssk, level, optname, optval, optlen); @@ -XXX,XX +XXX,XX @@ static int mptcp_getsockopt_first_sf_only(struct mptcp_sock *msk, int level, int struct sock *ssk; int ret; - lock_sock(sk); + sockopt_lock_sock(sk); ssk = msk->first; if (ssk) goto get; @@ -XXX,XX +XXX,XX @@ static int mptcp_getsockopt_first_sf_only(struct mptcp_sock *msk, int level, int ret = tcp_getsockopt(ssk, level, optname, optval, optlen); out: - release_sock(sk); + sockopt_release_sock(sk); return ret; } @@ -XXX,XX +XXX,XX @@ static int mptcp_getsockopt_tcpinfo(struct mptcp_sock *msk, char __user *optval, infoptr = optval + sfd.size_subflow_data; - lock_sock(sk); + sockopt_lock_sock(sk); mptcp_for_each_subflow(msk, subflow) { struct sock *ssk = mptcp_subflow_tcp_sock(subflow); @@ -XXX,XX +XXX,XX @@ static int mptcp_getsockopt_tcpinfo(struct mptcp_sock *msk, char __user *optval, tcp_get_info(ssk, &info); if (copy_to_user(infoptr, &info, sfd.size_user)) { - release_sock(sk); + sockopt_release_sock(sk); return -EFAULT; } @@ -XXX,XX +XXX,XX @@ static int mptcp_getsockopt_tcpinfo(struct mptcp_sock *msk, char __user *optval, } } - release_sock(sk); + sockopt_release_sock(sk); sfd.num_subflows = sfcount; @@ -XXX,XX +XXX,XX @@ static int mptcp_getsockopt_subflow_addrs(struct mptcp_sock *msk, char __user *o addrptr = optval + sfd.size_subflow_data; - lock_sock(sk); + sockopt_lock_sock(sk); mptcp_for_each_subflow(msk, subflow) { struct sock *ssk = mptcp_subflow_tcp_sock(subflow); @@ -XXX,XX +XXX,XX @@ static int mptcp_getsockopt_subflow_addrs(struct mptcp_sock *msk, char __user *o mptcp_get_sub_addrs(ssk, &a); if (copy_to_user(addrptr, &a, sfd.size_user)) { - release_sock(sk); + sockopt_release_sock(sk); return -EFAULT; } @@ -XXX,XX +XXX,XX @@ static int mptcp_getsockopt_subflow_addrs(struct mptcp_sock *msk, char __user *o } } - release_sock(sk); + sockopt_release_sock(sk); sfd.num_subflows = sfcount; @@ -XXX,XX +XXX,XX @@ static int mptcp_getsockopt_full_info(struct mptcp_sock *msk, char __user *optva sizeof(struct mptcp_subflow_info)); tcpinfoptr = u64_to_user_ptr(mfi.tcp_info); - lock_sock(sk); + sockopt_lock_sock(sk); mptcp_for_each_subflow(msk, subflow) { struct sock *ssk = mptcp_subflow_tcp_sock(subflow); struct mptcp_subflow_info sfinfo; @@ -XXX,XX +XXX,XX @@ static int mptcp_getsockopt_full_info(struct mptcp_sock *msk, char __user *optva tcpinfoptr += mfi.size_tcpinfo_user; sfinfoptr += mfi.size_sfinfo_user; } - release_sock(sk); + sockopt_release_sock(sk); mfi.num_subflows = sfcount; if (mptcp_put_full_info(&mfi, optval, copylen, optlen)) @@ -XXX,XX +XXX,XX @@ static int mptcp_getsockopt_full_info(struct mptcp_sock *msk, char __user *optva return 0; fail_release: - release_sock(sk); + sockopt_release_sock(sk); return -EFAULT; } @@ -XXX,XX +XXX,XX @@ int mptcp_getsockopt(struct sock *sk, int level, int optname, * is in TCP fallback, when socket options are passed through * to the one remaining subflow. */ - lock_sock(sk); + sockopt_lock_sock(sk); ssk = __mptcp_tcp_fallback(msk); - release_sock(sk); + sockopt_release_sock(sk); if (ssk) return tcp_getsockopt(ssk, level, optname, optval, option); -- 2.43.0 From: Gang Yan <yangang@kylinos.cn> Several MPTCP setsockopt handlers need to acquire the subflow lock via lock_sock(ssk) to propagate settings to each subflow. This lock can sleep and is therefore not usable in BPF context where sleeping is forbidden. The short-term solution is to make any sockopt operation that requires subflow-level lock fail with -EOPNOTSUPP when called from BPF context. Signed-off-by: Gang Yan <yangang@kylinos.cn> --- net/mptcp/sockopt.c | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c index XXXXXXX..XXXXXXX 100644 --- a/net/mptcp/sockopt.c +++ b/net/mptcp/sockopt.c @@ -XXX,XX +XXX,XX @@ static int mptcp_setsockopt_sol_socket_int(struct mptcp_sock *msk, int optname, if (ret) return ret; + if (has_current_bpf_ctx()) + return -EOPNOTSUPP; + switch (optname) { case SO_KEEPALIVE: case SO_DEBUG: @@ -XXX,XX +XXX,XX @@ static int mptcp_setsockopt_sol_socket_timestamping(struct mptcp_sock *msk, struct so_timestamping timestamping; int ret; + if (has_current_bpf_ctx()) + return -EOPNOTSUPP; + if (optlen == sizeof(timestamping)) { if (copy_from_sockptr(&timestamping, optval, sizeof(timestamping))) @@ -XXX,XX +XXX,XX @@ static int mptcp_setsockopt_sol_socket_linger(struct mptcp_sock *msk, sockptr_t sockptr_t kopt; int ret; + if (has_current_bpf_ctx()) + return -EOPNOTSUPP; + if (optlen < sizeof(ling)) return -EINVAL; @@ -XXX,XX +XXX,XX @@ static int mptcp_setsockopt_sol_tcp_congestion(struct mptcp_sock *msk, sockptr_t bool cap_net_admin; int ret; + if (has_current_bpf_ctx()) + return -EOPNOTSUPP; + if (optlen < 1) return -EINVAL; @@ -XXX,XX +XXX,XX @@ static int __mptcp_setsockopt_set_val(struct mptcp_sock *msk, struct mptcp_subflow_context *subflow; int err = 0; + if (has_current_bpf_ctx()) + return -EOPNOTSUPP; + mptcp_for_each_subflow(msk, subflow) { struct sock *ssk = mptcp_subflow_tcp_sock(subflow); int ret; @@ -XXX,XX +XXX,XX @@ static int __mptcp_setsockopt_sol_tcp_cork(struct mptcp_sock *msk, int val) struct mptcp_subflow_context *subflow; struct sock *sk = (struct sock *)msk; + if (has_current_bpf_ctx()) + return -EOPNOTSUPP; + sockopt_seq_inc(msk); msk->cork = !!val; mptcp_for_each_subflow(msk, subflow) { @@ -XXX,XX +XXX,XX @@ static int __mptcp_setsockopt_sol_tcp_nodelay(struct mptcp_sock *msk, int val) struct mptcp_subflow_context *subflow; struct sock *sk = (struct sock *)msk; + if (has_current_bpf_ctx()) + return -EOPNOTSUPP; + sockopt_seq_inc(msk); msk->nodelay = !!val; mptcp_for_each_subflow(msk, subflow) { @@ -XXX,XX +XXX,XX @@ static int mptcp_setsockopt_v4_set_tos(struct mptcp_sock *msk, int optname, struct sock *sk = (struct sock *)msk; int err, val; + if (has_current_bpf_ctx()) + return -EOPNOTSUPP; + err = ip_setsockopt(sk, SOL_IP, optname, optval, optlen); if (err != 0) @@ -XXX,XX +XXX,XX @@ int mptcp_set_rcvlowat(struct sock *sk, int val) if (sk->sk_protocol == IPPROTO_TCP) return -EINVAL; + if (has_current_bpf_ctx()) + return -EOPNOTSUPP; + if (sk->sk_userlocks & SOCK_RCVBUF_LOCK) cap = sk->sk_rcvbuf >> 1; else -- 2.43.0 From: Gang Yan <yangang@kylinos.cn> bpf_setsockopt() currently cannot be used on mptcp master sockets: __bpf_setsockopt() dispatches by level to the protocol-agnostic sol_*_sockopt() helpers, which either reject the msk (sk_protocol == IPPROTO_MPTCP) and the ssk (sk_is_tcp() is false) or bypass mptcp's own dispatch (e.g. SOL_IP going straight to do_ip_setsockopt()). This patch routes any level to mptcp_setsockopt(), which already handles all levels. Signed-off-by: Gang Yan <yangang@kylinos.cn> --- include/net/mptcp.h | 9 +++++++++ net/core/filter.c | 7 +++++++ 2 files changed, 16 insertions(+) diff --git a/include/net/mptcp.h b/include/net/mptcp.h index XXXXXXX..XXXXXXX 100644 --- a/include/net/mptcp.h +++ b/include/net/mptcp.h @@ -XXX,XX +XXX,XX @@ static inline __be32 mptcp_reset_option(const struct sk_buff *skb) } void mptcp_active_detect_blackhole(struct sock *sk, bool expired); + +int mptcp_setsockopt(struct sock *sk, int level, int optname, + sockptr_t optval, unsigned int optlen); #else static inline void mptcp_init(void) @@ -XXX,XX +XXX,XX @@ static inline struct request_sock *mptcp_subflow_reqsk_alloc(const struct reques static inline __be32 mptcp_reset_option(const struct sk_buff *skb) { return htonl(0u); } static inline void mptcp_active_detect_blackhole(struct sock *sk, bool expired) { } + +static inline int mptcp_setsockopt(struct sock *sk, int level, int optname, + sockptr_t optval, unsigned int optlen) +{ + return -EINVAL; +} #endif /* CONFIG_MPTCP */ #if IS_ENABLED(CONFIG_MPTCP_IPV6) diff --git a/net/core/filter.c b/net/core/filter.c index XXXXXXX..XXXXXXX 100644 --- a/net/core/filter.c +++ b/net/core/filter.c @@ -XXX,XX +XXX,XX @@ static int __bpf_setsockopt(struct sock *sk, int level, int optname, if (!sk_fullsock(sk)) return -EINVAL; + /* Route any bpf_setsockopt on the mptcp socket to mptcp_setsockopt, + * which handles all levels. + */ + if (IS_ENABLED(CONFIG_MPTCP) && sk->sk_protocol == IPPROTO_MPTCP) + return mptcp_setsockopt(sk, level, optname, + KERNEL_SOCKPTR(optval), optlen); + if (level == SOL_SOCKET) return sol_socket_sockopt(sk, optname, optval, &optlen, false); else if (IS_ENABLED(CONFIG_INET) && level == SOL_IP) -- 2.43.0 From: Gang Yan <yangang@kylinos.cn> This patch adds a helper named 'mptcp_call_bpf' like tcp_call_bpf. Invoke the new helper from mptcp_connect() with BPF_SOCK_OPS_TCP_CONNECT_CB, placed after the subflow lock is acquired and before tcp_connect(). At this point the msk lock is held by __inet_stream_connect(), mirroring the placement of TCP_CONNECT_CB in tcp_v4_connect()/tcp_v6_connect(). 'bpf_sock_ops_cb_flags_set' can be called via msk, so using sk_is_tcp() to avoid this issue. Signed-off-by: Gang Yan <yangang@kylinos.cn> --- Note: I'm using CONFIG_BPF_JIT here because it is used in bpf.c for conditional compilation. However, maybe the macro in bpf.c should be changed to CONFIG_BPF instead? WDYT? --- net/core/filter.c | 3 +++ net/mptcp/protocol.c | 6 ++++++ net/mptcp/protocol.h | 29 +++++++++++++++++++++++++++++ 3 files changed, 38 insertions(+) diff --git a/net/core/filter.c b/net/core/filter.c index XXXXXXX..XXXXXXX 100644 --- a/net/core/filter.c +++ b/net/core/filter.c @@ -XXX,XX +XXX,XX @@ BPF_CALL_2(bpf_sock_ops_cb_flags_set, struct bpf_sock_ops_kern *, bpf_sock, if (!IS_ENABLED(CONFIG_INET) || !sk_fullsock(sk)) return -EINVAL; + if (!sk_is_tcp(sk)) + return -EOPNOTSUPP; + tcp_sk(sk)->bpf_sock_ops_cb_flags = val; return argval & (~BPF_SOCK_OPS_ALL_CB_FLAGS); diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c index XXXXXXX..XXXXXXX 100644 --- a/net/mptcp/protocol.c +++ b/net/mptcp/protocol.c @@ -XXX,XX +XXX,XX @@ static int mptcp_connect(struct sock *sk, struct sockaddr_unsized *uaddr, if (!msk->fastopening) lock_sock(ssk); + /* Notify cgroup BPF on the msk before initiating the subflow connect. + * Mirrors BPF_SOCK_OPS_TCP_CONNECT_CB; msk lock is held by the + * caller (__inet_stream_connect) and ssk is held before. + */ + mptcp_call_bpf(sk, BPF_SOCK_OPS_TCP_CONNECT_CB, 0, NULL); + /* the following mirrors closely a very small chunk of code from * __inet_stream_connect() */ diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h index XXXXXXX..XXXXXXX 100644 --- a/net/mptcp/protocol.h +++ b/net/mptcp/protocol.h @@ -XXX,XX +XXX,XX @@ mptcp_token_join_cookie_init_state(struct mptcp_subflow_request_sock *subflow_re static inline void mptcp_join_cookie_init(void) {} #endif +#ifdef CONFIG_BPF_JIT +static inline int mptcp_call_bpf(struct sock *sk, int op, u32 nargs, u32 *args) +{ + struct bpf_sock_ops_kern sock_ops; + int ret; + + memset(&sock_ops, 0, offsetof(struct bpf_sock_ops_kern, temp)); + + if (sk_fullsock(sk)) { + sock_ops.is_fullsock = 1; + sock_owned_by_me(sk); + } + + sock_ops.sk = sk; + sock_ops.op = op; + + if (nargs > 0) + memcpy(sock_ops.args, args, nargs * sizeof(*args)); + + ret = BPF_CGROUP_RUN_PROG_SOCK_OPS(&sock_ops); + return ret == 0 ? sock_ops.reply : -1; +} +#else +static inline int mptcp_call_bpf(struct sock *sk, int op, u32 nargs, u32 *args) +{ + return -1; +} +#endif + #endif /* __MPTCP_PROTOCOL_H */ -- 2.43.0 From: Gang Yan <yangang@kylinos.cn> Add a BPF sockops program attached to BPF_CGROUP_SOCK_OPS that fires on BPF_SOCK_OPS_TCP_CONNECT_CB and exercises bpf_setsockopt() on the mptcp master socket (msk). Two scenarios are covered by the new "setsockopt" subtest: - TCP_INQ must succeed and the value (1) must be observable from userspace via getsockopt() on the mptcp socket. - TCP_CONGESTION needs the subflow lock and is therefore rejected in bpf context with -EOPNOTSUPP. Signed-off-by: Gang Yan <yangang@kylinos.cn> --- .../testing/selftests/bpf/prog_tests/mptcp.c | 61 +++++++++++++++++++ .../selftests/bpf/progs/mptcp_setsockopt.c | 32 ++++++++++ 2 files changed, 93 insertions(+) create mode 100644 tools/testing/selftests/bpf/progs/mptcp_setsockopt.c diff --git a/tools/testing/selftests/bpf/prog_tests/mptcp.c b/tools/testing/selftests/bpf/prog_tests/mptcp.c index XXXXXXX..XXXXXXX 100644 --- a/tools/testing/selftests/bpf/prog_tests/mptcp.c +++ b/tools/testing/selftests/bpf/prog_tests/mptcp.c @@ -XXX,XX +XXX,XX @@ #include "mptcp_bpf_rr.skel.h" #include "mptcp_bpf_red.skel.h" #include "mptcp_bpf_burst.skel.h" +#include "mptcp_setsockopt.skel.h" #define NS_TEST "mptcp_ns" #define ADDR_1 "10.0.1.1" @@ -XXX,XX +XXX,XX @@ static void test_burst(void) mptcp_bpf_burst__destroy(skel); } +static void test_setsockopt(void) +{ + struct mptcp_setsockopt *skel; + struct netns_obj *netns; + int cgroup_fd, server_fd, client_fd; + int err; + int inq; + socklen_t len; + + cgroup_fd = test__join_cgroup("/mptcp_setsockopt"); + if (!ASSERT_OK_FD(cgroup_fd, "join_cgroup")) + return; + + skel = mptcp_setsockopt__open_and_load(); + if (!ASSERT_OK_PTR(skel, "skel_open_load")) + goto close_cgroup; + + skel->links.mptcp_connect_cb = + bpf_program__attach_cgroup(skel->progs.mptcp_connect_cb, + cgroup_fd); + if (!ASSERT_OK_PTR(skel->links.mptcp_connect_cb, "attach connect_cb")) + goto skel_destroy; + + netns = netns_new(NS_TEST, true); + if (!ASSERT_OK_PTR(netns, "netns_new")) + goto skel_destroy; + + server_fd = start_mptcp_server(AF_INET, NULL, 0, 0); + if (!ASSERT_OK_FD(server_fd, "start_mptcp_server")) + goto close_netns; + + client_fd = connect_to_fd(server_fd, 0); + if (!ASSERT_OK_FD(client_fd, "connect_to_fd")) + goto close_server; + + /* TCP_INQ should be set successfullly */ + ASSERT_EQ(skel->bss->connect_cb_inq_ret, 0, "connect_cb TCP_INQ ret"); + + len = sizeof(inq); + err = getsockopt(client_fd, SOL_TCP, TCP_INQ, &inq, &len); + if (ASSERT_OK(err, "getsockopt TCP_INQ")) + ASSERT_EQ(inq, 1, "TCP_INQ value"); + + /* TCP_CONGESTION should be -EOPNOTSUPP */ + ASSERT_EQ(skel->bss->connect_cb_cc_ret, -EOPNOTSUPP, + "connect_cb TCP_CONGESTION ret"); + + close(client_fd); +close_server: + close(server_fd); +close_netns: + netns_free(netns); +skel_destroy: + mptcp_setsockopt__destroy(skel); +close_cgroup: + close(cgroup_fd); +} + void test_mptcp(void) { if (test__start_subtest("base")) @@ -XXX,XX +XXX,XX @@ void test_mptcp(void) test_red(); if (test__start_subtest("burst")) test_burst(); + if (test__start_subtest("setsockopt")) + test_setsockopt(); } diff --git a/tools/testing/selftests/bpf/progs/mptcp_setsockopt.c b/tools/testing/selftests/bpf/progs/mptcp_setsockopt.c new file mode 100644 index XXXXXXX..XXXXXXX --- /dev/null +++ b/tools/testing/selftests/bpf/progs/mptcp_setsockopt.c @@ -XXX,XX +XXX,XX @@ +#include "bpf_tracing_net.h" +#include "mptcp_bpf.h" + +#ifndef TCP_INQ +#define TCP_INQ 36 +#endif + +int connect_cb_inq_ret; +int connect_cb_cc_ret; + +char cc_reno[TCP_CA_NAME_MAX] = "reno"; + +SEC("sockops") +int mptcp_connect_cb(struct bpf_sock_ops *skops) +{ + struct bpf_sock *sk = skops->sk; + int one = 1; + + if (skops->op != BPF_SOCK_OPS_TCP_CONNECT_CB) + return 1; + + if (!sk || sk->protocol != IPPROTO_MPTCP) + return 1; + + connect_cb_inq_ret = + bpf_setsockopt(skops, SOL_TCP, TCP_INQ, &one, sizeof(one)); + connect_cb_cc_ret = + bpf_setsockopt(skops, SOL_TCP, TCP_CONGESTION, + cc_reno, sizeof(cc_reno)); + + return 1; +} -- 2.43.0 From: Gang Yan <yangang@kylinos.cn> Hi, Matt, Geliang, Paolo Patch 3-4 have been reviewed by Paolo before, and ready for merge [1][2]. Changelog: v3: - Patch 2 keeps the mptcp_setsockopt_all_sf as Geliang suggested. v2: - Patches 1 and 2 are new in this series; they address TCP_MAXSEG handling in the bpf_setsockopt() path. - Patch 4 adds an early return to fix msk->sk_rcvlowat being unexpectedly modified, an issue seen in v1. - Patch 5 makes the hook safe for the non-tcp master socket: it guards bpf_sock_ops_cb_flags_set() with sk_is_tcp() to prevent out-of-bounds heap reads/writes through tcp_sk(sk)->bpf_sock_ops_cb_flags, and does not set is_locked_tcp_sock for the msk (unlike tcp_call_bpf()). That flag authorizes the verifier's direct tcp_sock-offset field accesses; since the msk is not a tcp_sock, leaving it at the default 0 is safe. v1: Link: https://patchwork.kernel.org/project/mptcp/cover/20260713095735.1222033-1-gang.yan@linux.dev/ [1] https://patchwork.kernel.org/project/mptcp/patch/20260522-sockopt_lock-v5-2-108629a46e98@kylinos.cn/ [2] https://patchwork.kernel.org/project/mptcp/patch/20260522-sockopt_lock-v5-4-108629a46e98@kylinos.cn/ Gang Yan (7): mptcp: drop unused @max arg of __mptcp_setsockopt_set_val mptcp: take TCP_MAXSEG handling into __mptcp_setsockopt_set_val mptcp: use sockopt_lock/release_sock in sockopt mptcp: reject sockopt requiring ssks' lock in BPF context mptcp: enable bpf_setsockopt on the master socket mptcp: add TCP_CONNECT_CB sock_ops hook selftests: bpf: verify mptcp bpf_setsockopt from TCP_CONNECT_CB include/net/mptcp.h | 9 ++ net/core/filter.c | 10 ++ net/ipv4/tcp.c | 1 + net/mptcp/protocol.c | 6 + net/mptcp/protocol.h | 29 ++++ net/mptcp/sockopt.c | 127 +++++++++++------- .../testing/selftests/bpf/prog_tests/mptcp.c | 61 +++++++++ .../selftests/bpf/progs/mptcp_setsockopt.c | 32 +++++ 8 files changed, 223 insertions(+), 52 deletions(-) create mode 100644 tools/testing/selftests/bpf/progs/mptcp_setsockopt.c -- 2.43.0 From: Gang Yan <yangang@kylinos.cn> The @max argument is never read in the function body. Remove it and the MAX_TCP_KEEP* values passed by the TCP_KEEPIDLE/INTVL/KEEPCNT callers. Signed-off-by: Gang Yan <yangang@kylinos.cn> --- net/mptcp/sockopt.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c index XXXXXXX..XXXXXXX 100644 --- a/net/mptcp/sockopt.c +++ b/net/mptcp/sockopt.c @@ -XXX,XX +XXX,XX @@ static int mptcp_setsockopt_sol_tcp_congestion(struct mptcp_sock *msk, sockptr_t return ret; } -static int __mptcp_setsockopt_set_val(struct mptcp_sock *msk, int max, +static int __mptcp_setsockopt_set_val(struct mptcp_sock *msk, int (*set_val)(struct sock *, int), int *msk_val, int val) { @@ -XXX,XX +XXX,XX @@ static int mptcp_setsockopt_sol_tcp(struct mptcp_sock *msk, int optname, ret = __mptcp_setsockopt_sol_tcp_nodelay(msk, val); break; case TCP_KEEPIDLE: - ret = __mptcp_setsockopt_set_val(msk, MAX_TCP_KEEPIDLE, - &tcp_sock_set_keepidle_locked, + ret = __mptcp_setsockopt_set_val(msk, &tcp_sock_set_keepidle_locked, &msk->keepalive_idle, val); break; case TCP_KEEPINTVL: - ret = __mptcp_setsockopt_set_val(msk, MAX_TCP_KEEPINTVL, - &tcp_sock_set_keepintvl, + ret = __mptcp_setsockopt_set_val(msk, &tcp_sock_set_keepintvl, &msk->keepalive_intvl, val); break; case TCP_KEEPCNT: - ret = __mptcp_setsockopt_set_val(msk, MAX_TCP_KEEPCNT, - &tcp_sock_set_keepcnt, + ret = __mptcp_setsockopt_set_val(msk, &tcp_sock_set_keepcnt, &msk->keepalive_cnt, val); break; -- 2.43.0 From: Gang Yan <yangang@kylinos.cn> mptcp_setsockopt_all_sf is only used in 'TCP_MAXSEG', and it can be replaced with __mptcp_setsockopt_set_val. Signed-off-by: Gang Yan <yangang@kylinos.cn> --- net/ipv4/tcp.c | 1 + net/mptcp/sockopt.c | 5 ++--- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c index XXXXXXX..XXXXXXX 100644 --- a/net/ipv4/tcp.c +++ b/net/ipv4/tcp.c @@ -XXX,XX +XXX,XX @@ int tcp_sock_set_maxseg(struct sock *sk, int val) WRITE_ONCE(tcp_sk(sk)->rx_opt.user_mss, val); return 0; } +EXPORT_SYMBOL(tcp_sock_set_maxseg); /* * Socket option code for TCP. diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c index XXXXXXX..XXXXXXX 100644 --- a/net/mptcp/sockopt.c +++ b/net/mptcp/sockopt.c @@ -XXX,XX +XXX,XX @@ static int mptcp_setsockopt_sol_tcp(struct mptcp_sock *msk, int optname, val); break; case TCP_MAXSEG: - msk->maxseg = val; - ret = mptcp_setsockopt_all_sf(msk, SOL_TCP, optname, optval, - optlen); + ret = __mptcp_setsockopt_set_val(msk, &tcp_sock_set_maxseg, + &msk->maxseg, val); break; default: ret = -ENOPROTOOPT; -- 2.43.0 From: Gang Yan <yangang@kylinos.cn> TCP and the core socket layer use sockopt_lock_sock() / sockopt_release_sock() in their setsockopt and getsockopt handlers. Switch the MPTCP socket (msk) level lock_sock()/release_sock() calls to use the BPF-aware wrappers, making the MPTCP sockopt codepaths consistent with the rest of the networking stack. Signed-off-by: Gang Yan <yangang@kylinos.cn> --- net/mptcp/sockopt.c | 84 ++++++++++++++++++++++----------------------- 1 file changed, 42 insertions(+), 42 deletions(-) diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c index XXXXXXX..XXXXXXX 100644 --- a/net/mptcp/sockopt.c +++ b/net/mptcp/sockopt.c @@ -XXX,XX +XXX,XX @@ static void mptcp_sol_socket_sync_intval(struct mptcp_sock *msk, int optname, in struct mptcp_subflow_context *subflow; struct sock *sk = (struct sock *)msk; - lock_sock(sk); + sockopt_lock_sock(sk); sockopt_seq_inc(msk); mptcp_for_each_subflow(msk, subflow) { @@ -XXX,XX +XXX,XX @@ static void mptcp_sol_socket_sync_intval(struct mptcp_sock *msk, int optname, in unlock_sock_fast(ssk, slow); } - release_sock(sk); + sockopt_release_sock(sk); } static int mptcp_sol_socket_intval(struct mptcp_sock *msk, int optname, int val) @@ -XXX,XX +XXX,XX @@ static int mptcp_setsockopt_sol_socket_tstamp(struct mptcp_sock *msk, int optnam if (ret) return ret; - lock_sock(sk); + sockopt_lock_sock(sk); mptcp_for_each_subflow(msk, subflow) { struct sock *ssk = mptcp_subflow_tcp_sock(subflow); @@ -XXX,XX +XXX,XX @@ static int mptcp_setsockopt_sol_socket_tstamp(struct mptcp_sock *msk, int optnam release_sock(ssk); } - release_sock(sk); + sockopt_release_sock(sk); return 0; } @@ -XXX,XX +XXX,XX @@ static int mptcp_setsockopt_sol_socket_timestamping(struct mptcp_sock *msk, if (ret) return ret; - lock_sock(sk); + sockopt_lock_sock(sk); mptcp_for_each_subflow(msk, subflow) { struct sock *ssk = mptcp_subflow_tcp_sock(subflow); @@ -XXX,XX +XXX,XX @@ static int mptcp_setsockopt_sol_socket_timestamping(struct mptcp_sock *msk, ret = err; } - release_sock(sk); + sockopt_release_sock(sk); return ret; } @@ -XXX,XX +XXX,XX @@ static int mptcp_setsockopt_sol_socket_linger(struct mptcp_sock *msk, sockptr_t if (ret) return ret; - lock_sock(sk); + sockopt_lock_sock(sk); sockopt_seq_inc(msk); mptcp_for_each_subflow(msk, subflow) { struct sock *ssk = mptcp_subflow_tcp_sock(subflow); @@ -XXX,XX +XXX,XX @@ static int mptcp_setsockopt_sol_socket_linger(struct mptcp_sock *msk, sockptr_t unlock_sock_fast(ssk, slow); } - release_sock(sk); + sockopt_release_sock(sk); return 0; } @@ -XXX,XX +XXX,XX @@ static int mptcp_setsockopt_sol_socket(struct mptcp_sock *msk, int optname, case SO_REUSEADDR: case SO_BINDTODEVICE: case SO_BINDTOIFINDEX: - lock_sock(sk); + sockopt_lock_sock(sk); ssk = __mptcp_nmpc_sk(msk); if (IS_ERR(ssk)) { - release_sock(sk); + sockopt_release_sock(sk); return PTR_ERR(ssk); } @@ -XXX,XX +XXX,XX @@ static int mptcp_setsockopt_sol_socket(struct mptcp_sock *msk, int optname, else if (optname == SO_BINDTOIFINDEX) sk->sk_bound_dev_if = ssk->sk_bound_dev_if; } - release_sock(sk); + sockopt_release_sock(sk); return ret; case SO_KEEPALIVE: case SO_PRIORITY: @@ -XXX,XX +XXX,XX @@ static int mptcp_setsockopt_v6(struct mptcp_sock *msk, int optname, case IPV6_V6ONLY: case IPV6_TRANSPARENT: case IPV6_FREEBIND: - lock_sock(sk); + sockopt_lock_sock(sk); ssk = __mptcp_nmpc_sk(msk); if (IS_ERR(ssk)) { - release_sock(sk); + sockopt_release_sock(sk); return PTR_ERR(ssk); } ret = tcp_setsockopt(ssk, SOL_IPV6, optname, optval, optlen); if (ret != 0) { - release_sock(sk); + sockopt_release_sock(sk); return ret; } @@ -XXX,XX +XXX,XX @@ static int mptcp_setsockopt_v6(struct mptcp_sock *msk, int optname, break; } - release_sock(sk); + sockopt_release_sock(sk); break; } @@ -XXX,XX +XXX,XX @@ static int mptcp_setsockopt_sol_tcp_congestion(struct mptcp_sock *msk, sockptr_t cap_net_admin = ns_capable(sock_net(sk)->user_ns, CAP_NET_ADMIN); ret = 0; - lock_sock(sk); + sockopt_lock_sock(sk); sockopt_seq_inc(msk); mptcp_for_each_subflow(msk, subflow) { struct sock *ssk = mptcp_subflow_tcp_sock(subflow); @@ -XXX,XX +XXX,XX @@ static int mptcp_setsockopt_sol_tcp_congestion(struct mptcp_sock *msk, sockptr_t if (ret == 0) strscpy(msk->ca_name, name, sizeof(msk->ca_name)); - release_sock(sk); + sockopt_release_sock(sk); return ret; } @@ -XXX,XX +XXX,XX @@ static int mptcp_setsockopt_sol_ip_set(struct mptcp_sock *msk, int optname, if (err != 0) return err; - lock_sock(sk); + sockopt_lock_sock(sk); ssk = __mptcp_nmpc_sk(msk); if (IS_ERR(ssk)) { - release_sock(sk); + sockopt_release_sock(sk); return PTR_ERR(ssk); } @@ -XXX,XX +XXX,XX @@ static int mptcp_setsockopt_sol_ip_set(struct mptcp_sock *msk, int optname, READ_ONCE(inet_sk(sk)->local_port_range)); break; default: - release_sock(sk); + sockopt_release_sock(sk); WARN_ON_ONCE(1); return -EOPNOTSUPP; } sockopt_seq_inc(msk); - release_sock(sk); + sockopt_release_sock(sk); return 0; } @@ -XXX,XX +XXX,XX @@ static int mptcp_setsockopt_v4_set_tos(struct mptcp_sock *msk, int optname, if (err != 0) return err; - lock_sock(sk); + sockopt_lock_sock(sk); sockopt_seq_inc(msk); val = READ_ONCE(inet_sk(sk)->tos); mptcp_for_each_subflow(msk, subflow) { @@ -XXX,XX +XXX,XX @@ static int mptcp_setsockopt_v4_set_tos(struct mptcp_sock *msk, int optname, __ip_sock_set_tos(ssk, val); unlock_sock_fast(ssk, slow); } - release_sock(sk); + sockopt_release_sock(sk); return 0; } @@ -XXX,XX +XXX,XX @@ static int mptcp_setsockopt_first_sf_only(struct mptcp_sock *msk, int level, int int ret; /* Limit to first subflow, before the connection establishment */ - lock_sock(sk); + sockopt_lock_sock(sk); ssk = __mptcp_nmpc_sk(msk); if (IS_ERR(ssk)) { ret = PTR_ERR(ssk); @@ -XXX,XX +XXX,XX @@ static int mptcp_setsockopt_first_sf_only(struct mptcp_sock *msk, int level, int ret = tcp_setsockopt(ssk, level, optname, optval, optlen); unlock: - release_sock(sk); + sockopt_release_sock(sk); return ret; } @@ -XXX,XX +XXX,XX @@ static int mptcp_setsockopt_sol_tcp(struct mptcp_sock *msk, int optname, if (ret) return ret; - lock_sock(sk); + sockopt_lock_sock(sk); switch (optname) { case TCP_INQ: if (val < 0 || val > 1) @@ -XXX,XX +XXX,XX @@ static int mptcp_setsockopt_sol_tcp(struct mptcp_sock *msk, int optname, ret = -ENOPROTOOPT; } - release_sock(sk); + sockopt_release_sock(sk); return ret; } @@ -XXX,XX +XXX,XX @@ int mptcp_setsockopt(struct sock *sk, int level, int optname, * is in TCP fallback, when TCP socket options are passed through * to the one remaining subflow. */ - lock_sock(sk); + sockopt_lock_sock(sk); ssk = __mptcp_tcp_fallback(msk); - release_sock(sk); + sockopt_release_sock(sk); if (ssk) return tcp_setsockopt(ssk, level, optname, optval, optlen); @@ -XXX,XX +XXX,XX @@ static int mptcp_getsockopt_first_sf_only(struct mptcp_sock *msk, int level, int struct sock *ssk; int ret; - lock_sock(sk); + sockopt_lock_sock(sk); ssk = msk->first; if (ssk) goto get; @@ -XXX,XX +XXX,XX @@ static int mptcp_getsockopt_first_sf_only(struct mptcp_sock *msk, int level, int ret = tcp_getsockopt(ssk, level, optname, optval, optlen); out: - release_sock(sk); + sockopt_release_sock(sk); return ret; } @@ -XXX,XX +XXX,XX @@ static int mptcp_getsockopt_tcpinfo(struct mptcp_sock *msk, char __user *optval, infoptr = optval + sfd.size_subflow_data; - lock_sock(sk); + sockopt_lock_sock(sk); mptcp_for_each_subflow(msk, subflow) { struct sock *ssk = mptcp_subflow_tcp_sock(subflow); @@ -XXX,XX +XXX,XX @@ static int mptcp_getsockopt_tcpinfo(struct mptcp_sock *msk, char __user *optval, tcp_get_info(ssk, &info); if (copy_to_user(infoptr, &info, sfd.size_user)) { - release_sock(sk); + sockopt_release_sock(sk); return -EFAULT; } @@ -XXX,XX +XXX,XX @@ static int mptcp_getsockopt_tcpinfo(struct mptcp_sock *msk, char __user *optval, } } - release_sock(sk); + sockopt_release_sock(sk); sfd.num_subflows = sfcount; @@ -XXX,XX +XXX,XX @@ static int mptcp_getsockopt_subflow_addrs(struct mptcp_sock *msk, char __user *o addrptr = optval + sfd.size_subflow_data; - lock_sock(sk); + sockopt_lock_sock(sk); mptcp_for_each_subflow(msk, subflow) { struct sock *ssk = mptcp_subflow_tcp_sock(subflow); @@ -XXX,XX +XXX,XX @@ static int mptcp_getsockopt_subflow_addrs(struct mptcp_sock *msk, char __user *o mptcp_get_sub_addrs(ssk, &a); if (copy_to_user(addrptr, &a, sfd.size_user)) { - release_sock(sk); + sockopt_release_sock(sk); return -EFAULT; } @@ -XXX,XX +XXX,XX @@ static int mptcp_getsockopt_subflow_addrs(struct mptcp_sock *msk, char __user *o } } - release_sock(sk); + sockopt_release_sock(sk); sfd.num_subflows = sfcount; @@ -XXX,XX +XXX,XX @@ static int mptcp_getsockopt_full_info(struct mptcp_sock *msk, char __user *optva sizeof(struct mptcp_subflow_info)); tcpinfoptr = u64_to_user_ptr(mfi.tcp_info); - lock_sock(sk); + sockopt_lock_sock(sk); mptcp_for_each_subflow(msk, subflow) { struct sock *ssk = mptcp_subflow_tcp_sock(subflow); struct mptcp_subflow_info sfinfo; @@ -XXX,XX +XXX,XX @@ static int mptcp_getsockopt_full_info(struct mptcp_sock *msk, char __user *optva tcpinfoptr += mfi.size_tcpinfo_user; sfinfoptr += mfi.size_sfinfo_user; } - release_sock(sk); + sockopt_release_sock(sk); mfi.num_subflows = sfcount; if (mptcp_put_full_info(&mfi, optval, copylen, optlen)) @@ -XXX,XX +XXX,XX @@ static int mptcp_getsockopt_full_info(struct mptcp_sock *msk, char __user *optva return 0; fail_release: - release_sock(sk); + sockopt_release_sock(sk); return -EFAULT; } @@ -XXX,XX +XXX,XX @@ int mptcp_getsockopt(struct sock *sk, int level, int optname, * is in TCP fallback, when socket options are passed through * to the one remaining subflow. */ - lock_sock(sk); + sockopt_lock_sock(sk); ssk = __mptcp_tcp_fallback(msk); - release_sock(sk); + sockopt_release_sock(sk); if (ssk) return tcp_getsockopt(ssk, level, optname, optval, option); -- 2.43.0 From: Gang Yan <yangang@kylinos.cn> Several MPTCP setsockopt handlers need to acquire the subflow lock via lock_sock(ssk) to propagate settings to each subflow. This lock can sleep and is therefore not usable in BPF context where sleeping is forbidden. The short-term solution is to make any sockopt operation that requires subflow-level lock fail with -EOPNOTSUPP when called from BPF context. Signed-off-by: Gang Yan <yangang@kylinos.cn> --- net/mptcp/sockopt.c | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c index XXXXXXX..XXXXXXX 100644 --- a/net/mptcp/sockopt.c +++ b/net/mptcp/sockopt.c @@ -XXX,XX +XXX,XX @@ static int mptcp_setsockopt_sol_socket_int(struct mptcp_sock *msk, int optname, if (ret) return ret; + if (has_current_bpf_ctx()) + return -EOPNOTSUPP; + switch (optname) { case SO_KEEPALIVE: case SO_DEBUG: @@ -XXX,XX +XXX,XX @@ static int mptcp_setsockopt_sol_socket_timestamping(struct mptcp_sock *msk, struct so_timestamping timestamping; int ret; + if (has_current_bpf_ctx()) + return -EOPNOTSUPP; + if (optlen == sizeof(timestamping)) { if (copy_from_sockptr(&timestamping, optval, sizeof(timestamping))) @@ -XXX,XX +XXX,XX @@ static int mptcp_setsockopt_sol_socket_linger(struct mptcp_sock *msk, sockptr_t sockptr_t kopt; int ret; + if (has_current_bpf_ctx()) + return -EOPNOTSUPP; + if (optlen < sizeof(ling)) return -EINVAL; @@ -XXX,XX +XXX,XX @@ static int mptcp_setsockopt_sol_tcp_congestion(struct mptcp_sock *msk, sockptr_t bool cap_net_admin; int ret; + if (has_current_bpf_ctx()) + return -EOPNOTSUPP; + if (optlen < 1) return -EINVAL; @@ -XXX,XX +XXX,XX @@ static int __mptcp_setsockopt_set_val(struct mptcp_sock *msk, struct mptcp_subflow_context *subflow; int err = 0; + if (has_current_bpf_ctx()) + return -EOPNOTSUPP; + mptcp_for_each_subflow(msk, subflow) { struct sock *ssk = mptcp_subflow_tcp_sock(subflow); int ret; @@ -XXX,XX +XXX,XX @@ static int __mptcp_setsockopt_sol_tcp_cork(struct mptcp_sock *msk, int val) struct mptcp_subflow_context *subflow; struct sock *sk = (struct sock *)msk; + if (has_current_bpf_ctx()) + return -EOPNOTSUPP; + sockopt_seq_inc(msk); msk->cork = !!val; mptcp_for_each_subflow(msk, subflow) { @@ -XXX,XX +XXX,XX @@ static int __mptcp_setsockopt_sol_tcp_nodelay(struct mptcp_sock *msk, int val) struct mptcp_subflow_context *subflow; struct sock *sk = (struct sock *)msk; + if (has_current_bpf_ctx()) + return -EOPNOTSUPP; + sockopt_seq_inc(msk); msk->nodelay = !!val; mptcp_for_each_subflow(msk, subflow) { @@ -XXX,XX +XXX,XX @@ static int mptcp_setsockopt_v4_set_tos(struct mptcp_sock *msk, int optname, struct sock *sk = (struct sock *)msk; int err, val; + if (has_current_bpf_ctx()) + return -EOPNOTSUPP; + err = ip_setsockopt(sk, SOL_IP, optname, optval, optlen); if (err != 0) @@ -XXX,XX +XXX,XX @@ int mptcp_set_rcvlowat(struct sock *sk, int val) if (sk->sk_protocol == IPPROTO_TCP) return -EINVAL; + if (has_current_bpf_ctx()) + return -EOPNOTSUPP; + if (sk->sk_userlocks & SOCK_RCVBUF_LOCK) cap = sk->sk_rcvbuf >> 1; else -- 2.43.0 From: Gang Yan <yangang@kylinos.cn> bpf_setsockopt() currently cannot be used on mptcp master sockets: __bpf_setsockopt() dispatches by level to the protocol-agnostic sol_*_sockopt() helpers, which either reject the msk (sk_protocol == IPPROTO_MPTCP) and the ssk (sk_is_tcp() is false) or bypass mptcp's own dispatch (e.g. SOL_IP going straight to do_ip_setsockopt()). This patch routes any level to mptcp_setsockopt(), which already handles all levels. Signed-off-by: Gang Yan <yangang@kylinos.cn> --- include/net/mptcp.h | 9 +++++++++ net/core/filter.c | 7 +++++++ 2 files changed, 16 insertions(+) diff --git a/include/net/mptcp.h b/include/net/mptcp.h index XXXXXXX..XXXXXXX 100644 --- a/include/net/mptcp.h +++ b/include/net/mptcp.h @@ -XXX,XX +XXX,XX @@ static inline __be32 mptcp_reset_option(const struct sk_buff *skb) } void mptcp_active_detect_blackhole(struct sock *sk, bool expired); + +int mptcp_setsockopt(struct sock *sk, int level, int optname, + sockptr_t optval, unsigned int optlen); #else static inline void mptcp_init(void) @@ -XXX,XX +XXX,XX @@ static inline struct request_sock *mptcp_subflow_reqsk_alloc(const struct reques static inline __be32 mptcp_reset_option(const struct sk_buff *skb) { return htonl(0u); } static inline void mptcp_active_detect_blackhole(struct sock *sk, bool expired) { } + +static inline int mptcp_setsockopt(struct sock *sk, int level, int optname, + sockptr_t optval, unsigned int optlen) +{ + return -EINVAL; +} #endif /* CONFIG_MPTCP */ #if IS_ENABLED(CONFIG_MPTCP_IPV6) diff --git a/net/core/filter.c b/net/core/filter.c index XXXXXXX..XXXXXXX 100644 --- a/net/core/filter.c +++ b/net/core/filter.c @@ -XXX,XX +XXX,XX @@ static int __bpf_setsockopt(struct sock *sk, int level, int optname, if (!sk_fullsock(sk)) return -EINVAL; + /* Route any bpf_setsockopt on the mptcp socket to mptcp_setsockopt, + * which handles all levels. + */ + if (IS_ENABLED(CONFIG_MPTCP) && sk->sk_protocol == IPPROTO_MPTCP) + return mptcp_setsockopt(sk, level, optname, + KERNEL_SOCKPTR(optval), optlen); + if (level == SOL_SOCKET) return sol_socket_sockopt(sk, optname, optval, &optlen, false); else if (IS_ENABLED(CONFIG_INET) && level == SOL_IP) -- 2.43.0 From: Gang Yan <yangang@kylinos.cn> This patch adds a helper named 'mptcp_call_bpf' like tcp_call_bpf. Invoke the new helper from mptcp_connect() with BPF_SOCK_OPS_TCP_CONNECT_CB, placed after the subflow lock is acquired and before tcp_connect(). At this point the msk lock is held by __inet_stream_connect(), mirroring the placement of TCP_CONNECT_CB in tcp_v4_connect()/tcp_v6_connect(). 'bpf_sock_ops_cb_flags_set' can be called via msk, so using sk_is_tcp() to avoid this issue. Signed-off-by: Gang Yan <yangang@kylinos.cn> --- net/core/filter.c | 3 +++ net/mptcp/protocol.c | 6 ++++++ net/mptcp/protocol.h | 29 +++++++++++++++++++++++++++++ 3 files changed, 38 insertions(+) diff --git a/net/core/filter.c b/net/core/filter.c index XXXXXXX..XXXXXXX 100644 --- a/net/core/filter.c +++ b/net/core/filter.c @@ -XXX,XX +XXX,XX @@ BPF_CALL_2(bpf_sock_ops_cb_flags_set, struct bpf_sock_ops_kern *, bpf_sock, if (!IS_ENABLED(CONFIG_INET) || !sk_fullsock(sk)) return -EINVAL; + if (!sk_is_tcp(sk)) + return -EOPNOTSUPP; + tcp_sk(sk)->bpf_sock_ops_cb_flags = val; return argval & (~BPF_SOCK_OPS_ALL_CB_FLAGS); diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c index XXXXXXX..XXXXXXX 100644 --- a/net/mptcp/protocol.c +++ b/net/mptcp/protocol.c @@ -XXX,XX +XXX,XX @@ static int mptcp_connect(struct sock *sk, struct sockaddr_unsized *uaddr, if (!msk->fastopening) lock_sock(ssk); + /* Notify cgroup BPF on the msk before initiating the subflow connect. + * Mirrors BPF_SOCK_OPS_TCP_CONNECT_CB; msk lock is held by the + * caller (__inet_stream_connect) and ssk is held before. + */ + mptcp_call_bpf(sk, BPF_SOCK_OPS_TCP_CONNECT_CB, 0, NULL); + /* the following mirrors closely a very small chunk of code from * __inet_stream_connect() */ diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h index XXXXXXX..XXXXXXX 100644 --- a/net/mptcp/protocol.h +++ b/net/mptcp/protocol.h @@ -XXX,XX +XXX,XX @@ mptcp_token_join_cookie_init_state(struct mptcp_subflow_request_sock *subflow_re static inline void mptcp_join_cookie_init(void) {} #endif +#ifdef CONFIG_BPF_JIT +static inline int mptcp_call_bpf(struct sock *sk, int op, u32 nargs, u32 *args) +{ + struct bpf_sock_ops_kern sock_ops; + int ret; + + memset(&sock_ops, 0, offsetof(struct bpf_sock_ops_kern, temp)); + + if (sk_fullsock(sk)) { + sock_ops.is_fullsock = 1; + sock_owned_by_me(sk); + } + + sock_ops.sk = sk; + sock_ops.op = op; + + if (nargs > 0) + memcpy(sock_ops.args, args, nargs * sizeof(*args)); + + ret = BPF_CGROUP_RUN_PROG_SOCK_OPS(&sock_ops); + return ret == 0 ? sock_ops.reply : -1; +} +#else +static inline int mptcp_call_bpf(struct sock *sk, int op, u32 nargs, u32 *args) +{ + return -1; +} +#endif + #endif /* __MPTCP_PROTOCOL_H */ -- 2.43.0 From: Gang Yan <yangang@kylinos.cn> Add a BPF sockops program attached to BPF_CGROUP_SOCK_OPS that fires on BPF_SOCK_OPS_TCP_CONNECT_CB and exercises bpf_setsockopt() on the mptcp master socket (msk). Two scenarios are covered by the new "setsockopt" subtest: - TCP_INQ must succeed and the value (1) must be observable from userspace via getsockopt() on the mptcp socket. - TCP_CONGESTION needs the subflow lock and is therefore rejected in bpf context with -EOPNOTSUPP. Signed-off-by: Gang Yan <yangang@kylinos.cn> --- .../testing/selftests/bpf/prog_tests/mptcp.c | 61 +++++++++++++++++++ .../selftests/bpf/progs/mptcp_setsockopt.c | 32 ++++++++++ 2 files changed, 93 insertions(+) create mode 100644 tools/testing/selftests/bpf/progs/mptcp_setsockopt.c diff --git a/tools/testing/selftests/bpf/prog_tests/mptcp.c b/tools/testing/selftests/bpf/prog_tests/mptcp.c index XXXXXXX..XXXXXXX 100644 --- a/tools/testing/selftests/bpf/prog_tests/mptcp.c +++ b/tools/testing/selftests/bpf/prog_tests/mptcp.c @@ -XXX,XX +XXX,XX @@ #include "mptcp_bpf_rr.skel.h" #include "mptcp_bpf_red.skel.h" #include "mptcp_bpf_burst.skel.h" +#include "mptcp_setsockopt.skel.h" #define NS_TEST "mptcp_ns" #define ADDR_1 "10.0.1.1" @@ -XXX,XX +XXX,XX @@ static void test_burst(void) mptcp_bpf_burst__destroy(skel); } +static void test_setsockopt(void) +{ + struct mptcp_setsockopt *skel; + struct netns_obj *netns; + int cgroup_fd, server_fd, client_fd; + int err; + int inq; + socklen_t len; + + cgroup_fd = test__join_cgroup("/mptcp_setsockopt"); + if (!ASSERT_OK_FD(cgroup_fd, "join_cgroup")) + return; + + skel = mptcp_setsockopt__open_and_load(); + if (!ASSERT_OK_PTR(skel, "skel_open_load")) + goto close_cgroup; + + skel->links.mptcp_connect_cb = + bpf_program__attach_cgroup(skel->progs.mptcp_connect_cb, + cgroup_fd); + if (!ASSERT_OK_PTR(skel->links.mptcp_connect_cb, "attach connect_cb")) + goto skel_destroy; + + netns = netns_new(NS_TEST, true); + if (!ASSERT_OK_PTR(netns, "netns_new")) + goto skel_destroy; + + server_fd = start_mptcp_server(AF_INET, NULL, 0, 0); + if (!ASSERT_OK_FD(server_fd, "start_mptcp_server")) + goto close_netns; + + client_fd = connect_to_fd(server_fd, 0); + if (!ASSERT_OK_FD(client_fd, "connect_to_fd")) + goto close_server; + + /* TCP_INQ should be set successfullly */ + ASSERT_EQ(skel->bss->connect_cb_inq_ret, 0, "connect_cb TCP_INQ ret"); + + len = sizeof(inq); + err = getsockopt(client_fd, SOL_TCP, TCP_INQ, &inq, &len); + if (ASSERT_OK(err, "getsockopt TCP_INQ")) + ASSERT_EQ(inq, 1, "TCP_INQ value"); + + /* TCP_CONGESTION should be -EOPNOTSUPP */ + ASSERT_EQ(skel->bss->connect_cb_cc_ret, -EOPNOTSUPP, + "connect_cb TCP_CONGESTION ret"); + + close(client_fd); +close_server: + close(server_fd); +close_netns: + netns_free(netns); +skel_destroy: + mptcp_setsockopt__destroy(skel); +close_cgroup: + close(cgroup_fd); +} + void test_mptcp(void) { if (test__start_subtest("base")) @@ -XXX,XX +XXX,XX @@ void test_mptcp(void) test_red(); if (test__start_subtest("burst")) test_burst(); + if (test__start_subtest("setsockopt")) + test_setsockopt(); } diff --git a/tools/testing/selftests/bpf/progs/mptcp_setsockopt.c b/tools/testing/selftests/bpf/progs/mptcp_setsockopt.c new file mode 100644 index XXXXXXX..XXXXXXX --- /dev/null +++ b/tools/testing/selftests/bpf/progs/mptcp_setsockopt.c @@ -XXX,XX +XXX,XX @@ +#include "bpf_tracing_net.h" +#include "mptcp_bpf.h" + +#ifndef TCP_INQ +#define TCP_INQ 36 +#endif + +int connect_cb_inq_ret; +int connect_cb_cc_ret; + +char cc_reno[TCP_CA_NAME_MAX] = "reno"; + +SEC("sockops") +int mptcp_connect_cb(struct bpf_sock_ops *skops) +{ + struct bpf_sock *sk = skops->sk; + int one = 1; + + if (skops->op != BPF_SOCK_OPS_TCP_CONNECT_CB) + return 1; + + if (!sk || sk->protocol != IPPROTO_MPTCP) + return 1; + + connect_cb_inq_ret = + bpf_setsockopt(skops, SOL_TCP, TCP_INQ, &one, sizeof(one)); + connect_cb_cc_ret = + bpf_setsockopt(skops, SOL_TCP, TCP_CONGESTION, + cc_reno, sizeof(cc_reno)); + + return 1; +} -- 2.43.0