No upstream commit exists for this patch.
Backport of commit 705318a99a13 ("io_uring/af_unix: disable sending io_uring over sockets") introduced registered files leaks in 5.10/5.15 stable branches when CONFIG_UNIX is enabled.
The 5.10/5.15 backports removed io_sqe_file_register() calls from io_install_fixed_file() and __io_sqe_files_update() so that newly added files aren't passed to UNIX-related skbs and thus can't be put during unregistering process. Skbs in the ring socket receive queue are released but there is no skb having reference to the newly updated file.
In other words, when CONFIG_UNIX is enabled there would be no fput() when files are unregistered for the corresponding fget() from io_install_fixed_file() and __io_sqe_files_update().
Drop several code paths related to SCM_RIGHTS as a partial change from commit 6e5e6d274956 ("io_uring: drop any code related to SCM_RIGHTS"). This code is useless in stable branches now, too, but is causing leaks in 5.10/5.15.
As stated above, the affected code was removed in upstream by commit 6e5e6d274956 ("io_uring: drop any code related to SCM_RIGHTS").
Fresher stables from 6.1 have io_file_need_scm() stub function which usage is effectively equivalent to dropping most of SCM-related code.
5.4 seems not to be affected with this problem since SCM-related functions have been dropped there by the backport-patch.
Found by Linux Verification Center (linuxtesting.org) with Syzkaller.
Fixes: 705318a99a13 ("io_uring/af_unix: disable sending io_uring over sockets") Signed-off-by: Fedor Pchelkin pchelkin@ispras.ru --- I feel io_uring-SCM related code should be dropped entirely from the stable branches as the backports already differ greatly between versions and some parts are still kept, some have been dropped in a non-consistent order. Though this might contradict with stable kernel rules or be inappropriate for some other reason.
io_uring/io_uring.c | 177 -------------------------------------------- 1 file changed, 177 deletions(-)
diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c index 936abc6ee450..6ad078a3bf30 100644 --- a/io_uring/io_uring.c +++ b/io_uring/io_uring.c @@ -62,7 +62,6 @@ #include <linux/net.h> #include <net/sock.h> #include <net/af_unix.h> -#include <net/scm.h> #include <linux/anon_inodes.h> #include <linux/sched/mm.h> #include <linux/uaccess.h> @@ -7989,15 +7988,6 @@ static void io_free_file_tables(struct io_file_table *table)
static void __io_sqe_files_unregister(struct io_ring_ctx *ctx) { -#if defined(CONFIG_UNIX) - if (ctx->ring_sock) { - struct sock *sock = ctx->ring_sock->sk; - struct sk_buff *skb; - - while ((skb = skb_dequeue(&sock->sk_receive_queue)) != NULL) - kfree_skb(skb); - } -#else int i;
for (i = 0; i < ctx->nr_user_files; i++) { @@ -8007,7 +7997,6 @@ static void __io_sqe_files_unregister(struct io_ring_ctx *ctx) if (file) fput(file); } -#endif io_free_file_tables(&ctx->file_table); io_rsrc_data_free(ctx->file_data); ctx->file_data = NULL; @@ -8159,170 +8148,10 @@ static struct io_sq_data *io_get_sq_data(struct io_uring_params *p, return sqd; }
-#if defined(CONFIG_UNIX) -/* - * Ensure the UNIX gc is aware of our file set, so we are certain that - * the io_uring can be safely unregistered on process exit, even if we have - * loops in the file referencing. - */ -static int __io_sqe_files_scm(struct io_ring_ctx *ctx, int nr, int offset) -{ - struct sock *sk = ctx->ring_sock->sk; - struct scm_fp_list *fpl; - struct sk_buff *skb; - int i, nr_files; - - fpl = kzalloc(sizeof(*fpl), GFP_KERNEL); - if (!fpl) - return -ENOMEM; - - skb = alloc_skb(0, GFP_KERNEL); - if (!skb) { - kfree(fpl); - return -ENOMEM; - } - - skb->sk = sk; - skb->scm_io_uring = 1; - - nr_files = 0; - fpl->user = get_uid(current_user()); - for (i = 0; i < nr; i++) { - struct file *file = io_file_from_index(ctx, i + offset); - - if (!file) - continue; - fpl->fp[nr_files] = get_file(file); - unix_inflight(fpl->user, fpl->fp[nr_files]); - nr_files++; - } - - if (nr_files) { - fpl->max = SCM_MAX_FD; - fpl->count = nr_files; - UNIXCB(skb).fp = fpl; - skb->destructor = unix_destruct_scm; - refcount_add(skb->truesize, &sk->sk_wmem_alloc); - skb_queue_head(&sk->sk_receive_queue, skb); - - for (i = 0; i < nr; i++) { - struct file *file = io_file_from_index(ctx, i + offset); - - if (file) - fput(file); - } - } else { - kfree_skb(skb); - free_uid(fpl->user); - kfree(fpl); - } - - return 0; -} - -/* - * If UNIX sockets are enabled, fd passing can cause a reference cycle which - * causes regular reference counting to break down. We rely on the UNIX - * garbage collection to take care of this problem for us. - */ -static int io_sqe_files_scm(struct io_ring_ctx *ctx) -{ - unsigned left, total; - int ret = 0; - - total = 0; - left = ctx->nr_user_files; - while (left) { - unsigned this_files = min_t(unsigned, left, SCM_MAX_FD); - - ret = __io_sqe_files_scm(ctx, this_files, total); - if (ret) - break; - left -= this_files; - total += this_files; - } - - if (!ret) - return 0; - - while (total < ctx->nr_user_files) { - struct file *file = io_file_from_index(ctx, total); - - if (file) - fput(file); - total++; - } - - return ret; -} -#else -static int io_sqe_files_scm(struct io_ring_ctx *ctx) -{ - return 0; -} -#endif - static void io_rsrc_file_put(struct io_ring_ctx *ctx, struct io_rsrc_put *prsrc) { struct file *file = prsrc->file; -#if defined(CONFIG_UNIX) - struct sock *sock = ctx->ring_sock->sk; - struct sk_buff_head list, *head = &sock->sk_receive_queue; - struct sk_buff *skb; - int i; - - __skb_queue_head_init(&list); - - /* - * Find the skb that holds this file in its SCM_RIGHTS. When found, - * remove this entry and rearrange the file array. - */ - skb = skb_dequeue(head); - while (skb) { - struct scm_fp_list *fp; - - fp = UNIXCB(skb).fp; - for (i = 0; i < fp->count; i++) { - int left; - - if (fp->fp[i] != file) - continue; - - unix_notinflight(fp->user, fp->fp[i]); - left = fp->count - 1 - i; - if (left) { - memmove(&fp->fp[i], &fp->fp[i + 1], - left * sizeof(struct file *)); - } - fp->count--; - if (!fp->count) { - kfree_skb(skb); - skb = NULL; - } else { - __skb_queue_tail(&list, skb); - } - fput(file); - file = NULL; - break; - } - - if (!file) - break; - - __skb_queue_tail(&list, skb); - - skb = skb_dequeue(head); - } - - if (skb_peek(&list)) { - spin_lock_irq(&head->lock); - while ((skb = __skb_dequeue(&list)) != NULL) - __skb_queue_tail(head, skb); - spin_unlock_irq(&head->lock); - } -#else fput(file); -#endif }
static void __io_rsrc_put_work(struct io_rsrc_node *ref_node) @@ -8433,12 +8262,6 @@ static int io_sqe_files_register(struct io_ring_ctx *ctx, void __user *arg, io_fixed_file_set(io_fixed_file_slot(&ctx->file_table, i), file); }
- ret = io_sqe_files_scm(ctx); - if (ret) { - __io_sqe_files_unregister(ctx); - return ret; - } - io_rsrc_node_switch(ctx, NULL); return ret; out_fput:
On 3/12/24 8:23 AM, Fedor Pchelkin wrote:
No upstream commit exists for this patch.
Backport of commit 705318a99a13 ("io_uring/af_unix: disable sending io_uring over sockets") introduced registered files leaks in 5.10/5.15 stable branches when CONFIG_UNIX is enabled.
The 5.10/5.15 backports removed io_sqe_file_register() calls from io_install_fixed_file() and __io_sqe_files_update() so that newly added files aren't passed to UNIX-related skbs and thus can't be put during unregistering process. Skbs in the ring socket receive queue are released but there is no skb having reference to the newly updated file.
In other words, when CONFIG_UNIX is enabled there would be no fput() when files are unregistered for the corresponding fget() from io_install_fixed_file() and __io_sqe_files_update().
Drop several code paths related to SCM_RIGHTS as a partial change from commit 6e5e6d274956 ("io_uring: drop any code related to SCM_RIGHTS"). This code is useless in stable branches now, too, but is causing leaks in 5.10/5.15.
As stated above, the affected code was removed in upstream by commit 6e5e6d274956 ("io_uring: drop any code related to SCM_RIGHTS").
Fresher stables from 6.1 have io_file_need_scm() stub function which usage is effectively equivalent to dropping most of SCM-related code.
5.4 seems not to be affected with this problem since SCM-related functions have been dropped there by the backport-patch.
Found by Linux Verification Center (linuxtesting.org) with Syzkaller.
Fixes: 705318a99a13 ("io_uring/af_unix: disable sending io_uring over sockets") Signed-off-by: Fedor Pchelkin pchelkin@ispras.ru
I feel io_uring-SCM related code should be dropped entirely from the stable branches as the backports already differ greatly between versions and some parts are still kept, some have been dropped in a non-consistent order. Though this might contradict with stable kernel rules or be inappropriate for some other reason.
Looks fine to me, and I agree, it makes much more sense to drop it all from 5.10/5.15-stable as well to keep them in sync with upstream. And I think this is fine for stable, dropping code is always a good thing.
On 24/03/12 08:34AM, Jens Axboe wrote:
On 3/12/24 8:23 AM, Fedor Pchelkin wrote:
[...]
I feel io_uring-SCM related code should be dropped entirely from the stable branches as the backports already differ greatly between versions and some parts are still kept, some have been dropped in a non-consistent order. Though this might contradict with stable kernel rules or be inappropriate for some other reason.
Looks fine to me, and I agree, it makes much more sense to drop it all from 5.10/5.15-stable as well to keep them in sync with upstream. And I think this is fine for stable, dropping code is always a good thing.
Alright, got it. So that would require dropping it from all of the supported 5.4, 6.1, 6.6, 6.7, too.
Would it be okay if I'll send this as a series?
-- Fedor
On 3/12/24 9:14 AM, Fedor Pchelkin wrote:
On 24/03/12 08:34AM, Jens Axboe wrote:
On 3/12/24 8:23 AM, Fedor Pchelkin wrote:
[...]
I feel io_uring-SCM related code should be dropped entirely from the stable branches as the backports already differ greatly between versions and some parts are still kept, some have been dropped in a non-consistent order. Though this might contradict with stable kernel rules or be inappropriate for some other reason.
Looks fine to me, and I agree, it makes much more sense to drop it all from 5.10/5.15-stable as well to keep them in sync with upstream. And I think this is fine for stable, dropping code is always a good thing.
Alright, got it. So that would require dropping it from all of the supported 5.4, 6.1, 6.6, 6.7, too.
Would it be okay if I'll send this as a series?
Yeah I think so, keeping the code more in sync is always a good thing when it comes to stable. Just make sure you mark the backport commits with the appropriate upstream shas. Thanks!
On 3/12/24 9:21 AM, Jens Axboe wrote:
On 3/12/24 9:14 AM, Fedor Pchelkin wrote:
On 24/03/12 08:34AM, Jens Axboe wrote:
On 3/12/24 8:23 AM, Fedor Pchelkin wrote:
[...]
I feel io_uring-SCM related code should be dropped entirely from the stable branches as the backports already differ greatly between versions and some parts are still kept, some have been dropped in a non-consistent order. Though this might contradict with stable kernel rules or be inappropriate for some other reason.
Looks fine to me, and I agree, it makes much more sense to drop it all from 5.10/5.15-stable as well to keep them in sync with upstream. And I think this is fine for stable, dropping code is always a good thing.
Alright, got it. So that would require dropping it from all of the supported 5.4, 6.1, 6.6, 6.7, too.
Would it be okay if I'll send this as a series?
Yeah I think so, keeping the code more in sync is always a good thing when it comes to stable. Just make sure you mark the backport commits with the appropriate upstream shas. Thanks!
I'll just do these backports myself, thanks for bringing it up.
On 24/03/12 11:54AM, Jens Axboe wrote:
On 3/12/24 9:21 AM, Jens Axboe wrote:
On 3/12/24 9:14 AM, Fedor Pchelkin wrote:
On 24/03/12 08:34AM, Jens Axboe wrote:
On 3/12/24 8:23 AM, Fedor Pchelkin wrote:
[...]
I feel io_uring-SCM related code should be dropped entirely from the stable branches as the backports already differ greatly between versions and some parts are still kept, some have been dropped in a non-consistent order. Though this might contradict with stable kernel rules or be inappropriate for some other reason.
Looks fine to me, and I agree, it makes much more sense to drop it all from 5.10/5.15-stable as well to keep them in sync with upstream. And I think this is fine for stable, dropping code is always a good thing.
Alright, got it. So that would require dropping it from all of the supported 5.4, 6.1, 6.6, 6.7, too.
Would it be okay if I'll send this as a series?
Yeah I think so, keeping the code more in sync is always a good thing when it comes to stable. Just make sure you mark the backport commits with the appropriate upstream shas. Thanks!
I'll just do these backports myself, thanks for bringing it up.
Great, thanks!
-- Fedor
Hi,
OK, here they are. Two patches attached for every stable kernel, that gets rid of the remnants of the SCM related code:
5.4 5.10 and 5.15 (same patches) 6.1 6.6 6.7
Would appreciate if Fedor and Pavel could give them a once over, but I think they are all fine. It's just deleting the code...
Barring no complaints, Greg can you queue them up? Thanks!
On 24/03/13 06:40PM, Jens Axboe wrote:
Hi,
OK, here they are. Two patches attached for every stable kernel, that gets rid of the remnants of the SCM related code:
5.4 5.10 and 5.15 (same patches) 6.1 6.6 6.7
Would appreciate if Fedor and Pavel could give them a once over, but I think they are all fine. It's just deleting the code...
Thank you, Jens!
FWIW, I think it's all good and it eliminates the reported problem obviously. Compiled and tested the repro with my kernel config.
Just a minor notice - stable rules declare two common ways for upstream patch mentioning in backports [1]. And the first one starts from lowercase. No big deal here definitely but maybe somebody has some handling of these two variants - by regexps or similar, I actually don't know. But I see in the git history that Greg also applies the variant you've used.
[1]: https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html#opti...
-- Fedor
On 3/14/24 9:55 AM, Fedor Pchelkin wrote:
On 24/03/13 06:40PM, Jens Axboe wrote:
Hi,
OK, here they are. Two patches attached for every stable kernel, that gets rid of the remnants of the SCM related code:
5.4 5.10 and 5.15 (same patches) 6.1 6.6 6.7
Would appreciate if Fedor and Pavel could give them a once over, but I think they are all fine. It's just deleting the code...
Thank you, Jens!
FWIW, I think it's all good and it eliminates the reported problem obviously. Compiled and tested the repro with my kernel config.
Great, thanks for checking!
Just a minor notice - stable rules declare two common ways for upstream patch mentioning in backports [1]. And the first one starts from lowercase. No big deal here definitely but maybe somebody has some handling of these two variants - by regexps or similar, I actually don't know. But I see in the git history that Greg also applies the variant you've used.
Honestly that doesn't matter, as long as the upstream commit is referenced. I always do it this way, not my first stable rodeo.
Hi Greg/stable,
Can you queue these up? Two patches for each stable kernel, all named appropriately.
Thanks!
-------- Forwarded Message -------- Subject: Re: [PATCH 5.10/5.15] io_uring: fix registered files leak Date: Wed, 13 Mar 2024 18:40:29 -0600 From: Jens Axboe axboe@kernel.dk To: Fedor Pchelkin pchelkin@ispras.ru CC: Pavel Begunkov asml.silence@gmail.com, Greg Kroah-Hartman gregkh@linuxfoundation.org, stable@vger.kernel.org, io-uring@vger.kernel.org, linux-kernel@vger.kernel.org, Alexey Khoroshilov khoroshilov@ispras.ru, lvc-project@linuxtesting.org, Nikita Zhandarovich n.zhandarovich@fintech.ru, Roman Belyaev belyaevrd@yandex.ru
Hi,
OK, here they are. Two patches attached for every stable kernel, that gets rid of the remnants of the SCM related code:
5.4 5.10 and 5.15 (same patches) 6.1 6.6 6.7
Would appreciate if Fedor and Pavel could give them a once over, but I think they are all fine. It's just deleting the code...
Barring no complaints, Greg can you queue them up? Thanks!
On Thu, Mar 14, 2024 at 10:15:18AM -0600, Jens Axboe wrote:
Hi Greg/stable,
Can you queue these up? Two patches for each stable kernel, all named appropriately.
I'll queue it up, thanks!
On 3/16/24 4:29 AM, Sasha Levin wrote:
On Thu, Mar 14, 2024 at 10:15:18AM -0600, Jens Axboe wrote:
Hi Greg/stable,
Can you queue these up? Two patches for each stable kernel, all named appropriately.
I'll queue it up, thanks!
Thanks!
linux-stable-mirror@lists.linaro.org
-
Fedor Pchelkin -
Jens Axboe -
Sasha Levin