| Message ID | 20220727060915.2372520-1-kafai@fb.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf: net: Remove duplicated codes from bpf_setsockopt() | expand |
| Context | Check | Description |
|---|---|---|
| bpf/vmtest-bpf-next-PR | fail | PR summary |
| netdev/tree_selection | success | Clearly marked for bpf-next, async |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Series has a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 6 this patch: 6 |
| netdev/cc_maintainers | success | CCed 6 of 6 maintainers |
| netdev/build_clang | success | Errors and warnings before: 5 this patch: 5 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 6 this patch: 6 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 57 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
| bpf/vmtest-bpf-next-VM_Test-1 | success | Logs for Kernel LATEST on ubuntu-latest with gcc |
| bpf/vmtest-bpf-next-VM_Test-2 | success | Logs for Kernel LATEST on ubuntu-latest with llvm-15 |
| bpf/vmtest-bpf-next-VM_Test-3 | fail | Logs for Kernel LATEST on z15 with gcc |
On 07/26, Martin KaFai Lau wrote: > When bpf program calling bpf_setsockopt(SOL_SOCKET), > it could be run in softirq and doesn't make sense to do the capable > check. There was a similar situation in bpf_setsockopt(TCP_CONGESTION). Should we instead skip these capability checks based on something like in_serving_softirq? I wonder if we might be mixing too much into that is_bpf flag (locking assumptions, context assumptions, etc)?
On Wed, Jul 27, 2022 at 09:54:08AM -0700, sdf@google.com wrote: > On 07/26, Martin KaFai Lau wrote: > > When bpf program calling bpf_setsockopt(SOL_SOCKET), > > it could be run in softirq and doesn't make sense to do the capable > > check. There was a similar situation in bpf_setsockopt(TCP_CONGESTION). > > Should we instead skip these capability checks based on something like > in_serving_softirq? I wonder if we might be mixing too much into that > is_bpf flag (locking assumptions, context assumptions, etc)? Yes, the bit can be splitted as another reply in patch 2. I don't think in_serving_softirq is a good fit name. Some of the hooks is not in_serving_softirq. is_bpf should be a better name for this.
diff --git a/net/core/sock.c b/net/core/sock.c index 61d927a5f6cb..f2c582491d5f 100644 --- a/net/core/sock.c +++ b/net/core/sock.c @@ -620,7 +620,7 @@ struct dst_entry *sk_dst_check(struct sock *sk, u32 cookie) } EXPORT_SYMBOL(sk_dst_check); -static int sock_bindtoindex_locked(struct sock *sk, int ifindex) +static int sock_bindtoindex_locked(struct sock *sk, int ifindex, bool cap_check) { int ret = -ENOPROTOOPT; #ifdef CONFIG_NETDEVICES @@ -628,7 +628,8 @@ static int sock_bindtoindex_locked(struct sock *sk, int ifindex) /* Sorry... */ ret = -EPERM; - if (sk->sk_bound_dev_if && !ns_capable(net->user_ns, CAP_NET_RAW)) + if (sk->sk_bound_dev_if && cap_check && + !ns_capable(net->user_ns, CAP_NET_RAW)) goto out; ret = -EINVAL; @@ -656,7 +657,7 @@ int sock_bindtoindex(struct sock *sk, int ifindex, bool lock_sk) if (lock_sk) lock_sock(sk); - ret = sock_bindtoindex_locked(sk, ifindex); + ret = sock_bindtoindex_locked(sk, ifindex, true); if (lock_sk) release_sock(sk); @@ -704,7 +705,7 @@ static int sock_setbindtodevice(struct sock *sk, sockptr_t optval, int optlen) } lock_sock_sockopt(sk, optval); - ret = sock_bindtoindex_locked(sk, index); + ret = sock_bindtoindex_locked(sk, index, !optval.is_bpf); release_sock_sockopt(sk, optval); out: #endif @@ -1166,6 +1167,7 @@ int sock_setsockopt(struct sock *sk, int level, int optname, case SO_PRIORITY: if ((val >= 0 && val <= 6) || + optval.is_bpf || ns_capable(sock_net(sk)->user_ns, CAP_NET_RAW) || ns_capable(sock_net(sk)->user_ns, CAP_NET_ADMIN)) sk->sk_priority = val; @@ -1312,7 +1314,8 @@ int sock_setsockopt(struct sock *sk, int level, int optname, clear_bit(SOCK_PASSSEC, &sock->flags); break; case SO_MARK: - if (!ns_capable(sock_net(sk)->user_ns, CAP_NET_RAW) && + if (!optval.is_bpf && + !ns_capable(sock_net(sk)->user_ns, CAP_NET_RAW) && !ns_capable(sock_net(sk)->user_ns, CAP_NET_ADMIN)) { ret = -EPERM; break; @@ -1456,7 +1459,7 @@ int sock_setsockopt(struct sock *sk, int level, int optname, break; case SO_BINDTOIFINDEX: - ret = sock_bindtoindex_locked(sk, val); + ret = sock_bindtoindex_locked(sk, val, !optval.is_bpf); break; case SO_BUF_LOCK:
When bpf program calling bpf_setsockopt(SOL_SOCKET), it could be run in softirq and doesn't make sense to do the capable check. There was a similar situation in bpf_setsockopt(TCP_CONGESTION). In commit 8d650cdedaab ("tcp: fix tcp_set_congestion_control() use from bpf hook") tcp_set_congestion_control(..., cap_net_admin) was added to skip the cap check for bpf prog. A similar change is done in this patch for SO_MARK, SO_PRIORITY, and SO_BINDTO{DEVICE,IFINDEX} which are the optnames allowed by bpf_setsockopt(SOL_SOCKET). This will allow the sock_setsockopt() to be reused by bpf_setsockopt(SOL_SOCKET) in a latter patch. Signed-off-by: Martin KaFai Lau <kafai@fb.com> --- net/core/sock.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-)