| Message ID | c716c88321939156909cfa1bd8b0faaf1c804103.1701868795.git.asml.silence@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 69db702c83874fbaa2a51af761e35a8e5a593b95 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [RESEND] io_uring/af_unix: disable sending io_uring over sockets | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
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 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(-)