| Message ID | 20230224071745.20717-1-equinox@diac24.net (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] packet: allow MSG_NOSIGNAL in recvmsg | expand |
On Fri, Feb 24, 2023 at 8:18 AM David Lamparter <equinox@diac24.net> wrote: > > packet_recvmsg() whitelists a bunch of MSG_* flags, which notably does > not include MSG_NOSIGNAL. Unfortunately, io_uring always sets > MSG_NOSIGNAL, meaning AF_PACKET sockets can't be used in io_uring > recvmsg(). > > As AF_PACKET sockets never generate SIGPIPE to begin with, MSG_NOSIGNAL > is a no-op and can simply be ignored. > > Signed-off-by: David Lamparter <equinox@diac24.net> > Cc: Willem de Bruijn <willemdebruijn.kernel@gmail.com> > --- > net/packet/af_packet.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > This is odd... I think MSG_NOSIGNAL flag has a meaning for sendmsg() (or write sides in general) EPIPE is not supposed to be generated at the receiving side ? So I would rather make io_uring slightly faster : diff --git a/io_uring/net.c b/io_uring/net.c index cbd4b725f58c98e5bc5bf88d5707db5c8302e071..b7f190ca528e6e259eb2b072d7a16aaba98848cb 100644 --- a/io_uring/net.c +++ b/io_uring/net.c @@ -567,7 +567,7 @@ int io_recvmsg_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe) sr->flags = READ_ONCE(sqe->ioprio); if (sr->flags & ~(RECVMSG_FLAGS)) return -EINVAL; - sr->msg_flags = READ_ONCE(sqe->msg_flags) | MSG_NOSIGNAL; + sr->msg_flags = READ_ONCE(sqe->msg_flags); if (sr->msg_flags & MSG_DONTWAIT) req->flags |= REQ_F_NOWAIT; if (sr->msg_flags & MSG_ERRQUEUE)
On Fri, Feb 24, 2023 at 11:26:27AM +0100, Eric Dumazet wrote: > On Fri, Feb 24, 2023 at 8:18 AM David Lamparter <equinox@diac24.net> wrote: [...] > > packet_recvmsg() whitelists a bunch of MSG_* flags, which notably does > > not include MSG_NOSIGNAL. Unfortunately, io_uring always sets > > MSG_NOSIGNAL, meaning AF_PACKET sockets can't be used in io_uring > > recvmsg(). > > This is odd... I think MSG_NOSIGNAL flag has a meaning for sendmsg() > (or write sides in general) > > EPIPE is not supposed to be generated at the receiving side ? I would agree, but then again the behavior is inconsistent between socket types. (AF_INET6, SOCK_RAW, ...) works fine with io_uring/MSG_NOSIGNAL, meanwhile setting MSG_NOSIGNAL on (AF_PACKET, SOCK_RAW, ...) gives EINVAL. Just to get consistency, MSG_NOSIGNAL might be worth ignoring in AF_PACKET recvmsg? Independent of dropping it from io_uring... > So I would rather make io_uring slightly faster : [...] > - sr->msg_flags = READ_ONCE(sqe->msg_flags) | MSG_NOSIGNAL; > + sr->msg_flags = READ_ONCE(sqe->msg_flags);
On Fri, Feb 24, 2023 at 1:02 PM David Lamparter <equinox@diac24.net> wrote: > > On Fri, Feb 24, 2023 at 11:26:27AM +0100, Eric Dumazet wrote: > > On Fri, Feb 24, 2023 at 8:18 AM David Lamparter <equinox@diac24.net> wrote: > [...] > > > packet_recvmsg() whitelists a bunch of MSG_* flags, which notably does > > > not include MSG_NOSIGNAL. Unfortunately, io_uring always sets > > > MSG_NOSIGNAL, meaning AF_PACKET sockets can't be used in io_uring > > > recvmsg(). > > > > This is odd... I think MSG_NOSIGNAL flag has a meaning for sendmsg() > > (or write sides in general) > > > > EPIPE is not supposed to be generated at the receiving side ? > > I would agree, but then again the behavior is inconsistent between > socket types. (AF_INET6, SOCK_RAW, ...) works fine with > io_uring/MSG_NOSIGNAL, meanwhile setting MSG_NOSIGNAL on (AF_PACKET, > SOCK_RAW, ...) gives EINVAL. > > Just to get consistency, MSG_NOSIGNAL might be worth ignoring in > AF_PACKET recvmsg? Independent of dropping it from io_uring... > Probably because rawv6_recvmsg() never bothered to reject unknown flags. (Maybe the reason for that was that RAW sockets were privileged ones back in linux-2.6) It is too late to add a check there, it might break some applications mistakenly adding MSG_NOSIGNAL (or any currently ignored bits) Consistency would be to make sure no recvmsg() handler pretends MSG_NOSIGNAL has a meaning. Your patch would prevent us using this bit for a future purpose in af_packet. > > So I would rather make io_uring slightly faster : > [...] > > - sr->msg_flags = READ_ONCE(sqe->msg_flags) | MSG_NOSIGNAL; > > + sr->msg_flags = READ_ONCE(sqe->msg_flags);
On 2/24/23 3:26 AM, Eric Dumazet wrote: > On Fri, Feb 24, 2023 at 8:18 AM David Lamparter <equinox@diac24.net> wrote: >> >> packet_recvmsg() whitelists a bunch of MSG_* flags, which notably does >> not include MSG_NOSIGNAL. Unfortunately, io_uring always sets >> MSG_NOSIGNAL, meaning AF_PACKET sockets can't be used in io_uring >> recvmsg(). >> >> As AF_PACKET sockets never generate SIGPIPE to begin with, MSG_NOSIGNAL >> is a no-op and can simply be ignored. >> >> Signed-off-by: David Lamparter <equinox@diac24.net> >> Cc: Willem de Bruijn <willemdebruijn.kernel@gmail.com> >> --- >> net/packet/af_packet.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> > > This is odd... I think MSG_NOSIGNAL flag has a meaning for sendmsg() > (or write sides in general) > > EPIPE is not supposed to be generated at the receiving side ? > > So I would rather make io_uring slightly faster : > > > diff --git a/io_uring/net.c b/io_uring/net.c > index cbd4b725f58c98e5bc5bf88d5707db5c8302e071..b7f190ca528e6e259eb2b072d7a16aaba98848cb > 100644 > --- a/io_uring/net.c > +++ b/io_uring/net.c > @@ -567,7 +567,7 @@ int io_recvmsg_prep(struct io_kiocb *req, const > struct io_uring_sqe *sqe) > sr->flags = READ_ONCE(sqe->ioprio); > if (sr->flags & ~(RECVMSG_FLAGS)) > return -EINVAL; > - sr->msg_flags = READ_ONCE(sqe->msg_flags) | MSG_NOSIGNAL; > + sr->msg_flags = READ_ONCE(sqe->msg_flags); > if (sr->msg_flags & MSG_DONTWAIT) > req->flags |= REQ_F_NOWAIT; > if (sr->msg_flags & MSG_ERRQUEUE) This looks fine to me. Do you want to send a "proper" patch for this?
On Fri, Feb 24, 2023 at 3:32 PM Jens Axboe <axboe@kernel.dk> wrote: > > This looks fine to me. Do you want to send a "proper" patch for > this? Sure, or perhaps David wanted to take care of this. Thanks.
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c index d4e76e2ae153..67c0a57e6dd8 100644 --- a/net/packet/af_packet.c +++ b/net/packet/af_packet.c @@ -3410,7 +3410,7 @@ static int packet_recvmsg(struct socket *sock, struct msghdr *msg, size_t len, unsigned int origlen = 0; err = -EINVAL; - if (flags & ~(MSG_PEEK|MSG_DONTWAIT|MSG_TRUNC|MSG_CMSG_COMPAT|MSG_ERRQUEUE)) + if (flags & ~(MSG_PEEK|MSG_DONTWAIT|MSG_TRUNC|MSG_CMSG_COMPAT|MSG_ERRQUEUE|MSG_NOSIGNAL)) goto out; #if 0
packet_recvmsg() whitelists a bunch of MSG_* flags, which notably does not include MSG_NOSIGNAL. Unfortunately, io_uring always sets MSG_NOSIGNAL, meaning AF_PACKET sockets can't be used in io_uring recvmsg(). As AF_PACKET sockets never generate SIGPIPE to begin with, MSG_NOSIGNAL is a no-op and can simply be ignored. Signed-off-by: David Lamparter <equinox@diac24.net> Cc: Willem de Bruijn <willemdebruijn.kernel@gmail.com> --- net/packet/af_packet.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)