在 2024/6/15 14:40, Eric Dumazet 写道:
On Sat, Jun 15, 2024 at 12:24 AM Kuniyuki Iwashima <kuniyu@xxxxxxxxxx> wrote:
From: luoxuanqiang <luoxuanqiang@xxxxxxxxxx>
Date: Fri, 14 Jun 2024 20:42:07 +0800
在 2024/6/14 18:54, Florian Westphal 写道:
luoxuanqiang <luoxuanqiang@xxxxxxxxxx> wrote:
include/net/inet_connection_sock.h | 2 +-
net/dccp/ipv4.c | 2 +-
net/dccp/ipv6.c | 2 +-
net/ipv4/inet_connection_sock.c | 15 +++++++++++----
net/ipv4/tcp_input.c | 11 ++++++++++-
5 files changed, 24 insertions(+), 8 deletions(-)
diff --git a/include/net/inet_connection_sock.h b/include/net/inet_connection_sock.h
index 7d6b1254c92d..8773d161d184 100644
--- a/include/net/inet_connection_sock.h
+++ b/include/net/inet_connection_sock.h
@@ -264,7 +264,7 @@ struct sock *inet_csk_reqsk_queue_add(struct sock *sk,
struct request_sock *req,
struct sock *child);
void inet_csk_reqsk_queue_hash_add(struct sock *sk, struct request_sock *req,
- unsigned long timeout);
+ unsigned long timeout, bool *found_dup_sk);
Nit:
I think it would be preferrable to change retval to bool rather than
bool *found_dup_sk extra arg, so one can do
+1
bool inet_csk_reqsk_queue_hash_add(struct sock *sk, struct request_sock *req,
unsigned long timeout)
{
if (!reqsk_queue_hash_req(req, timeout))
return false;
i.e. let retval indicate wheter reqsk was inserted or not.
Patch looks good to me otherwise.
Thank you for your confirmation!
Regarding your suggestion, I had considered it before,
but besides tcp_conn_request() calling inet_csk_reqsk_queue_hash_add(),
dccp_v4(v6)_conn_request() also calls it. However, there is no
consideration for a failed insertion within that function, so it's
reasonable to let the caller decide whether to check for duplicate
reqsk.
I guess you followed 01770a1661657 where found_dup_sk was introduced,
but note that the commit is specific to TCP SYN Cookie and TCP Fast Open
and DCCP is not related.
Then, own_req is common to TCP and DCCP, so found_dup_sk was added as an
additional argument.
However, another similar commit 5e0724d027f05 actually added own_req check
in DCCP path.
I personally would'nt care if DCCP was not changed to handle such a
failure because DCCP will be removed next year, but I still prefer
Florian's suggestion.
Other things to consider :
- I presume this patch targets net tree, and luoxuanqiang needs the
fix to reach stable trees.
- This means a Fixes: tag is needed
- This also means that we should favor a patch with no or trivial
conflicts for stable backports.
Should the patch target the net-next tree, then the requirements can
be different.
Hello Eric and Kuniyuk,
Thank you for the information!
I've tested the kernel versions 4.19 and 6.10, and they both have
similar issues (I suspect this problem has been around for quite some
time). My intention is to propose a fix to the more stable branches as
soon as possible to cover a wider range. Like Eric mentioned, I hope to
minimize conflicts, so I expect to keep the original DCCP logic intact
and refer to the check for found_dup_sk in 01770a1661657. For DCCP, if
insertion into ehash fails, we might also need to consider handling
rsk_refcnt, as tcp_conn_request() requires rsk_refcnt to be 0 to release
reqsk.
Of course, if DCCP will be removed from net-next, I agree with
Kuniyuki and Florian's suggestions and will envision a better commit
content.
BRs!
