On Wed, Apr 10, 2024 at 9:21 PM Antoine Tenart <aten...@kernel.org> wrote: > > Quoting Jason Xing (2024-04-10 14:54:51) > > Hi Antoine, > > > > On Wed, Apr 10, 2024 at 8:14 PM Antoine Tenart <aten...@kernel.org> wrote: > > > > > > Quoting Jason Xing (2024-04-09 12:09:30) > > > > void (*send_reset)(const struct sock *sk, > > > > - struct sk_buff *skb); > > > > + struct sk_buff *skb, > > > > + int reason); > > > > > what should be 'reason' harder. Eg. when looking at the code or when > > > using BTF (to then install debugging probes with BPF) this is not > > > obvious. > > > > Only one number if we want to extract the reason with BPF, right? I > > haven't tried it. > > Yes, we can get 'reason'. Knowing the type helps. > > > > A similar approach could be done as the one used for drop reasons: enum > > > skb_drop_reason is used for parameters (eg. kfree_skb_reason) but other > > > valid values (subsystem drop reasons) can be used too if casted (to > > > u32). We could use 'enum sk_rst_reason' and cast the other values. WDYT? > > > > I have been haunted by this 'issue' for a long time... > > > > Are you suggesting doing so as below for readability: > > 1) replace the reason parameter in all the related functions (like > > .send_reset(), tcp_v4_send_reset(), etc) by using 'enum sk_rst_reason' > > type? > > 2) in patch [4/6], when it needs to pass the specific reason in those > > functions, we can cast it to 'enum sk_rst_reason'? > > > > One modification I just made based on this patchset if I understand > > correctly: > > diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c > > index 4889fccbf754..e0419b8496b5 100644 > > --- a/net/ipv4/tcp_ipv4.c > > +++ b/net/ipv4/tcp_ipv4.c > > @@ -725,7 +725,7 @@ static bool tcp_v4_ao_sign_reset(const struct sock > > *sk, struct sk_buff *skb, > > */ > > > > static void tcp_v4_send_reset(const struct sock *sk, struct sk_buff *skb, > > - int reason) > > + enum sk_rst_reason reason) > > { > > const struct tcphdr *th = tcp_hdr(skb); > > struct { > > @@ -1935,7 +1935,7 @@ int tcp_v4_do_rcv(struct sock *sk, struct sk_buff > > *skb) > > return 0; > > > > reset: > > - tcp_v4_send_reset(rsk, skb, reason); > > + tcp_v4_send_reset(rsk, skb, (enum sk_rst_reason)reason); > > discard: > > kfree_skb_reason(skb, reason); > > /* Be careful here. If this function gets more complicated and > > > > That's right. I think (u32) can also be used for the cast to make the > compiler happy in 2), but the above makes sense.
Got it :) Will update soon. Thanks, Jason