Hi Antoine,
On Wed, Apr 10, 2024 at 8:14 PM Antoine Tenart <[email protected]> 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);
>
> I get that 'int' is used instead of 'enum sk_rst_reason' to allow
> passing drop reasons too without casting, but that makes understanding
Yes!
> 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.
>
> 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
It, indeed, looks better (clean and clear) :)
So I also ought to adjust the trace_tcp_send_reset part...
Thanks,
Jason
