| Message ID | c716c88321939156909cfa1bd8b0faaf1c804103.1701868795.git.asml.silence@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/1] io_uring/af_unix: disable sending io_uring over sockets | expand |
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++; > }
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 <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
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.
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
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 <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
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
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!
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?
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
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.
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
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++; }
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(-)