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

[PATCH 1/1] 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 1/1] io_uring/af_unix: disable sending io_uring over sockets

[Pavel Begunkov]

On 12/6/23 13:26, 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.

As it involves AF_UNIX I should have CC'ed net maintainers,
I'll be resending it.


> 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++;
>   	}

-- 
Pavel Begunkov

[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 1/1] io_uring/af_unix: disable sending io_uring over sockets

[Jens Axboe]

On Wed, 06 Dec 2023 13:26:47 +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.
> 
> [...]

Applied, thanks!

[1/1] io_uring/af_unix: disable sending io_uring over sockets
      (no commit info)

Best regards,
-- 
Jens Axboe

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

[Jeff Moyer]

Jens Axboe <axboe@kernel.dk> writes:

> On Wed, 06 Dec 2023 13:26:47 +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.
>> 
>> [...]
>
> Applied, thanks!

So, this will break existing users, right?

-Jeff

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

[Jann Horn]

On Fri, Dec 8, 2023 at 4:00 PM Jeff Moyer <jmoyer@redhat.com> wrote:
> Jens Axboe <axboe@kernel.dk> writes:
>
> > On Wed, 06 Dec 2023 13:26:47 +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.
> >>
> >> [...]
> >
> > Applied, thanks!
>
> So, this will break existing users, right?

Do you know of anyone actually sending io_uring FDs over unix domain
sockets? That seems to me like a fairly weird thing to do.

Thinking again about who might possibly do such a thing, the only
usecase I can think of is CRIU; and from what I can tell, CRIU doesn't
yet support io_uring anyway.

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

[Jeff Moyer]

Jann Horn <jannh@google.com> writes:

> On Fri, Dec 8, 2023 at 4:00 PM Jeff Moyer <jmoyer@redhat.com> wrote:
>> Jens Axboe <axboe@kernel.dk> writes:
>>
>> > On Wed, 06 Dec 2023 13:26:47 +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.
>> >>
>> >> [...]
>> >
>> > Applied, thanks!
>>
>> So, this will break existing users, right?
>
> Do you know of anyone actually sending io_uring FDs over unix domain
> sockets?

I do not.  However, it's tough to prove no software is doing this.

> That seems to me like a fairly weird thing to do.

I am often surprised by what developers choose to do.  I attribute that
to my lack of imagination.

> Thinking again about who might possibly do such a thing, the only
> usecase I can think of is CRIU; and from what I can tell, CRIU doesn't
> yet support io_uring anyway.

I'm not lobbying against turning this off, and I'm sure Pavel had
already considered the potential impact before posting.  It would be
good to include that information in the commit message, in my opinion.

Cheers,
Jeff

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

[Jens Axboe]

On 12/8/23 9:06 AM, Jeff Moyer wrote:
>>> So, this will break existing users, right?
>>
>> Do you know of anyone actually sending io_uring FDs over unix domain
>> sockets?
> 
> I do not.  However, it's tough to prove no software is doing this.

This is obviously true, however I think it's very low risk here as it's
mostly a legacy/historic use case and that really should not what's
using io_uring. On top of that, the most efficient ways of using
io_uring will exclude passing and using a ring from a different task
anyway.

>> That seems to me like a fairly weird thing to do.
> 
> I am often surprised by what developers choose to do.  I attribute that
> to my lack of imagination.

I think you stated that very professionally, in my experience the
reasonings are rather different :-)

>> Thinking again about who might possibly do such a thing, the only
>> usecase I can think of is CRIU; and from what I can tell, CRIU doesn't
>> yet support io_uring anyway.
> 
> I'm not lobbying against turning this off, and I'm sure Pavel had
> already considered the potential impact before posting.  It would be
> good to include that information in the commit message, in my opinion.

It's too late for that now, but I can mention it in the pull request at
least.

-- 
Jens Axboe

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

[Jeff Moyer]

Jens Axboe <axboe@kernel.dk> writes:

> On 12/8/23 9:06 AM, Jeff Moyer wrote:
>>>> So, this will break existing users, right?
>>>
>>> Do you know of anyone actually sending io_uring FDs over unix domain
>>> sockets?
>> 
>> I do not.  However, it's tough to prove no software is doing this.
>
> This is obviously true, however I think it's very low risk here as it's
> mostly a legacy/historic use case and that really should not what's
> using io_uring. On top of that, the most efficient ways of using
> io_uring will exclude passing and using a ring from a different task
> anyway.
>
>>> That seems to me like a fairly weird thing to do.
>> 
>> I am often surprised by what developers choose to do.  I attribute that
>> to my lack of imagination.
>
> I think you stated that very professionally, in my experience the
> reasonings are rather different :-)

I thought you might like that.  :)

>>> Thinking again about who might possibly do such a thing, the only
>>> usecase I can think of is CRIU; and from what I can tell, CRIU doesn't
>>> yet support io_uring anyway.
>> 
>> I'm not lobbying against turning this off, and I'm sure Pavel had
>> already considered the potential impact before posting.  It would be
>> good to include that information in the commit message, in my opinion.
>
> It's too late for that now, but I can mention it in the pull request at
> least.

Thanks, Jens, much appreciated.

Cheers,
Jeff

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

[Pavel Begunkov]

On 12/8/23 16:06, Jeff Moyer wrote:
> Jann Horn <jannh@google.com> writes:
> 
>> On Fri, Dec 8, 2023 at 4:00 PM Jeff Moyer <jmoyer@redhat.com> wrote:
>>> Jens Axboe <axboe@kernel.dk> writes:
>>>
>>>> On Wed, 06 Dec 2023 13:26:47 +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.
>>>>>
>>>>> [...]
>>>>
>>>> Applied, thanks!
>>>
>>> So, this will break existing users, right?
>>
>> Do you know of anyone actually sending io_uring FDs over unix domain
>> sockets?
> 
> I do not.  However, it's tough to prove no software is doing this.
> 
>> That seems to me like a fairly weird thing to do.
> 
> I am often surprised by what developers choose to do.  I attribute that
> to my lack of imagination.
> 
>> Thinking again about who might possibly do such a thing, the only
>> usecase I can think of is CRIU; and from what I can tell, CRIU doesn't
>> yet support io_uring anyway.
> 
> I'm not lobbying against turning this off, and I'm sure Pavel had
> already considered the potential impact before posting.  It would be

Jens already replied, but I wanted to add that it was discussed and
decided to go forward with, because there are no too obvious / known
users, and the benefits including safety outweigh it.

> good to include that information in the commit message, in my opinion.

Yeah, agree with that

-- 
Pavel Begunkov

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