Re: [PATCH net-next v5 05/15] net-timestamp: add strict check in some BPF calls

[Martin KaFai Lau <martin.lau@linux.dev>]

On 1/15/25 3:32 PM, Jason Xing wrote:
>> +static bool is_locked_tcp_sock_ops(struct bpf_sock_ops_kern *bpf_sock)
>> +{
>> +       return bpf_sock->op <= BPF_SOCK_OPS_WRITE_HDR_OPT_CB;
> 
> I wonder if I can use the code snippets in the previous reply in this
> thread, only checking if we are in the timestamping callback?
> +#define BPF_SOCK_OPTS_TS               (BPF_SOCK_OPS_TS_SCHED_OPT_CB | \
> +                                        BPF_SOCK_OPS_TS_SW_OPT_CB | \
> +                                        BPF_SOCK_OPS_TS_ACK_OPT_CB | \
> +                                        BPF_SOCK_OPS_TS_TCP_SND_CB)

Note that BPF_SOCK_OPS_*_CB is not a bit.

My understanding is it is a blacklist. Please correct me if I miss-interpret the 
intention.

> 
> Then other developers won't worry too much whether they will cause
> some safety problems. If not, they will/must add callbacks earlier
> than BPF_SOCK_OPS_WRITE_HDR_OPT_CB.

It can't be added earlier because it is in uapi. If the future new cb is safe to 
use these helpers, then it needs to adjust the BPF_SOCK_OPS_WRITE_HDR_OPT_CB 
check. is_locked_tcp_sock_ops() is a whitelist. The worst is someone will 
discover the helpers are not usable in the new cb, so no safety issue.

If forgot to adjust the blacklist and the new cb should not use the helpers, 
then it is a safety issue.

Anyhow, I don't have a strong opinion here. I did think about checking the new 
TS callback instead. I went with the simplest way in the code and also 
considering the BPF_SOCK_OPS_TS_*_CB is only introduced starting from patch 7.