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 <[email protected]>
---
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:
--
2.44.0
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 <[email protected]>
> ---
> 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.
--
Jens Axboe
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!
--
Jens Axboe
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.
--
Jens Axboe
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!
--
Jens Axboe
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#option-3
--
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.
--
Jens Axboe