Message-ID: <CAL+tcoA_eU98hMoWA2UM2LD_fNp=geY0uUfc+4pDnbUCKK6=Ew@mail.gmail.com>
Date: Fri, 19 Apr 2024 16:00:20 +0800
From: Jason Xing <kerneljasonxing@...il.com>
To: Eric Dumazet <edumazet@...gle.com>
Cc: Jakub Kicinski <kuba@...nel.org>, dsahern@...nel.org, matttbe@...nel.org, 
	martineau@...nel.org, geliang@...nel.org, pabeni@...hat.com, 
	davem@...emloft.net, rostedt@...dmis.org, mhiramat@...nel.org, 
	mathieu.desnoyers@...icios.com, atenart@...nel.org, mptcp@...ts.linux.dev, 
	netdev@...r.kernel.org, linux-trace-kernel@...r.kernel.org, 
	Jason Xing <kernelxing@...cent.com>
Subject: Re: [PATCH net-next v6 0/7] Implement reset reason mechanism to detect

On Fri, Apr 19, 2024 at 3:44 PM Eric Dumazet <edumazet@...gle.com> wrote:
>
> On Fri, Apr 19, 2024 at 9:29 AM Jason Xing <kerneljasonxing@...il.com> wrote:
> >
> > On Fri, Apr 19, 2024 at 3:02 PM Eric Dumazet <edumazet@...gle.com> wrote:
> > >
> > > On Fri, Apr 19, 2024 at 4:31 AM Jason Xing <kerneljasonxing@...il.com> wrote:
> > > >
> > > > On Fri, Apr 19, 2024 at 7:26 AM Jason Xing <kerneljasonxing@...il.com> wrote:
> > > > >
> > > > > > When I said "If you feel the need to put them in a special group, this
> > > > > > is fine by me.",
> > > > > > this was really about partitioning the existing enum into groups, if
> > > > > > you prefer having a group of 'RES reasons'
> > > > >
> > > > > Are you suggesting copying what we need from enum skb_drop_reason{} to
> > > > > enum sk_rst_reason{}? Why not reusing them directly. I have no idea
> > > > > what the side effect of cast conversion itself is?
> > > >
> > > > Sorry that I'm writing this email. I'm worried my statement is not
> > > > that clear, so I write one simple snippet which can help me explain
> > > > well :)
> > > >
> > > > Allow me give NO_SOCKET as an example:
> > > > diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
> > > > index e63a3bf99617..2c9f7364de45 100644
> > > > --- a/net/ipv4/icmp.c
> > > > +++ b/net/ipv4/icmp.c
> > > > @@ -767,6 +767,7 @@ void __icmp_send(struct sk_buff *skb_in, int type,
> > > > int code, __be32 info,
> > > >         if (!fl4.saddr)
> > > >                 fl4.saddr = htonl(INADDR_DUMMY);
> > > >
> > > > +       trace_icmp_send(skb_in, type, code);
> > > >         icmp_push_reply(sk, &icmp_param, &fl4, &ipc, &rt);
> > > >  ende:
> > > >         ip_rt_put(rt);
> > > > diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
> > > > index 1e650ec71d2f..d5963831280f 100644
> > > > --- a/net/ipv4/tcp_ipv4.c
> > > > +++ b/net/ipv4/tcp_ipv4.c
> > > > @@ -2160,6 +2160,7 @@ int tcp_v4_rcv(struct sk_buff *skb)
> > > >  {
> > > >         struct net *net = dev_net(skb->dev);
> > > >         enum skb_drop_reason drop_reason;
> > > > +       enum sk_rst_reason rst_reason;
> > > >         int sdif = inet_sdif(skb);
> > > >         int dif = inet_iif(skb);
> > > >         const struct iphdr *iph;
> > > > @@ -2355,7 +2356,8 @@ int tcp_v4_rcv(struct sk_buff *skb)
> > > >  bad_packet:
> > > >                 __TCP_INC_STATS(net, TCP_MIB_INERRS);
> > > >         } else {
> > > > -               tcp_v4_send_reset(NULL, skb);
> > > > +               rst_reason = RST_REASON_NO_SOCKET;
> > > > +               tcp_v4_send_reset(NULL, skb, rst_reason);
> > > >         }
> > > >
> > > >  discard_it:
> > > >
> > > > As you can see, we need to add a new 'rst_reason' variable which
> > > > actually is the same as drop reason. They are the same except for the
> > > > enum type... Such rst_reasons/drop_reasons are all over the place.
> > > >
> > > > Eric, if you have a strong preference, I can do it as you said.
> > > >
> > > > Well, how about explicitly casting them like this based on the current
> > > > series. It looks better and clearer and more helpful to people who is
> > > > reading codes to understand:
> > > > diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
> > > > index 461b4d2b7cfe..eb125163d819 100644
> > > > --- a/net/ipv4/tcp_ipv4.c
> > > > +++ b/net/ipv4/tcp_ipv4.c
> > > > @@ -1936,7 +1936,7 @@ int tcp_v4_do_rcv(struct sock *sk, struct sk_buff *skb)
> > > >         return 0;
> > > >
> > > >  reset:
> > > > -       tcp_v4_send_reset(rsk, skb, (u32)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 makes no sense to declare an enum sk_rst_reason and then convert it
> > > to drop_reason
> > > or vice versa.
> > >
> > > Next thing you know, compiler guys will add a new -Woption that will
> > > forbid such conversions.
> > >
> > > Please add to tcp_v4_send_reset() an skb_drop_reason, not a new enum.
> >
> > Ah... It looks like I didn't make myself clear again. Sorry...
> > Actually I wrote this part many times. My conclusion is that It's not
> > feasible to do this.
> >
> > REASONS:
> > If we __only__ need to deal with this passive reset in TCP, it's fine.
> > We pass a skb_drop_reason which should also be converted to u32 type
> > in tcp_v4_send_reset() as you said, it can work. People who use the
> > trace will see the reason with the 'SKB_DROP_REASON' prefix stripped.
> >
> > But we have to deal with other cases. A few questions are listed here:
> > 1) What about tcp_send_active_reset() in TCP/MPTCP? Passing weird drop
> > reasons? There is no drop reason at all. I think people will get
> > confused. So I believe we should invent new definitions to cope with
> > it.
> > 2) What about the .send_reset callback in the subflow_syn_recv_sock()
> > in MPTCP? The reasons in MPTCP are only definitions (such as
> > MPTCP_RST_EUNSPEC). I don't think we can convert them into the enum
> > skb_drop_reason type.
> >
> > So where should we group those various reasons?
> >
> > Introducing a new enum is for extension and compatibility for all
> > kinds of reset reasons.
> >
> > What do you think?
>
> I will stop repeating myself.
>
> enums are not what you think.
>
> type safety is there for a reason.
>
> Can you show me another place in networking stacks where we cast enums
> to others ?

No, I've checked this a month ago.

BTW, I don't know the dangers of casting enum types. I know you will
answer it, but I still insist on asking, hoping someone seeing this
will help me.

Using skb_drop_reason can only deal with the passive reset in TCP. It
is just one of all kinds of reset cases :(

Forgive me, I cannot come up with a good way to cover all the cases TT

I've tried..Sorry...

If other experts see this thread, please help me. I would appreciate
it. I have strong interests and feel strong responsibility to
implement something like this patch series. It can be very useful!!

Thanks again.

Powered by blists - more mailing lists