| Message ID | fb3746774c97c3042beb7d76cf9f23e57d7f85e1.1618413785.git.pabeni@redhat.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [mptcp-next] mptcp: ignore unsupported msg flags | expand |
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.
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
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.
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
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.
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)) {
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(-)