public inbox for [email protected]
 help / color / mirror / Atom feed
* [PATCH 1/1] io_uring/af_unix: disable sending io_uring over sockets
@ 2023-12-06 13:26 Pavel Begunkov
  2023-12-06 13:53 ` Pavel Begunkov
                   ` (4 more replies)
  0 siblings, 5 replies; 15+ messages in thread
From: Pavel Begunkov @ 2023-12-06 13:26 UTC (permalink / raw)
  To: io-uring; +Cc: Jens Axboe, asml.silence, jannh

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: [email protected]
Fixes: 0091bfc81741b ("io_uring/af_unix: defer registered files gc to io_uring release")
Reported-and-suggested-by: Jann Horn <[email protected]>
Signed-off-by: Pavel Begunkov <[email protected]>
---

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


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/1] io_uring/af_unix: disable sending io_uring over sockets
  2023-12-06 13:26 [PATCH 1/1] io_uring/af_unix: disable sending io_uring over sockets Pavel Begunkov
@ 2023-12-06 13:53 ` Pavel Begunkov
  2023-12-06 13:55 ` [PATCH RESEND] " Pavel Begunkov
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 15+ messages in thread
From: Pavel Begunkov @ 2023-12-06 13:53 UTC (permalink / raw)
  To: io-uring; +Cc: Jens Axboe, jannh

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: [email protected]
> Fixes: 0091bfc81741b ("io_uring/af_unix: defer registered files gc to io_uring release")
> Reported-and-suggested-by: Jann Horn <[email protected]>
> Signed-off-by: Pavel Begunkov <[email protected]>
> ---
> 
> 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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH RESEND] io_uring/af_unix: disable sending io_uring over sockets
  2023-12-06 13:26 [PATCH 1/1] io_uring/af_unix: disable sending io_uring over sockets Pavel Begunkov
  2023-12-06 13:53 ` Pavel Begunkov
@ 2023-12-06 13:55 ` Pavel Begunkov
  2023-12-07 20:33 ` [PATCH 1/1] " Jens Axboe
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 15+ messages in thread
From: Pavel Begunkov @ 2023-12-06 13:55 UTC (permalink / raw)
  To: io-uring
  Cc: Jens Axboe, asml.silence, jannh, davem, kuba, edumazet, pabeni,
	netdev

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: [email protected]
Fixes: 0091bfc81741b ("io_uring/af_unix: defer registered files gc to io_uring release")
Reported-and-suggested-by: Jann Horn <[email protected]>
Signed-off-by: Pavel Begunkov <[email protected]>
---

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


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/1] io_uring/af_unix: disable sending io_uring over sockets
  2023-12-06 13:26 [PATCH 1/1] io_uring/af_unix: disable sending io_uring over sockets Pavel Begunkov
  2023-12-06 13:53 ` Pavel Begunkov
  2023-12-06 13:55 ` [PATCH RESEND] " Pavel Begunkov
@ 2023-12-07 20:33 ` Jens Axboe
  2023-12-08 14:59   ` Jeff Moyer
  2023-12-09  1:40 ` [PATCH RESEND] " Jakub Kicinski
  2023-12-09 21:30 ` patchwork-bot+netdevbpf
  4 siblings, 1 reply; 15+ messages in thread
From: Jens Axboe @ 2023-12-07 20:33 UTC (permalink / raw)
  To: io-uring, Pavel Begunkov; +Cc: jannh


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




^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/1] io_uring/af_unix: disable sending io_uring over sockets
  2023-12-07 20:33 ` [PATCH 1/1] " Jens Axboe
@ 2023-12-08 14:59   ` Jeff Moyer
  2023-12-08 15:09     ` Jann Horn
  0 siblings, 1 reply; 15+ messages in thread
From: Jeff Moyer @ 2023-12-08 14:59 UTC (permalink / raw)
  To: Jens Axboe; +Cc: io-uring, Pavel Begunkov, jannh

