> On Mar 30, 2023, at 10:30 AM, Martin KaFai Lau <martin.lau@xxxxxxxxx> wrote: > > On 3/30/23 9:32 AM, Stanislav Fomichev wrote: >>>> Maybe make it more opt-in? (vs current "opt ipproto_raw out") >>>> >>>> if (sk->sk_prot->diag_destroy != udp_abort && >>>> sk->sk_prot->diag_destroy != tcp_abort) >>>> return -EOPNOTSUPP; >>>> >>>> Is it more robust? Or does it look uglier? ) >>>> But maybe fine as is, I'm just thinking out loud.. >>> >>> Do we expect the handler to be extended for more types? Probably not... So I'll leave it as is. >> My worry is about somebody adding .diag_destroy to some new/old >> protocol in the future, say sctp_prot, without being aware >> of this bpf_sock_destroy helper and its locking requirements. Ah, sctp!! > > Other helpers in filter.c is also opt-in. I think it is better to do the same here. IPPROTO_TCP and IPPROTO_UDP should have very good use case coverage to begin with. It can also help to ensure new selftests are written to cover the protocol supporting bpf_sock_destroy in the future. > > I like the comment in bpf_sock_destroy() in this patch. It will be even better if it can spell out more clearly that future supporting protocol needs to assume the lock_sock has already been done on the bpf side. Ack... I'll make it opt-in. > >> So having an opt-in here (as in sk_protocol == IPPROTO_TCP || >> sk_protocol == IPPROTO_UDP) feels more future-proof than your current >> opt-out (sk_proto != IPPROTO_RAW). >> WDYT? >>>>> + release_sock(sk); >>>> >>>>> return 0; >>>>> } >>>>> -- >>>>> 2.34.1 >>> >
