netdev - Re: [PATCH net-next v6 0/7] Implement reset reason mechanism to detect

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: Windows password security audit tool. GUI, reports in PDF.

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

Date: Fri, 19 Apr 2024 10:30:48 +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 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

Thanks for your patience again.

Jason