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.
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.
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