Jens Axboe <[email protected]> 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


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/1] io_uring/af_unix: disable sending io_uring over sockets
  2023-12-08 14:59   ` Jeff Moyer
@ 2023-12-08 15:09     ` Jann Horn
  2023-12-08 16:06       ` Jeff Moyer
  0 siblings, 1 reply; 15+ messages in thread
From: Jann Horn @ 2023-12-08 15:09 UTC (permalink / raw)
  To: Jeff Moyer; +Cc: Jens Axboe, io-uring, Pavel Begunkov

On Fri, Dec 8, 2023 at 4:00 PM Jeff Moyer <[email protected]> wrote:
> Jens Axboe <[email protected]> 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.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/1] io_uring/af_unix: disable sending io_uring over sockets
  2023-12-08 15:09     ` Jann Horn
@ 2023-12-08 16:06       ` Jeff Moyer
  2023-12-08 16:28         ` Jens Axboe
  2023-12-10  1:23         ` Pavel Begunkov
  0 siblings, 2 replies; 15+ messages in thread
From: Jeff Moyer @ 2023-12-08 16:06 UTC (permalink / raw)
  To: Jann Horn; +Cc: Jens Axboe, io-uring, Pavel Begunkov

Jann Horn <[email protected]> writes:

> On Fri, Dec 8, 2023 at 4:00 PM Jeff Moyer <[email protected]> wrote:
>> Jens Axboe <[email protected]> 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


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/1] io_uring/af_unix: disable sending io_uring over sockets
  2023-12-08 16:06       ` Jeff Moyer
@ 2023-12-08 16:28         ` Jens Axboe
  2023-12-08 17:08           ` Jeff Moyer
  2023-12-10  1:23         ` Pavel Begunkov
  1 sibling, 1 reply; 15+ messages in thread
From: Jens Axboe @ 2023-12-08 16:28 UTC (permalink / raw)
  To: Jeff Moyer, Jann Horn; +Cc: io-uring, Pavel Begunkov

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


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/1] io_uring/af_unix: disable sending io_uring over sockets
  2023-12-08 16:28         ` Jens Axboe
@ 2023-12-08 17:08           ` Jeff Moyer
  0 siblings, 0 replies; 15+ messages in thread
From: Jeff Moyer @ 2023-12-08 17:08 UTC (permalink / raw)
  To: Jens Axboe; +Cc: Jann Horn, io-uring, Pavel Begunkov

Jens Axboe <[email protected]> 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


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH RESEND] io_uring/af_unix: disable sending io_uring over sockets
  2023-12-06 13:26 [PATCH 1/1] io_uring/af_unix: disable sending io_uring over sockets Pavel Begunkov
                   ` (2 preceding siblings ...)
  2023-12-07 20:33 ` [PATCH 1/1] " Jens Axboe
@ 2023-12-09  1:40 ` Jakub Kicinski
  2023-12-09 21:30 ` patchwork-bot+netdevbpf
  4 siblings, 0 replies; 15+ messages in thread
From: Jakub Kicinski @ 2023-12-09  1:40 UTC (permalink / raw)
  To: Pavel Begunkov
  Cc: io-uring, Jens Axboe, jannh, davem, edumazet, pabeni, netdev

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: [email protected]
> Fixes: 0091bfc81741b ("io_uring/af_unix: defer registered files gc to io_uring release")
> Reported-and-suggested-by: Jann Horn <[email protected]>
> Signed-off-by: Pavel Begunkov <[email protected]>

Acked-by: Jakub Kicinski <[email protected]>
FWIW

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH RESEND] io_uring/af_unix: disable sending io_uring over sockets
  2023-12-06 13:26 [PATCH 1/1] io_uring/af_unix: disable sending io_uring over sockets Pavel Begunkov
                   ` (3 preceding siblings ...)
  2023-12-09  1:40 ` [PATCH RESEND] " Jakub Kicinski
@ 2023-12-09 21:30 ` patchwork-bot+netdevbpf
  2023-12-10  1:18   ` Pavel Begunkov
  4 siblings, 1 reply; 15+ messages in thread
From: patchwork-bot+netdevbpf @ 2023-12-09 21:30 UTC (permalink / raw)
  To: Pavel Begunkov
  Cc: io-uring, axboe, jannh, davem, kuba, edumazet, pabeni, netdev

Hello:

This patch was applied to netdev/net.git (main)
by David S. Miller <[email protected]>:

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



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH RESEND] io_uring/af_unix: disable sending io_uring over sockets
  2023-12-09 21:30 ` patchwork-bot+netdevbpf
