[PATCH RESEND] io_uring/af_unix: disable sending io_uring over sockets

[PATCH RESEND] io_uring/af_unix: disable sending io_uring over sockets

[Pavel Begunkov]

File reference cycles have caused lots of problems for io_uring
in the past, and it still doesn't work exactly right and races with
unix_stream_read_generic(). The safest fix would be to completely
disallow sending io_uring files via sockets via SCM_RIGHT, so there
are no possible cycles invloving registered files and thus rendering
SCM accounting on the io_uring side unnecessary.

Cc: stable@vger.kernel.org
Fixes: 0091bfc81741b ("io_uring/af_unix: defer registered files gc to io_uring release")
Reported-and-suggested-by: Jann Horn <jannh@google.com>
Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---

Note, it's a minimal patch intended for backporting, all the leftovers
will be cleaned up separately.

 io_uring/rsrc.h | 7 -------
 net/core/scm.c  | 6 ++++++
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/io_uring/rsrc.h b/io_uring/rsrc.h
index 8625181fb87a..08ac0d8e07ef 100644
--- a/io_uring/rsrc.h
+++ b/io_uring/rsrc.h
@@ -77,17 +77,10 @@ int io_sqe_files_register(struct io_ring_ctx *ctx, void __user *arg,
 
 int __io_scm_file_account(struct io_ring_ctx *ctx, struct file *file);
 
-#if defined(CONFIG_UNIX)
-static inline bool io_file_need_scm(struct file *filp)
-{
-	return !!unix_get_socket(filp);
-}
-#else
 static inline bool io_file_need_scm(struct file *filp)
 {
 	return false;
 }
-#endif
 
 static inline int io_scm_file_account(struct io_ring_ctx *ctx,
 				      struct file *file)
diff --git a/net/core/scm.c b/net/core/scm.c
index 880027ecf516..7dc47c17d863 100644
--- a/net/core/scm.c
+++ b/net/core/scm.c
@@ -26,6 +26,7 @@
 #include <linux/nsproxy.h>
 #include <linux/slab.h>
 #include <linux/errqueue.h>
+#include <linux/io_uring.h>
 
 #include <linux/uaccess.h>
 
@@ -103,6 +104,11 @@ static int scm_fp_copy(struct cmsghdr *cmsg, struct scm_fp_list **fplp)
 
 		if (fd < 0 || !(file = fget_raw(fd)))
 			return -EBADF;
+		/* don't allow io_uring files */
+		if (io_uring_get_socket(file)) {
+			fput(file);
+			return -EINVAL;
+		}
 		*fpp++ = file;
 		fpl->count++;
 	}
-- 
2.43.0

Re: [PATCH RESEND] io_uring/af_unix: disable sending io_uring over sockets

[Jakub Kicinski]

On Wed,  6 Dec 2023 13:55:19 +0000 Pavel Begunkov wrote:
> File reference cycles have caused lots of problems for io_uring
> in the past, and it still doesn't work exactly right and races with
> unix_stream_read_generic(). The safest fix would be to completely
> disallow sending io_uring files via sockets via SCM_RIGHT, so there
> are no possible cycles invloving registered files and thus rendering
> SCM accounting on the io_uring side unnecessary.
> 
> Cc: stable@vger.kernel.org
> Fixes: 0091bfc81741b ("io_uring/af_unix: defer registered files gc to io_uring release")
> Reported-and-suggested-by: Jann Horn <jannh@google.com>
> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>

Acked-by: Jakub Kicinski <kuba@kernel.org>
FWIW

Re: [PATCH RESEND] io_uring/af_unix: disable sending io_uring over sockets

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by David S. Miller <davem@davemloft.net>:

On Wed,  6 Dec 2023 13:55:19 +0000 you wrote:
> File reference cycles have caused lots of problems for io_uring
> in the past, and it still doesn't work exactly right and races with
> unix_stream_read_generic(). The safest fix would be to completely
> disallow sending io_uring files via sockets via SCM_RIGHT, so there
> are no possible cycles invloving registered files and thus rendering
> SCM accounting on the io_uring side unnecessary.
> 
> [...]

Here is the summary with links:
  - [RESEND] io_uring/af_unix: disable sending io_uring over sockets
    https://git.kernel.org/netdev/net/c/69db702c8387

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH RESEND] io_uring/af_unix: disable sending io_uring over sockets

[Pavel Begunkov]

On 12/9/23 21:30, patchwork-bot+netdevbpf@kernel.org wrote:
> Hello:
> 
> This patch was applied to netdev/net.git (main)
> by David S. Miller <davem@davemloft.net>:
> 
> On Wed,  6 Dec 2023 13:55:19 +0000 you wrote:
>> File reference cycles have caused lots of problems for io_uring
>> in the past, and it still doesn't work exactly right and races with
>> unix_stream_read_generic(). The safest fix would be to completely
>> disallow sending io_uring files via sockets via SCM_RIGHT, so there
>> are no possible cycles invloving registered files and thus rendering
>> SCM accounting on the io_uring side unnecessary.
>>
>> [...]
> 
> Here is the summary with links:
>    - [RESEND] io_uring/af_unix: disable sending io_uring over sockets
>      https://git.kernel.org/netdev/net/c/69db702c8387

It has already been taken by Jens into the io_uring tree, and a pr
with it was merged by Linus. I think it should be dropped from
the net tree?

-- 
Pavel Begunkov

Re: [PATCH RESEND] io_uring/af_unix: disable sending io_uring over sockets

[Jakub Kicinski]

On Sun, 10 Dec 2023 01:18:00 +0000 Pavel Begunkov wrote:
> > Here is the summary with links:
> >    - [RESEND] io_uring/af_unix: disable sending io_uring over sockets
> >      https://git.kernel.org/netdev/net/c/69db702c8387  
> 
> It has already been taken by Jens into the io_uring tree, and a pr
> with it was merged by Linus. I think it should be dropped from
> the net tree?

Ugh, I think if I revert it now it can only hurt.
Git will figure out that the change is identical, and won't complain 
at the merge (unless we change it again on top, IIUC).

If I may, however, in the most polite way possible put forward 
the suggestion to send a notification to the list when patch is
applied, it helps avoid such confusion... I do hate most automated 
emails myself, but an "applied" notification is good.

Re: [PATCH RESEND] io_uring/af_unix: disable sending io_uring over sockets

[Jens Axboe]

On Dec 11, 2023, at 7:39 PM, Jakub Kicinski <kuba@kernel.org> wrote:
> 
> On Sun, 10 Dec 2023 01:18:00 +0000 Pavel Begunkov wrote:
>>> Here is the summary with links:
>>>   - [RESEND] io_uring/af_unix: disable sending io_uring over sockets
>>>     https://git.kernel.org/netdev/net/c/69db702c8387  
>> 
>> It has already been taken by Jens into the io_uring tree, and a pr
>> with it was merged by Linus. I think it should be dropped from
>> the net tree?
> 
> Ugh, I think if I revert it now it can only hurt.
> Git will figure out that the change is identical, and won't complain
> at the merge (unless we change it again on top, IIUC).

Yeah, git will handle it just fine, it’ll just be an empty duplicate. Annoying, but not the end of the world. 

> If I may, however, in the most polite way possible put forward
> the suggestion to send a notification to the list when patch is
> applied, it helps avoid such confusion... I do hate most automated
> emails myself, but an "applied" notification is good.

I did do that, I always do. But looks like b4 replies to the first email rather than the one that had netdev cc’ed, which may be why this happened in the first place. 

— 
Jens Axboe