[PATCH net-next] packet: allow MSG_NOSIGNAL in recvmsg

[PATCH net-next] packet: allow MSG_NOSIGNAL in recvmsg

[David Lamparter]

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(-)

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
-- 
2.39.2

Re: [PATCH net-next] packet: allow MSG_NOSIGNAL in recvmsg

[Eric Dumazet]

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)

Re: [PATCH net-next] packet: allow MSG_NOSIGNAL in recvmsg

[David Lamparter]

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);

Re: [PATCH net-next] packet: allow MSG_NOSIGNAL in recvmsg

[Eric Dumazet]

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);

Re: [PATCH net-next] packet: allow MSG_NOSIGNAL in recvmsg

[Jens Axboe]

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?

-- 
Jens Axboe

Re: [PATCH net-next] packet: allow MSG_NOSIGNAL in recvmsg

[Eric Dumazet]

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.

[PATCH] io_uring: remove MSG_NOSIGNAL from recvmsg

[David Lamparter]

MSG_NOSIGNAL is not applicable for the receiving side, SIGPIPE is
generated when trying to write to a "broken pipe".  AF_PACKET's
packet_recvmsg() does enforce this, giving back EINVAL when MSG_NOSIGNAL
is set - making it unuseable in io_uring's recvmsg.

Remove MSG_NOSIGNAL from io_recvmsg_prep().

Signed-off-by: David Lamparter <equinox@diac24.net>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Jens Axboe <axboe@kernel.dk>
---

> Sure, or perhaps David wanted to take care of this.

Here you go.  But maybe give me a few hours to test/confirm...

diff --git a/io_uring/net.c b/io_uring/net.c
index cbd4b725f58c..b7f190ca528e 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)
-- 
2.39.2

Re: [PATCH] io_uring: remove MSG_NOSIGNAL from recvmsg

[David Lamparter]

On Fri, Feb 24, 2023 at 04:01:24PM +0100, David Lamparter wrote:
> MSG_NOSIGNAL is not applicable for the receiving side, SIGPIPE is
> generated when trying to write to a "broken pipe".  AF_PACKET's
> packet_recvmsg() does enforce this, giving back EINVAL when MSG_NOSIGNAL
> is set - making it unuseable in io_uring's recvmsg.
> ---
> > Sure, or perhaps David wanted to take care of this.
> 
> Here you go.  But maybe give me a few hours to test/confirm...

Unsurprisingly, it works as expected.

Re: [PATCH] io_uring: remove MSG_NOSIGNAL from recvmsg

[Eric Dumazet]

On Fri, Feb 24, 2023 at 4:04 PM David Lamparter <equinox@diac24.net> wrote:
>
> MSG_NOSIGNAL is not applicable for the receiving side, SIGPIPE is
> generated when trying to write to a "broken pipe".  AF_PACKET's
> packet_recvmsg() does enforce this, giving back EINVAL when MSG_NOSIGNAL
> is set - making it unuseable in io_uring's recvmsg.
>
> Remove MSG_NOSIGNAL from io_recvmsg_prep().
>
> Signed-off-by: David Lamparter <equinox@diac24.net>
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: Jens Axboe <axboe@kernel.dk>
> ---
>
> > Sure, or perhaps David wanted to take care of this.
>
> Here you go.  But maybe give me a few hours to test/confirm...
>

Reviewed-by: Eric Dumazet <edumazet@google.com>

Thanks !

Re: [PATCH] io_uring: remove MSG_NOSIGNAL from recvmsg

[Jens Axboe]

On Fri, 24 Feb 2023 16:01:24 +0100, David Lamparter wrote:
> MSG_NOSIGNAL is not applicable for the receiving side, SIGPIPE is
> generated when trying to write to a "broken pipe".  AF_PACKET's
> packet_recvmsg() does enforce this, giving back EINVAL when MSG_NOSIGNAL
> is set - making it unuseable in io_uring's recvmsg.
> 
> Remove MSG_NOSIGNAL from io_recvmsg_prep().
> 
> [...]

Applied, thanks!

[1/1] io_uring: remove MSG_NOSIGNAL from recvmsg
      commit: 4492575406d8592b623987cb36b8234d285cfa17

Best regards,
-- 
Jens Axboe