On Thu, Jan 16, 2025 at 5:48 AM Martin KaFai Lau <martin.lau@xxxxxxxxx> wrote:
>
> On 1/12/25 3:37 AM, Jason Xing wrote:
> > In the next round, we will support the UDP proto for SO_TIMESTAMPING
> > bpf extension, so we need to ensure there is no safety problem.
> >
> > Signed-off-by: Jason Xing <kerneljasonxing@xxxxxxxxx>
> > ---
> > net/core/filter.c | 9 +++++++--
> > 1 file changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/net/core/filter.c b/net/core/filter.c
> > index 0e915268db5f..517f09aabc92 100644
> > --- a/net/core/filter.c
> > +++ b/net/core/filter.c
> > @@ -5571,7 +5571,7 @@ static int __bpf_getsockopt(struct sock *sk, int level, int optname,
> > static int _bpf_getsockopt(struct sock *sk, int level, int optname,
> > char *optval, int optlen)
> > {
> > - if (sk_fullsock(sk))
> > + if (sk_fullsock(sk) && optname != SK_BPF_CB_FLAGS)
> > sock_owned_by_me(sk);
> > return __bpf_getsockopt(sk, level, optname, optval, optlen);
> > }
> > @@ -5776,6 +5776,7 @@ BPF_CALL_5(bpf_sock_ops_getsockopt, struct bpf_sock_ops_kern *, bpf_sock,
> > int, level, int, optname, char *, optval, int, optlen)
> > {
> > if (IS_ENABLED(CONFIG_INET) && level == SOL_TCP &&
> > + bpf_sock->sk->sk_protocol == IPPROTO_TCP &&
> > optname >= TCP_BPF_SYN && optname <= TCP_BPF_SYN_MAC) {
> > int ret, copy_len = 0;
> > const u8 *start;
> > @@ -5817,7 +5818,8 @@ BPF_CALL_2(bpf_sock_ops_cb_flags_set, struct bpf_sock_ops_kern *, bpf_sock,
> > struct sock *sk = bpf_sock->sk;
> > int val = argval & BPF_SOCK_OPS_ALL_CB_FLAGS;
> >
> > - if (!IS_ENABLED(CONFIG_INET) || !sk_fullsock(sk))
> > + if (!IS_ENABLED(CONFIG_INET) || !sk_fullsock(sk) ||
> > + sk->sk_protocol != IPPROTO_TCP)
> > return -EINVAL;
> >
> > tcp_sk(sk)->bpf_sock_ops_cb_flags = val;
> > @@ -7626,6 +7628,9 @@ BPF_CALL_4(bpf_sock_ops_load_hdr_opt, struct bpf_sock_ops_kern *, bpf_sock,
> > u8 search_kind, search_len, copy_len, magic_len;
> > int ret;
> >
> > + if (bpf_sock->op != SK_BPF_CB_FLAGS)
>
> SK_BPF_CB_FLAGS is not an op enum, so the check is incorrect. It does break the
> existing test.
>
> ./test_progs -t tcp_hdr_options
> WARNING! Selftests relying on bpf_testmod.ko will be skipped.
> #402/1 tcp_hdr_options/simple_estab:FAIL
> #402/2 tcp_hdr_options/no_exprm_estab:FAIL
> #402/3 tcp_hdr_options/syncookie_estab:FAIL
> #402/4 tcp_hdr_options/fastopen_estab:FAIL
> #402/5 tcp_hdr_options/fin:FAIL
> #402/6 tcp_hdr_options/misc:FAIL
> #402 tcp_hdr_options:FAIL
> #402/1 tcp_hdr_options/simple_estab:FAIL
> #402/2 tcp_hdr_options/no_exprm_estab:FAIL
> #402/3 tcp_hdr_options/syncookie_estab:FAIL
> #402/4 tcp_hdr_options/fastopen_estab:FAIL
> #402/5 tcp_hdr_options/fin:FAIL
> #402/6 tcp_hdr_options/misc:FAIL
> #402 tcp_hdr_options:FAIL
>
Right, kernel test robot also discovered this one.
>
> Many changes of this set is in bpf and the newly added selftest is also a bpf
> prog, all bpf selftests should be run before posting.
> (Documentation/bpf/bpf_devel_QA.rst)
>
> The bpf CI can automatically pick it up and get an auto email on breakage like
> this if the set is tagged to bpf-next. We can figure out where to land the set
> later (bpf-next/net or net-next/main) when it is ready.
>
> All these changes also need a test in selftests/bpf. For example, I expect there
> is a test to ensure calling these bpf helpers from the new tstamp callback will
> get a negative errno value.
>
> For patch 4 and patch 5, I would suggest keeping it simple to only check for
> bpf_sock->op for the helpers that make tcp_sock and/or locked sk assumption.
> Something like this on top of your patch. Untested:
>
> diff --git i/net/core/filter.c w/net/core/filter.c
> index 517f09aabc92..ccb13b61c528 100644
> --- i/net/core/filter.c
> +++ w/net/core/filter.c
> @@ -7620,6 +7620,11 @@ static const u8 *bpf_search_tcp_opt(const u8 *op, const
> u8 *opend,
> return ERR_PTR(-ENOMSG);
> }
>
> +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)
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.
> +}
> +
> BPF_CALL_4(bpf_sock_ops_load_hdr_opt, struct bpf_sock_ops_kern *, bpf_sock,
> void *, search_res, u32, len, u64, flags)
> {
> @@ -7628,8 +7633,8 @@ BPF_CALL_4(bpf_sock_ops_load_hdr_opt, struct
> bpf_sock_ops_kern *, bpf_sock,
> u8 search_kind, search_len, copy_len, magic_len;
> int ret;
>
> - if (bpf_sock->op != SK_BPF_CB_FLAGS)
> - return -EINVAL;
> + if (!is_locked_tcp_sock_ops(bpf_sock))
> + return -EOPNOTSUPP;
Right, thanks, I will do that.
>
> /* 2 byte is the minimal option len except TCPOPT_NOP and
> * TCPOPT_EOL which are useless for the bpf prog to learn
>
>
> > + return -EINVAL;
> > +
> > /* 2 byte is the minimal option len except TCPOPT_NOP and
> > * TCPOPT_EOL which are useless for the bpf prog to learn
> > * and this helper disallow loading them also.
>
