| 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
| ||
|
Message-ID: <a88a0731-6cbe-4987-b1e9-afa51f9ab057@linux.alibaba.com>
Date: Thu, 7 Mar 2024 21:53:13 +0800
From: Wen Gu <guwen@...ux.alibaba.com>
To: Dmitry Antipov <dmantipov@...dex.ru>,
"wenjia@...ux.ibm.com" <wenjia@...ux.ibm.com>
Cc: "lvc-project@...uxtesting.org" <lvc-project@...uxtesting.org>,
"linux-s390@...r.kernel.org" <linux-s390@...r.kernel.org>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"jaka@...ux.ibm.com" <jaka@...ux.ibm.com>
Subject: Re: [lvc-project] [PATCH] [RFC] net: smc: fix fasync leak in
smc_release()
On 2024/3/7 02:07, Dmitry Antipov wrote:
> On 3/6/24 17:45, Wen Gu wrote:
>
>> IIUC, the fallback (or more precisely the private_data change) essentially
>> always happens when the lock_sock(smc->sk) is held, except in smc_listen_work()
>> or smc_listen_decline(), but at that moment, userspace program can not yet
>> acquire this new socket to add fasync entries to the fasync_list.
>>
>> So IMHO, the above patch should work, since it checks the private_data under
>> the lock_sock(sk). But if I missed something, please correct me.
>
> Well, the whole picture is somewhat more complicated. Consider the
> following diagram (an underlying kernel socket is in [], e.g. [smc->sk]):
>
> Thread 0 Thread 1
>
> ioctl(sock, FIOASYNC, [1])
> ...
> sock = filp->private_data;
> lock_sock(sock [smc->sk]);
> sock_fasync(sock, ..., 1) ; new fasync_struct linked to smc->sk
> release_sock(sock [smc->sk]);
> ...
> lock_sock([smc->sk]);
> ...
> smc_switch_to_fallback()
> ...
> smc->clcsock->file->private_data = smc->clcsock;
> ...
> release_sock([smc->sk]);
> ioctl(sock, FIOASYNC, [0])
> ...
> sock = filp->private_data;
> lock_sock(sock [smc->clcsock]);
> sock_fasync(sock, ..., 0) ; nothing to unlink from smc->clcsock
> ; since fasync entry was linked to smc->sk
> release_sock(sock [smc->clcsock]);
I don't understand why the fasync entry now can't be removed from
clcsock->wq.fasync_list? since the fasync entry has been moved to
clcsock->wq.fasync_list during fallback.
The process you described above is:
1) An fasync entry was added into smc->sk.sk_socket->wq.fasync_list;
2) then fallback occurs, and the fasync entry is moved to clcsock->wq.fasync_list,
and file->private_data is changed to smc->clcsock;
3) lastly we removed the fasync entry from clcsock->wq.fasync_list.
It can be reproduced by follows, right?
#include <signal.h>
#include <unistd.h>
#include <pthread.h>
#include <sys/ioctl.h>
#include <sys/socket.h>
#include <stdio.h>
int main (int argc, char *argv[])
{
struct msghdr msg = { 0 };
int sock;
int on;
sock = socket(AF_SMC, SOCK_STREAM, 0);
/* add fasync entry */
on = 1;
ioctl(sock, FIOASYNC, &on);
/* fallback */
sendmsg(sock, &msg, MSG_FASTOPEN);
/* remove fasync entry */
on = 0;
ioctl(sock, FIOASYNC, &on);
close(sock);
return 0;
}
and I added some prints in the kernel for quick debug:
diff --git a/fs/fcntl.c b/fs/fcntl.c
index c80a6acad742..79b8e435c380 100644
--- a/fs/fcntl.c
+++ b/fs/fcntl.c
@@ -880,6 +880,7 @@ int fasync_remove_entry(struct file *filp, struct fasync_struct **fapp)
call_rcu(&fa->fa_rcu, fasync_free_rcu);
filp->f_flags &= ~FASYNC;
result = 1;
+ pr_warn("%s: wq->fasync_list %pK, fasync entry %pK\n", __func__, &(*fapp), fa);
break;
}
spin_unlock(&fasync_lock);
@@ -932,6 +933,7 @@ struct fasync_struct *fasync_insert_entry(int fd, struct file *filp, struct fasy
new->fa_next = *fapp;
rcu_assign_pointer(*fapp, new);
filp->f_flags |= FASYNC;
+ pr_warn("%s: wq->fasync_list %pK, fasync entry %pK\n", __func__, &(*fapp), new);
out:
spin_unlock(&fasync_lock);
diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
index 4b52b3b159c0..3b9737d42dbd 100644
--- a/net/smc/af_smc.c
+++ b/net/smc/af_smc.c
@@ -925,6 +925,9 @@ static int smc_switch_to_fallback(struct smc_sock *smc, int reason_code)
smc->clcsock->wq.fasync_list =
smc->sk.sk_socket->wq.fasync_list;
smc->sk.sk_socket->wq.fasync_list = NULL;
+ pr_warn("%s: smc->sk wq.fasync_list %pK, clcsock->wq.fasync_list %pK\n",
+ __func__, &smc->sk.sk_socket->wq.fasync_list,
+ &smc->clcsock->wq.fasync_list);
/* There might be some wait entries remaining
* in smc sk->sk_wq and they should be woken up
ran the reproducer, and dmesg shows:
[] fasync_insert_entry: wq->fasync_list ffff96fdc0425e98, fasync entry ffff96fdccc62690
[] smc: smc_switch_to_fallback: smc->sk wq.fasync_list ffff96fdc0425e98, clcsock->wq.fasync_list ffff96fdc0426ed8
[] fasync_remove_entry: wq->fasync_list ffff96fdc0426ed8, fasync entry ffff96fdccc62690
It shows that
1. an fasync entry ffff96fdccc62690 is added into ffff96fdc0425e98 (smc->sk wq.fasync_list)
2. then fallback, all the fasync entris in smc->sk wq.fasync_list will be moved to clcsock->wq.fasync_list.
3. then the fasync entry ffff96fdccc62690 (the one in #1) is removed from ffff96fdc0426ed8 (clcsock->wq.fasync_list)
What's wrong with this?
In fact, I think what does cause this leak (maybe one of causes) is the race
I discribed through the diagram in
https://lore.kernel.org/netdev/19d7d71b-c911-45cc-9671-235d98720be6@linux.alibaba.com/
1) sock_fasync() got the filp->private_data->wq (that is smc->sk.sk_socket->wq)
and record it in 'wq';
2) meanwhile, fallback occurs and filp->private_data changed, and from now on,
user can only operate the clcsock based on file->private_data;
3) (race here) and sock_fasync() keep going and add an entry to 'wq'->fasync_list
(that is smc->sk.sk_socket->wq); This fasync entry is the one that we can't
removed later, since we start to operate clcsock after fallback.
Thanks!
> ...
> close(sock [smc->clcsock]);
> __fput(...);
> file->f_op->fasync(sock, [0]) ; always failed -
> ; should use
> ; smc->sk instead
> file->f_op->release()
> ...
> smc_restore_fallback_changes()
> ...
> file->private_data = smc->sk.sk_socket;
>
> That is, smc_restore_fallback_changes() restores filp->private_data to
> smc->sk. If __fput() would have called file->f_op->release() _before_
> file->f_op->fasync(), the fix would be as simple as adding
>
> smc->sk.sk_socket->wq.fasync_list = smc->clcsock->wq.fasync_list;
>
> to smc_restore_fallback_changes(). But since file->f_op->fasync() is called
> before file->f_op->release(), the former always makes an attempt to unlink fasync
> entry from smc->clcsock instead of smc->sk, thus introducing the memory leak.
>
> And an idea with shared wait queue was intended in attempt to eliminate
> this chicken-egg lookalike problem completely.
>
> Dmitry
Powered by blists - more mailing lists