netdev - Re: [PATCH net-next v3 2/6] rstreason: prepare for passive reset

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <171275527215.4303.17205725451869291289@kwain>
Date: Wed, 10 Apr 2024 15:21:12 +0200
From: Antoine Tenart <atenart@...nel.org>
To: Jason Xing <kerneljasonxing@...il.com>
Cc: davem@...emloft.net, dsahern@...nel.org, edumazet@...gle.com, geliang@...nel.org, kuba@...nel.org, martineau@...nel.org, mathieu.desnoyers@...icios.com, matttbe@...nel.org, mhiramat@...nel.org, pabeni@...hat.com, rostedt@...dmis.org, mptcp@...ts.linux.dev, linux-trace-kernel@...r.kernel.org, netdev@...r.kernel.org, Jason Xing <kernelxing@...cent.com>
Subject: Re: [PATCH net-next v3 2/6] rstreason: prepare for passive reset

Quoting Jason Xing (2024-04-10 14:54:51)
> Hi Antoine,
> 
> On Wed, Apr 10, 2024 at 8:14 PM Antoine Tenart <atenart@...nel.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.

Thanks!
Antoine