Quoting Jason Xing (2024-04-10 14:54:51)
> 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);
>
> > 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.
Thanks!
Antoine
