* [PATCH mptcp-next] mptcp: ignore unsupported msg flags
@ 2021-04-14 15:23 Paolo Abeni
2021-04-14 15:51 ` Florian Westphal
0 siblings, 1 reply; 6+ messages in thread
From: Paolo Abeni @ 2021-04-14 15:23 UTC (permalink / raw
To: mptcp
Currently mptcp_sendmsg()/mptcp_recvmsg() fail with EOPNOTSUPP
if the user-space provides some unsupported flag. That is
unexpected and may foul existing applications migrated to MPTCP,
which expect a different behavior.
Change the mentioned function to silently ignore the unsupported
flags.
Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/162
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
net/mptcp/protocol.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 2d895c3c8746..bdec946b9b04 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -1585,8 +1585,8 @@ static int mptcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
int ret = 0;
long timeo;
- if (msg->msg_flags & ~(MSG_MORE | MSG_DONTWAIT | MSG_NOSIGNAL))
- return -EOPNOTSUPP;
+ /* ignore unsupported flags */
+ msg->msg_flags &= MSG_MORE | MSG_DONTWAIT | MSG_NOSIGNAL;
mptcp_lock_sock(sk, __mptcp_wmem_reserve(sk, min_t(size_t, 1 << 20, len)));
@@ -1916,8 +1916,8 @@ static int mptcp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
int target;
long timeo;
- if (msg->msg_flags & ~(MSG_WAITALL | MSG_DONTWAIT))
- return -EOPNOTSUPP;
+ /* ignore unsupported flags */
+ msg->msg_flags &= MSG_WAITALL | MSG_DONTWAIT;
mptcp_lock_sock(sk, __mptcp_splice_receive_queue(sk));
if (unlikely(sk->sk_state == TCP_LISTEN)) {
--
2.26.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH mptcp-next] mptcp: ignore unsupported msg flags
2021-04-14 15:23 [PATCH mptcp-next] mptcp: ignore unsupported msg flags Paolo Abeni
@ 2021-04-14 15:51 ` Florian Westphal
2021-04-14 16:07 ` Paolo Abeni
0 siblings, 1 reply; 6+ messages in thread
From: Florian Westphal @ 2021-04-14 15:51 UTC (permalink / raw
To: Paolo Abeni; +Cc: mptcp
Paolo Abeni <pabeni@redhat.com> wrote:
> Currently mptcp_sendmsg()/mptcp_recvmsg() fail with EOPNOTSUPP
> if the user-space provides some unsupported flag. That is
> unexpected and may foul existing applications migrated to MPTCP,
> which expect a different behavior.
>
> Change the mentioned function to silently ignore the unsupported
> flags.
>
> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/162
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
> net/mptcp/protocol.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 2d895c3c8746..bdec946b9b04 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -1585,8 +1585,8 @@ static int mptcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
> int ret = 0;
> long timeo;
>
> - if (msg->msg_flags & ~(MSG_MORE | MSG_DONTWAIT | MSG_NOSIGNAL))
> - return -EOPNOTSUPP;
> + /* ignore unsupported flags */
> + msg->msg_flags &= MSG_MORE | MSG_DONTWAIT | MSG_NOSIGNAL;
Ignoring MSG_MORE is fine, but DONTWAIT and NOSIGNAL?
I'd prefer hard errors for those until mptcp does the right thing for
them.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH mptcp-next] mptcp: ignore unsupported msg flags
2021-04-14 15:51 ` Florian Westphal
@ 2021-04-14 16:07 ` Paolo Abeni
2021-04-15 8:27 ` Florian Westphal
0 siblings, 1 reply; 6+ messages in thread
From: Paolo Abeni @ 2021-04-14 16:07 UTC (permalink / raw
To: Florian Westphal; +Cc: mptcp
On Wed, 2021-04-14 at 17:51 +0200, Florian Westphal wrote:
> Paolo Abeni <pabeni@redhat.com> wrote:
> > Currently mptcp_sendmsg()/mptcp_recvmsg() fail with EOPNOTSUPP
> > if the user-space provides some unsupported flag. That is
> > unexpected and may foul existing applications migrated to MPTCP,
> > which expect a different behavior.
> >
> > Change the mentioned function to silently ignore the unsupported
> > flags.
> >
> > Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/162
> > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> > ---
> > net/mptcp/protocol.c | 8 ++++----
> > 1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> > index 2d895c3c8746..bdec946b9b04 100644
> > --- a/net/mptcp/protocol.c
> > +++ b/net/mptcp/protocol.c
> > @@ -1585,8 +1585,8 @@ static int mptcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
> > int ret = 0;
> > long timeo;
> >
> > - if (msg->msg_flags & ~(MSG_MORE | MSG_DONTWAIT | MSG_NOSIGNAL))
> > - return -EOPNOTSUPP;
> > + /* ignore unsupported flags */
> > + msg->msg_flags &= MSG_MORE | MSG_DONTWAIT | MSG_NOSIGNAL;
>
> Ignoring MSG_MORE is fine, but DONTWAIT and NOSIGNAL?
The intent of this patch is ignoring the unsupported flags. For sendmsg
that is all of them except MSG_MORE, MSG_DONTWAIT, MSG_NOSIGNAL:
MSG_OOB
MSG_PEEK
MSG_DONTROUTE
MSG_TRYHARD
MSG_CTRUNC
MSG_PROBE
MSG_TRUNC
MSG_EOR
MSG_FIN
MSG_SYN
MSG_CONFIRM
MSG_RST
MSG_ERRQUEUE
MSG_WAITFORONE
MSG_SENDPAGE_NOPOLICY
MSG_SENDPAGE_NOTLAST
MSG_BATCH
MSG_EOF
MSG_NO_SHARED_FRAGS
MSG_SENDPAGE_DECRYPTED
MSG_ZEROCOPY
MSG_FASTOPEN
MSG_CMSG_CLOEXEC
> I'd prefer hard errors for those until mptcp does the right thing for
> them.
should we define a 'bail on use' mask with a subset of the above? The
candidates I see are:
MSG_TRUNC
MSG_PEEK
MSG_OOB
Thanks,
Paolo
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH mptcp-next] mptcp: ignore unsupported msg flags
2021-04-14 16:07 ` Paolo Abeni
@ 2021-04-15 8:27 ` Florian Westphal
2021-04-15 9:33 ` Paolo Abeni
0 siblings, 1 reply; 6+ messages in thread
From: Florian Westphal @ 2021-04-15 8:27 UTC (permalink / raw
To: Paolo Abeni; +Cc: Florian Westphal, mptcp
Paolo Abeni <pabeni@redhat.com> wrote:
> The intent of this patch is ignoring the unsupported flags. For sendmsg
> that is all of them except MSG_MORE, MSG_DONTWAIT, MSG_NOSIGNAL:
>
> MSG_OOB
> MSG_PEEK
> MSG_DONTROUTE
> MSG_TRYHARD
> MSG_CTRUNC
> MSG_PROBE
> MSG_TRUNC
> MSG_EOR
> MSG_FIN
> MSG_SYN
> MSG_CONFIRM
> MSG_RST
> MSG_ERRQUEUE
> MSG_WAITFORONE
> MSG_SENDPAGE_NOPOLICY
> MSG_SENDPAGE_NOTLAST
> MSG_BATCH
> MSG_EOF
> MSG_NO_SHARED_FRAGS
> MSG_SENDPAGE_DECRYPTED
> MSG_ZEROCOPY
> MSG_FASTOPEN
> MSG_CMSG_CLOEXEC
Looks like TCP ignores all of them except fastopen:
sendto(5, "foo\n", 4, 0, NULL, 0) = 4
sendto(5, "foo\n", 4, MSG_OOB, NULL, 0) = 4
sendto(5, "foo\n", 4, MSG_PEEK, NULL, 0) = 4
sendto(5, "foo\n", 4, MSG_DONTROUTE, NULL, 0) = 4
sendto(5, "foo\n", 4, MSG_CTRUNC, NULL, 0) = 4
sendto(5, "foo\n", 4, MSG_PROBE, NULL, 0) = 4
sendto(5, "foo\n", 4, MSG_TRUNC, NULL, 0) = 4
sendto(5, "foo\n", 4, MSG_DONTWAIT, NULL, 0) = 4
sendto(5, "foo\n", 4, MSG_EOR, NULL, 0) = 4
sendto(5, "foo\n", 4, MSG_WAITALL, NULL, 0) = 4
sendto(5, "foo\n", 4, MSG_FIN, NULL, 0) = 4
sendto(5, "foo\n", 4, MSG_SYN, NULL, 0) = 4
sendto(5, "foo\n", 4, MSG_CONFIRM, NULL, 0) = 4
sendto(5, "foo\n", 4, MSG_RST, NULL, 0) = 4
sendto(5, "foo\n", 4, MSG_ERRQUEUE, NULL, 0) = 4
sendto(5, "foo\n", 4, MSG_NOSIGNAL, NULL, 0) = 4
sendto(5, "foo\n", 4, MSG_MORE, NULL, 0) = 4
sendto(5, "foo\n", 4, MSG_WAITFORONE, NULL, 0) = 4
sendto(5, "foo\n", 4, MSG_SENDPAGE_NOTLAST, NULL, 0) = 4
sendto(5, "foo\n", 4, MSG_BATCH, NULL, 0) = 4
sendto(5, "foo\n", 4, MSG_NO_SHARED_FRAGS, NULL, 0) = 4
sendto(5, "foo\n", 4, 0x100000 /* MSG_??? */, NULL, 0) = 4
sendto(5, "foo\n", 4, 0x200000 /* MSG_??? */, NULL, 0) = 4
sendto(5, "foo\n", 4, 0x400000 /* MSG_??? */, NULL, 0) = 4
sendto(5, "foo\n", 4, 0x800000 /* MSG_??? */, NULL, 0) = 4
sendto(5, "foo\n", 4, 0x1000000 /* MSG_??? */, NULL, 0) = 4
sendto(5, "foo\n", 4, 0x2000000 /* MSG_??? */, NULL, 0) = 4
sendto(5, "foo\n", 4, MSG_ZEROCOPY, NULL, 0) = 4
sendto(5, "foo\n", 4, 0x8000000 /* MSG_??? */, NULL, 0) = 4
sendto(5, "foo\n", 4, 0x10000000 /* MSG_??? */, NULL, 0) = 4
sendto(5, "foo\n", 4, MSG_FASTOPEN, NULL, 0) = -1 EISCONN (Transport
endpoint is already connected)
sendto(5, "foo\n", 4, MSG_CMSG_CLOEXEC, NULL, 0) = 4
sendto(5, "foo\n", 4, MSG_CMSG_COMPAT, NULL, 0) = 4
(fd 4 is a established tcp socket, after successful connect()).
> should we define a 'bail on use' mask with a subset of the above? The
> candidates I see are:
> MSG_TRUNC
> MSG_PEEK
> MSG_OOB
Looks like we only need to handle FASTOPEN.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH mptcp-next] mptcp: ignore unsupported msg flags
2021-04-15 8:27 ` Florian Westphal
@ 2021-04-15 9:33 ` Paolo Abeni
2021-04-15 9:44 ` Florian Westphal
0 siblings, 1 reply; 6+ messages in thread
From: Paolo Abeni @ 2021-04-15 9:33 UTC (permalink / raw
To: Florian Westphal; +Cc: mptcp
On Thu, 2021-04-15 at 10:27 +0200, Florian Westphal wrote:
> Paolo Abeni <pabeni@redhat.com> wrote:
> > The intent of this patch is ignoring the unsupported flags. For sendmsg
> > that is all of them except MSG_MORE, MSG_DONTWAIT, MSG_NOSIGNAL:
> >
> > MSG_OOB
> > MSG_PEEK
> > MSG_DONTROUTE
> > MSG_TRYHARD
> > MSG_CTRUNC
> > MSG_PROBE
> > MSG_TRUNC
> > MSG_EOR
> > MSG_FIN
> > MSG_SYN
> > MSG_CONFIRM
> > MSG_RST
> > MSG_ERRQUEUE
> > MSG_WAITFORONE
> > MSG_SENDPAGE_NOPOLICY
> > MSG_SENDPAGE_NOTLAST
> > MSG_BATCH
> > MSG_EOF
> > MSG_NO_SHARED_FRAGS
> > MSG_SENDPAGE_DECRYPTED
> > MSG_ZEROCOPY
> > MSG_FASTOPEN
> > MSG_CMSG_CLOEXEC
>
> Looks like TCP ignores all of them except fastopen:
>
> sendto(5, "foo\n", 4, 0, NULL, 0) = 4
> sendto(5, "foo\n", 4, MSG_OOB, NULL, 0) = 4
> sendto(5, "foo\n", 4, MSG_PEEK, NULL, 0) = 4
> sendto(5, "foo\n", 4, MSG_DONTROUTE, NULL, 0) = 4
> sendto(5, "foo\n", 4, MSG_CTRUNC, NULL, 0) = 4
> sendto(5, "foo\n", 4, MSG_PROBE, NULL, 0) = 4
> sendto(5, "foo\n", 4, MSG_TRUNC, NULL, 0) = 4
> sendto(5, "foo\n", 4, MSG_DONTWAIT, NULL, 0) = 4
> sendto(5, "foo\n", 4, MSG_EOR, NULL, 0) = 4
> sendto(5, "foo\n", 4, MSG_WAITALL, NULL, 0) = 4
> sendto(5, "foo\n", 4, MSG_FIN, NULL, 0) = 4
> sendto(5, "foo\n", 4, MSG_SYN, NULL, 0) = 4
> sendto(5, "foo\n", 4, MSG_CONFIRM, NULL, 0) = 4
> sendto(5, "foo\n", 4, MSG_RST, NULL, 0) = 4
> sendto(5, "foo\n", 4, MSG_ERRQUEUE, NULL, 0) = 4
> sendto(5, "foo\n", 4, MSG_NOSIGNAL, NULL, 0) = 4
> sendto(5, "foo\n", 4, MSG_MORE, NULL, 0) = 4
> sendto(5, "foo\n", 4, MSG_WAITFORONE, NULL, 0) = 4
> sendto(5, "foo\n", 4, MSG_SENDPAGE_NOTLAST, NULL, 0) = 4
> sendto(5, "foo\n", 4, MSG_BATCH, NULL, 0) = 4
> sendto(5, "foo\n", 4, MSG_NO_SHARED_FRAGS, NULL, 0) = 4
> sendto(5, "foo\n", 4, 0x100000 /* MSG_??? */, NULL, 0) = 4
> sendto(5, "foo\n", 4, 0x200000 /* MSG_??? */, NULL, 0) = 4
> sendto(5, "foo\n", 4, 0x400000 /* MSG_??? */, NULL, 0) = 4
> sendto(5, "foo\n", 4, 0x800000 /* MSG_??? */, NULL, 0) = 4
> sendto(5, "foo\n", 4, 0x1000000 /* MSG_??? */, NULL, 0) = 4
> sendto(5, "foo\n", 4, 0x2000000 /* MSG_??? */, NULL, 0) = 4
> sendto(5, "foo\n", 4, MSG_ZEROCOPY, NULL, 0) = 4
> sendto(5, "foo\n", 4, 0x8000000 /* MSG_??? */, NULL, 0) = 4
> sendto(5, "foo\n", 4, 0x10000000 /* MSG_??? */, NULL, 0) = 4
> sendto(5, "foo\n", 4, MSG_FASTOPEN, NULL, 0) = -1 EISCONN (Transport
> endpoint is already connected)
> sendto(5, "foo\n", 4, MSG_CMSG_CLOEXEC, NULL, 0) = 4
> sendto(5, "foo\n", 4, MSG_CMSG_COMPAT, NULL, 0) = 4
>
> (fd 4 is a established tcp socket, after successful connect()).
Thank you for the estensive testing!
Looking at the code even MSG_DONTWAIT, MSG_EOR, MSG_MORE, MSG_OOB
MSG_ZEROCOPY has some effect on TCP.
MSG_ZEROCOPY needs to be enabled via setsockopt()
Currently we implement MSG_DONTWAIT, silently ignore MSG_MORE and we
bail on MSG_OOB, MSG_EOR, MSG_ZEROCOPY
> > should we define a 'bail on use' mask with a subset of the above? The
> > candidates I see are:
>
> > MSG_TRUNC
> > MSG_PEEK
> > MSG_OOB
>
> Looks like we only need to handle FASTOPEN.
LGTM, for sendmsg() sake: ignoring MSG_OOB and MSG_EOR does not look
too evil to me ;)
Than there is recvmsg()...
With a quick code inspections looks like TCP support:
MSG_PEEK, MSG_TRUNC, MSG_OOB, MSG_WAITALL, MSG_DONTWAIT, MSG_DONTWAIT,
MSG_ERRQUEUE
MSG_ERRQUEUE requires to be enabled via IP_RECVERR()
We currently implement MSG_WAITALL and MSG_DONTWAIT, soon MSG_PEEK and
we bail on all the others.
Ignoring MSG_ERRQUEUE or MSG_TRUNC may silently break user-space
application.
I think we could provide a dummy impl for MSG_ERRQUEUE: always
returning empty msg, which will be consistent with the lack of
setsockopt support. We could even call into inet_recv_error(), as the
relevant queue will always be empty, and could possibly simplify a
complete implementation someday.
I also think we should bail on MSG_TRUNC, and silently ignore all the
others.
WDYT?
Thanks!
Paolo
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH mptcp-next] mptcp: ignore unsupported msg flags
2021-04-15 9:33 ` Paolo Abeni
@ 2021-04-15 9:44 ` Florian Westphal
0 siblings, 0 replies; 6+ messages in thread
From: Florian Westphal @ 2021-04-15 9:44 UTC (permalink / raw
To: Paolo Abeni; +Cc: Florian Westphal, mptcp
Paolo Abeni <pabeni@redhat.com> wrote:
> Looking at the code even MSG_DONTWAIT, MSG_EOR, MSG_MORE, MSG_OOB
> MSG_ZEROCOPY has some effect on TCP.
We handle DONTWAIT in mptcp, and the other ones, afaics, do not result
in a user-visible change (i.e., if we would not support DONTWAIT, we
would cause userspace do block in send() even if userspace specifically
asked for us not to, so that would be a bug).
But for the others I think ignoring is fine.
> MSG_ZEROCOPY needs to be enabled via setsockopt()
>
> Currently we implement MSG_DONTWAIT, silently ignore MSG_MORE and we
> bail on MSG_OOB, MSG_EOR, MSG_ZEROCOPY
>
> > > should we define a 'bail on use' mask with a subset of the above? The
> > > candidates I see are:
> >
> > > MSG_TRUNC
> > > MSG_PEEK
> > > MSG_OOB
> >
> > Looks like we only need to handle FASTOPEN.
>
> LGTM, for sendmsg() sake: ignoring MSG_OOB and MSG_EOR does not look
> too evil to me ;)
Agree.
> Than there is recvmsg()...
>
> With a quick code inspections looks like TCP support:
> MSG_PEEK, MSG_TRUNC, MSG_OOB, MSG_WAITALL, MSG_DONTWAIT, MSG_DONTWAIT,
> MSG_ERRQUEUE
>
> MSG_ERRQUEUE requires to be enabled via IP_RECVERR()
>
> We currently implement MSG_WAITALL and MSG_DONTWAIT, soon MSG_PEEK and
> we bail on all the others.
>
> Ignoring MSG_ERRQUEUE or MSG_TRUNC may silently break user-space
> application.
Indeed.
> I think we could provide a dummy impl for MSG_ERRQUEUE: always
> returning empty msg, which will be consistent with the lack of
> setsockopt support. We could even call into inet_recv_error(), as the
> relevant queue will always be empty, and could possibly simplify a
> complete implementation someday.
>
> I also think we should bail on MSG_TRUNC, and silently ignore all the
> others.
>
> WDYT?
Sounds good. MSG_TRUNC support should be easy to add as well.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-04-15 9:44 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-04-14 15:23 [PATCH mptcp-next] mptcp: ignore unsupported msg flags Paolo Abeni
2021-04-14 15:51 ` Florian Westphal
2021-04-14 16:07 ` Paolo Abeni
2021-04-15 8:27 ` Florian Westphal
2021-04-15 9:33 ` Paolo Abeni
2021-04-15 9:44 ` Florian Westphal
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.