From: Yewon Choi <woni9911@gmail.com>
To: Wenjia Zhang <wenjia@linux.ibm.com>,
Jan Karcher <jaka@linux.ibm.com>,
"D. Wythe" <alibuda@linux.alibaba.com>,
Tony Lu <tonylu@linux.alibaba.com>,
Wen Gu <guwen@linux.alibaba.com>,
"David S. Miller" <davem@davemloft.net>,
Eric Dumazet <edumazet@google.com>,
Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
linux-s390@vger.kernel.org, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org
Cc: "Dae R. Jeong" <threeearcat@gmail.com>
Subject: net/smc: Buggy reordering scenario in smc socket
Date: Mon, 15 Apr 2024 20:02:05 +0900 [thread overview]
Message-ID: <Zh0JLYHtd0i416XO@libra05> (raw)
Hello,
we suspect some buggy scenario due to memory reordering in concurrent execution
of setsockopt() and sendmmsg().
(CPU 1) setsockopt():
case TCP_FASTOPEN_NO_COOKIE:
...
smc_switch_to_fallback():
clcsock->file = sk.sk_socket->file; // (1)
clcsock->file->private_data = clcsock; // (2)
(CPU 2) __sys_sendmmsg():
sockfd_lookup_light():
sock_from_file():
sock = file->private_data; // (3)
...
fput_light(sock->file, fput_needed): // (4)
fput():
refcount_dec_and_test(sock->file->f_count) // null-ptr-deref
There is no memory barrier between (1) and (2), so (1) might be reordered after
(2) is written to memory. Then, execution order can be (2)->(3)->(4)->(1)
and (4) will read uninitialized value which may cause system crash.
This kind of reordering may happen in smc_ulp_init():
(CPU 1) smc_ulp_init():
...
smcsock->file = tcp->file; // (5)
smcsock->file->private_data = smcsock; // (6)
Execution order can be (6)->(3)->(4)->(5), showing same symptom as above.
One possible solution seems to be adding release semantic in (2) and (6).
diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
index 4b52b3b159c0..37c23ef3e2d5 100644
--- a/net/smc/af_smc.c
+++ b/net/smc/af_smc.c
@@ -921,7 +921,7 @@ static int smc_switch_to_fallback(struct smc_sock *smc, int reason_code)
trace_smc_switch_to_fallback(smc, reason_code);
if (smc->sk.sk_socket && smc->sk.sk_socket->file) {
smc->clcsock->file = smc->sk.sk_socket->file;
- smc->clcsock->file->private_data = smc->clcsock;
+ smp_store_release(&smc->clcsock->file->private_data, smc->clcsock);
smc->clcsock->wq.fasync_list =
smc->sk.sk_socket->wq.fasync_list;
smc->sk.sk_socket->wq.fasync_list = NULL;
@@ -3410,7 +3410,7 @@ static int smc_ulp_init(struct sock *sk)
/* replace tcp socket to smc */
smcsock->file = tcp->file;
- smcsock->file->private_data = smcsock;
+ smp_store_release(&smcsock->file->private_data, smcsock);
smcsock->file->f_inode = SOCK_INODE(smcsock); /* replace inode when sock_close */
smcsock->file->f_path.dentry->d_inode = SOCK_INODE(smcsock); /* dput() in __fput */
tcp->file = NULL;
I think we don't need memory barrier between (3) and (4) because there are
critical section between (3) and (4), so lock(lock_sock/release_sock) will do this.
Could you check these? If confirmed to be a bug, we will send a patch.
Best Regards,
Yewon Choi
next reply other threads:[~2024-04-15 11:02 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-15 11:02 Yewon Choi [this message]
[not found] ` <CAFgxCDwA8Lv8LLwEpJur3FKs=Gkkc0KE=bx7Q1Do2+iwdAzoCw@mail.gmail.com>
2024-04-15 11:16 ` net/smc: Buggy reordering scenario in smc socket Yewon Choi
2024-04-16 2:15 ` Tony Lu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Zh0JLYHtd0i416XO@libra05 \
--to=woni9911@gmail.com \
--cc=alibuda@linux.alibaba.com \
--cc=davem@davemloft.net \
--cc=edumazet@google.com \
--cc=guwen@linux.alibaba.com \
--cc=jaka@linux.ibm.com \
--cc=kuba@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-s390@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=threeearcat@gmail.com \
--cc=tonylu@linux.alibaba.com \
--cc=wenjia@linux.ibm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).