Re: [PATCH bpf 1/2] bpf: Fix out-of-bounds read of sk_protocol in bpf_sock_destroy()

[Jiayuan Chen]

on 9/3/26 10:01 PM, bot+bpf...@kernel.org wrote:
diff --git a/net/core/filter.c b/net/core/filter.c
index 61940e7535523..1bbb72138ac64 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -12912,8 +12912,8 @@ __bpf_kfunc_start_defs();
   * @sock: Pointer to socket to be destroyed
   *
   * Return:
- * On error, may return EPROTONOSUPPORT, EINVAL.
- * EPROTONOSUPPORT if protocol specific destroy handler is not supported.
+ * On error, may return EOPNOTSUPP, EINVAL.
+ * EOPNOTSUPP if protocol specific destroy handler is not supported.
   * 0 otherwise
The EPROTONOSUPPORT -> EOPNOTSUPP correction is right, but the rewritten
error list is incomplete. tcp_abort() returns -ENOENT when the socket is
already closed (net/ipv4/tcp.c, around the sk_state == TCP_CLOSE check):

     /* Avoid closing the same socket twice. */
     if (sk->sk_state == TCP_CLOSE) {
         if (!has_current_bpf_ctx())
             release_sock(sk);
         return -ENOENT;
     }

That error propagates through sk->sk_prot->diag_destroy(sk, ECONNABORTED)
and is reachable from a BPF iterator (destroying a sock the iterator
already destroyed, or one closed concurrently). In contrast, EINVAL is only
reachable through the narrower tcp_abort_override() path for MPTCP listener
subflows.

Should ENOENT be added to the list?


I'd like to describe it as "others" since diag_destroy may change it's 
return value.