@ 2023-12-10  1:18   ` Pavel Begunkov
  2023-12-12  2:39     ` Jakub Kicinski
  0 siblings, 1 reply; 15+ messages in thread
From: Pavel Begunkov @ 2023-12-10  1:18 UTC (permalink / raw)
  To: patchwork-bot+netdevbpf
  Cc: io-uring, axboe, jannh, davem, kuba, edumazet, pabeni, netdev

On 12/9/23 21:30, [email protected] wrote:
> Hello:
> 
> This patch was applied to netdev/net.git (main)
> by David S. Miller <[email protected]>:
> 
> 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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/1] io_uring/af_unix: disable sending io_uring over sockets
  2023-12-08 16:06       ` Jeff Moyer
  2023-12-08 16:28         ` Jens Axboe
@ 2023-12-10  1:23         ` Pavel Begunkov
  1 sibling, 0 replies; 15+ messages in thread
From: Pavel Begunkov @ 2023-12-10  1:23 UTC (permalink / raw)
  To: Jeff Moyer, Jann Horn; +Cc: Jens Axboe, io-uring

On 12/8/23 16:06, Jeff Moyer wrote:
> Jann Horn <[email protected]> writes:
> 
>> On Fri, Dec 8, 2023 at 4:00 PM Jeff Moyer <[email protected]> wrote:
>>> Jens Axboe <[email protected]> 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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH RESEND] io_uring/af_unix: disable sending io_uring over sockets
  2023-12-10  1:18   ` Pavel Begunkov
@ 2023-12-12  2:39     ` Jakub Kicinski
  2023-12-12  4:45       ` Jens Axboe
  0 siblings, 1 reply; 15+ messages in thread
From: Jakub Kicinski @ 2023-12-12  2:39 UTC (permalink / raw)
  To: Pavel Begunkov
  Cc: patchwork-bot+netdevbpf, io-uring, axboe, jannh, davem, edumazet,
	pabeni, netdev

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.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH RESEND] io_uring/af_unix: disable sending io_uring over sockets
  2023-12-12  2:39     ` Jakub Kicinski
@ 2023-12-12  4:45       ` Jens Axboe
  0 siblings, 0 replies; 15+ messages in thread
From: Jens Axboe @ 2023-12-12  4:45 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: Pavel Begunkov, patchwork-bot+netdevbpf, io-uring, jannh, davem,
	edumazet, pabeni, netdev

On Dec 11, 2023, at 7:39 PM, Jakub Kicinski <[email protected]> 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


^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2023-12-12  4:45 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-12-06 13:26 [PATCH 1/1] io_uring/af_unix: disable sending io_uring over sockets Pavel Begunkov
2023-12-06 13:53 ` Pavel Begunkov
2023-12-06 13:55 ` [PATCH RESEND] " Pavel Begunkov
2023-12-07 20:33 ` [PATCH 1/1] " Jens Axboe
2023-12-08 14:59   ` Jeff Moyer
2023-12-08 15:09     ` Jann Horn
2023-12-08 16:06       ` Jeff Moyer
2023-12-08 16:28         ` Jens Axboe
2023-12-08 17:08           ` Jeff Moyer
2023-12-10  1:23         ` Pavel Begunkov
2023-12-09  1:40 ` [PATCH RESEND] " Jakub Kicinski
2023-12-09 21:30 ` patchwork-bot+netdevbpf
2023-12-10  1:18   ` Pavel Begunkov
2023-12-12  2:39     ` Jakub Kicinski
2023-12-12  4:45       ` Jens Axboe

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox