Re: [PATCH bpf] bpf: Disallow bpf_{g,s}etsockopt() in cgroup UNIX getname hooks

> diff --git a/net/core/filter.c b/net/core/filter.c
> index 16845987b244..1e80a52ef86d 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -8350,10 +8350,8 @@ sock_addr_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
>  		case BPF_CGROUP_UNIX_SENDMSG:
>  		case BPF_CGROUP_INET4_GETPEERNAME:
>  		case BPF_CGROUP_INET6_GETPEERNAME:
> -		case BPF_CGROUP_UNIX_GETPEERNAME:
>  		case BPF_CGROUP_INET4_GETSOCKNAME:
>  		case BPF_CGROUP_INET6_GETSOCKNAME:
> -		case BPF_CGROUP_UNIX_GETSOCKNAME:
>  			return &bpf_sock_addr_setsockopt_proto;
>  		default:
>  			return NULL;
> @@ -8373,10 +8371,8 @@ sock_addr_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
>  		case BPF_CGROUP_UNIX_SENDMSG:
>  		case BPF_CGROUP_INET4_GETPEERNAME:
>  		case BPF_CGROUP_INET6_GETPEERNAME:
> -		case BPF_CGROUP_UNIX_GETPEERNAME:
>  		case BPF_CGROUP_INET4_GETSOCKNAME:
>  		case BPF_CGROUP_INET6_GETSOCKNAME:
> -		case BPF_CGROUP_UNIX_GETSOCKNAME:
>  			return &bpf_sock_addr_getsockopt_proto;

Is this removal a deliberate ABI restriction? Programs that previously
loaded with expected_attach_type BPF_CGROUP_UNIX_GETPEERNAME or
BPF_CGROUP_UNIX_GETSOCKNAME and called bpf_setsockopt() or
bpf_getsockopt() now fail BPF_PROG_LOAD with 'unknown func
bpf_setsockopt#...'.

For BPF_PROG_TYPE_CGROUP_SOCK_ADDR, bpf_prog_attach_check_attach_type()
requires an exact match:

  case BPF_PROG_TYPE_CGROUP_SOCK_ADDR:
  ...
	return attach_type == prog->expected_attach_type ? 0 : -EINVAL;

in kernel/bpf/syscall.c (bpf_link creation enforces the same check).
Gating on prog->expected_attach_type at verification time cannot be
worked around, so any working deployment can stop loading its programs
across a stable point release if this patch lands in stable kernels.

An alternative fix exists that removes the bug without removing
functionality: run the hook under BPF_CGROUP_RUN_SA_PROG_LOCK (as used
in unix_connect, unix_sendmsg, unix_recvmsg in net/unix/af_unix.c),
which wraps the run in lock_sock()/release_sock(). inet_getname() and
inet6_getname() take lock_sock() themselves, which is why the INET
GETPEERNAME/GETSOCKNAME labels can safely stay in the lists. Only
unix_getname() runs the hook without acquiring the socket lock (it uses
smp_load_acquire(&unix_sk(sk)->addr) instead).

The design intent points the other way: beecf11bc218 ("bpf:
Bpf_{g,s}etsockopt for struct bpf_sock_addr") introduced these helpers
with the explicit assumption that 'context sk is already locked and
socket lock is non-nestable', and cgroup_common_func_proto() in
kernel/bpf/cgroup.c already returns NULL for bpf_get_retval/
bpf_set_retval on these same getname attach types, so per-attach-type
helper gating has precedent.  No in-tree selftest breaks.

Should the changelog state that the removal is deliberate and that no
users are expected, making the ABI trade-off explicit for the maintainer
and for stable? Or explain why locking unix_getname()'s hook was
rejected as the fix?


>     _bpf_setsockopt() and _bpf_getsockopt() call sock_owned_by_me() for
>     full sockets, so these helpers expect the socket lock to be held.
>
>     BPF_CGROUP_UNIX_GETPEERNAME and BPF_CGROUP_UNIX_GETSOCKNAME run BPF
>     programs without acquiring the socket lock. A program attached to
>     either hook can therefore trigger the sock_owned_by_me() warning by
>     calling bpf_setsockopt() or bpf_getsockopt().

Is describing the impact as triggering 'the sock_owned_by_me() warning'
accurate?

sock_owned_by_me() is a no-op unless lockdep is built in:

  static inline void sock_owned_by_me(const struct sock *sk)
  {
  #ifdef CONFIG_LOCKDEP
	  WARN_ON_ONCE(!lockdep_sock_is_held(sk) && debug_locks);
  #endif
  }

in include/net/sock.h. On a production kernel without CONFIG_LOCKDEP
nothing is printed at all, so a reader may conclude the issue is
debug-only and does not warrant a Fixes: tag or a stable backport.

The actual consequence is that _bpf_setsockopt() proceeds
unconditionally after the assertion:

  if (sk_fullsock(sk))
	  sock_owned_by_me(sk);
  return __bpf_setsockopt(sk, level, optname, optval, optlen);

in net/core/filter.c (the same shape appears in _bpf_getsockopt()).
AF_UNIX sockets are always full sockets (sk_fullsock() is true), and
unix_getname() runs the hook with no socket lock at all. So a BPF
program on these hooks mutates socket state through sk_setsockopt() with
no serialization against concurrent socket operations.


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/31583057641