Re: [PATCH net-next 2/6] net: export sock_disable_timestamp() declaration

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Calling `bpf_setsockopt` to enable or disable `SK_BPF_CB_RX_TIMESTAMPING` from BPF hooks executing in softirq will trigger a kernel panic (sleep in atomic context) if `CONFIG_JUMP_LABEL` is not enabled.
- [Low] API asymmetry between `sock_enable_timestamp` and `sock_disable_timestamp` creates a dangerous bitmask trap.
--

commit ae551b79a5c9f8c77421dc360627b1e40e3ceaec
Author: Jason Xing <kerneljasonxing@gmail.com>
Subject: net: export sock_disable_timestamp() declaration

This patch removes the static modifier from sock_disable_timestamp() and adds
its declaration to include/net/sock.h. This change exposes the function to the
rest of the kernel, specifically allowing the BPF subsystem to dynamically turn
off the global time record function.

> diff --git a/include/net/sock.h b/include/net/sock.h
> index dccd3738c3687..8a49312ee0976 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
> @@ -3038,6 +3038,7 @@ static inline bool sk_listener_or_tw(const struct sock *sk)
>  }
>  
>  void sock_enable_timestamp(struct sock *sk, enum sock_flags flag);
> +void sock_disable_timestamp(struct sock *sk, unsigned long flags);

This isn't a bug, but does exporting these two functions with asymmetrical
signatures create a bitmask trap?

The newly exported sock_disable_timestamp() expects a bitmask (unsigned long
flags), while sock_enable_timestamp() expects an enum index (enum sock_flags
flag).

If a future developer mistakenly passes an enum index (like SOCK_TIMESTAMP
which has value 7) to sock_disable_timestamp(), wouldn't it silently clear
multiple unrelated socket flags (bits 0, 1, and 2) instead of the intended
flag?

>  int sock_recv_errqueue(struct sock *sk, struct msghdr *msg, int len, int level,
>  		       int type);

[ ... ]

> diff --git a/net/core/sock.c b/net/core/sock.c
> index b37b664b6eb92..2d552ae5edd42 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -474,7 +474,7 @@ static bool sock_needs_netstamp(const struct sock *sk)
>  	}
>  }
>  
> -static void sock_disable_timestamp(struct sock *sk, unsigned long flags)
> +void sock_disable_timestamp(struct sock *sk, unsigned long flags)
>  {
>  	if (sk->sk_flags & flags) {
>  		sk->sk_flags &= ~flags;

If BPF programs running in an atomic softirq context call bpf_setsockopt() to
disable SK_BPF_CB_RX_TIMESTAMPING, could this trigger a panic?

Looking at net_disable_timestamp() which is called immediately after this
bitmask update, if CONFIG_JUMP_LABEL is not enabled, the code falls back to
calling static_branch_dec() on netstamp_needed_key.

This expands to static_key_slow_dec(), which unconditionally acquires
jump_label_mutex and cpus_read_lock().

Since acquiring a sleeping mutex inside a softirq context triggers a
sleep-in-atomic panic, will this cause a deterministic crash when users
dynamically attach these BPF programs?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260518082344.96647-1-kerneljasonxing@gmail.com?part=2