* Re: [PATCH net-next] sock: ignore TIMESTAMP, RXQ_OVFL, WIFI_STATUS in sock_cmsg_send
2016-05-13 4:47 [PATCH net-next] sock: ignore TIMESTAMP, RXQ_OVFL, WIFI_STATUS in sock_cmsg_send Soheil Hassas Yeganeh
@ 2016-05-13 6:03 ` David Miller
2016-05-13 12:14 ` Soheil Hassas Yeganeh
2016-05-13 12:46 ` Sergei Shtylyov
2016-05-13 13:14 ` [PATCH net-next] sock: propagate __sock_cmsg_send() error Eric Dumazet
2 siblings, 1 reply; 7+ messages in thread
From: David Miller @ 2016-05-13 6:03 UTC (permalink / raw)
To: soheil.kdev; +Cc: netdev, edumazet, maze, willemb, soheil
From: Soheil Hassas Yeganeh <soheil.kdev@gmail.com>
Date: Fri, 13 May 2016 00:47:10 -0400
> From: Soheil Hassas Yeganeh <soheil@google.com>
>
> SO_TIMESTAMP(NS), RXQ_OVFL, and WIFI_STATUS can be returned as
> receive-side control messages from recvmsg(). Although invalid,
> some applications may reflect those receive-side control messages
> back to sendmsg(). Since socket-level control messages were being
> ignored in ipv4 and ipv6, such applications would not get an error.
>
> 24025c4 (ipv4: process socket-level control messages in IPv4) and
> ad1e46 (ipv6: process socket-level control messages in IPv6) add
> support for socket-level control messages in ipv4 and ipv6 on
> sendmsg(). This results in getting -EINVAL, if the application
> passes in a message with SO_WIFI_STATUS, SO_RXQ_OVFL, SO_TIMESTAMP
> and/or SO_TIMESTAMPNS that might have been received in recvmsg().
>
> Ignore SO_WIFI_STATUS, SO_TIMESTAMP(NS), and SO_RXQ_OVFL when
> processing socket-level control messages in send-side to remain
> backward compatible.
This patch is missing a proper Signed-Off-By: tag.
But I think this change is wrong. Just because we silently accepted
garbage in the past doesn't mean more strict checking is invalid.
Applications blindly echoing control messages from recvmsg to sendmsg
must be fixed.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-next] sock: ignore TIMESTAMP, RXQ_OVFL, WIFI_STATUS in sock_cmsg_send
2016-05-13 6:03 ` David Miller
@ 2016-05-13 12:14 ` Soheil Hassas Yeganeh
0 siblings, 0 replies; 7+ messages in thread
From: Soheil Hassas Yeganeh @ 2016-05-13 12:14 UTC (permalink / raw)
To: David Miller
Cc: Soheil Hassas Yeganeh, netdev, Eric Dumazet,
Maciej Żenczykowski, Willem de Bruijn
On Fri, May 13, 2016 at 2:03 AM, David Miller <davem@davemloft.net> wrote:
> From: Soheil Hassas Yeganeh <soheil.kdev@gmail.com>
> Date: Fri, 13 May 2016 00:47:10 -0400
>
>> From: Soheil Hassas Yeganeh <soheil@google.com>
>>
>> SO_TIMESTAMP(NS), RXQ_OVFL, and WIFI_STATUS can be returned as
>> receive-side control messages from recvmsg(). Although invalid,
>> some applications may reflect those receive-side control messages
>> back to sendmsg(). Since socket-level control messages were being
>> ignored in ipv4 and ipv6, such applications would not get an error.
>>
>> 24025c4 (ipv4: process socket-level control messages in IPv4) and
>> ad1e46 (ipv6: process socket-level control messages in IPv6) add
>> support for socket-level control messages in ipv4 and ipv6 on
>> sendmsg(). This results in getting -EINVAL, if the application
>> passes in a message with SO_WIFI_STATUS, SO_RXQ_OVFL, SO_TIMESTAMP
>> and/or SO_TIMESTAMPNS that might have been received in recvmsg().
>>
>> Ignore SO_WIFI_STATUS, SO_TIMESTAMP(NS), and SO_RXQ_OVFL when
>> processing socket-level control messages in send-side to remain
>> backward compatible.
>
> This patch is missing a proper Signed-Off-By: tag.
Oops, sorry. :(
> But I think this change is wrong. Just because we silently accepted
> garbage in the past doesn't mean more strict checking is invalid.
>
> Applications blindly echoing control messages from recvmsg to sendmsg
> must be fixed.
Agreed, by this patch, I just wanted to note such potential issues and
make sure I haven't broken any user-space application. Thanks David!
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-next] sock: ignore TIMESTAMP, RXQ_OVFL, WIFI_STATUS in sock_cmsg_send
2016-05-13 4:47 [PATCH net-next] sock: ignore TIMESTAMP, RXQ_OVFL, WIFI_STATUS in sock_cmsg_send Soheil Hassas Yeganeh
2016-05-13 6:03 ` David Miller
@ 2016-05-13 12:46 ` Sergei Shtylyov
2016-05-13 13:14 ` [PATCH net-next] sock: propagate __sock_cmsg_send() error Eric Dumazet
2 siblings, 0 replies; 7+ messages in thread
From: Sergei Shtylyov @ 2016-05-13 12:46 UTC (permalink / raw)
To: Soheil Hassas Yeganeh, davem, netdev
Cc: edumazet, maze, willemb, Soheil Hassas Yeganeh
Hello.
On 5/13/2016 7:47 AM, Soheil Hassas Yeganeh wrote:
> From: Soheil Hassas Yeganeh <soheil@google.com>
>
> SO_TIMESTAMP(NS), RXQ_OVFL, and WIFI_STATUS can be returned as
> receive-side control messages from recvmsg(). Although invalid,
> some applications may reflect those receive-side control messages
> back to sendmsg(). Since socket-level control messages were being
> ignored in ipv4 and ipv6, such applications would not get an error.
>
> 24025c4 (ipv4: process socket-level control messages in IPv4) and
> ad1e46 (ipv6: process socket-level control messages in IPv6) add
scripts/checkpatch.pl now enforces certain commit citing style, yours
doesn't quite match -- SHA1 should be at least 12 digits and the summary
enclosed in ("").
> support for socket-level control messages in ipv4 and ipv6 on
> sendmsg(). This results in getting -EINVAL, if the application
> passes in a message with SO_WIFI_STATUS, SO_RXQ_OVFL, SO_TIMESTAMP
> and/or SO_TIMESTAMPNS that might have been received in recvmsg().
>
> Ignore SO_WIFI_STATUS, SO_TIMESTAMP(NS), and SO_RXQ_OVFL when
> processing socket-level control messages in send-side to remain
> backward compatible.
> ---
> net/core/sock.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/net/core/sock.c b/net/core/sock.c
> index 08bf97e..1e0bcd0 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -1938,6 +1938,12 @@ int __sock_cmsg_send(struct sock *sk, struct msghdr *msg, struct cmsghdr *cmsg,
> sockc->tsflags &= ~SOF_TIMESTAMPING_TX_RECORD_MASK;
> sockc->tsflags |= tsflags;
> break;
> + /* Ignore the following types on send to remain backward compatible. */
> + case SO_RXQ_OVFL: /* Fall through */
> + case SO_TIMESTAMP: /* Fall through */
> + case SO_TIMESTAMPNS: /* Fall through */
^^^^^^^^^^^^^^^^^^
There's no need for such comments here.
> + case SO_WIFI_STATUS:
> + break;
> default:
> return -EINVAL;
> }
MBR, Sergei
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH net-next] sock: propagate __sock_cmsg_send() error
2016-05-13 4:47 [PATCH net-next] sock: ignore TIMESTAMP, RXQ_OVFL, WIFI_STATUS in sock_cmsg_send Soheil Hassas Yeganeh
2016-05-13 6:03 ` David Miller
2016-05-13 12:46 ` Sergei Shtylyov
@ 2016-05-13 13:14 ` Eric Dumazet
2016-05-13 13:29 ` Soheil Hassas Yeganeh
2016-05-16 17:46 ` David Miller
2 siblings, 2 replies; 7+ messages in thread
From: Eric Dumazet @ 2016-05-13 13:14 UTC (permalink / raw)
To: Soheil Hassas Yeganeh
Cc: davem, netdev, edumazet, maze, willemb, Soheil Hassas Yeganeh
From: Eric Dumazet <edumazet@google.com>
__sock_cmsg_send() might return different error codes, not only -EINVAL.
Fixes: 24025c465f77 ("ipv4: process socket-level control messages in IPv4")
Fixes: ad1e46a83716 ("ipv6: process socket-level control messages in IPv6")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Soheil Hassas Yeganeh <soheil@google.com>
---
net/ipv4/ip_sockglue.c | 5 +++--
net/ipv6/datagram.c | 5 +++--
2 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/net/ipv4/ip_sockglue.c b/net/ipv4/ip_sockglue.c
index 5805762d7fc79a702f1d67e802992b822f66f000..71a52f4d4cffba2db9353f43dc817689bf4fab10 100644
--- a/net/ipv4/ip_sockglue.c
+++ b/net/ipv4/ip_sockglue.c
@@ -247,8 +247,9 @@ int ip_cmsg_send(struct sock *sk, struct msghdr *msg, struct ipcm_cookie *ipc,
}
#endif
if (cmsg->cmsg_level == SOL_SOCKET) {
- if (__sock_cmsg_send(sk, msg, cmsg, &ipc->sockc))
- return -EINVAL;
+ err = __sock_cmsg_send(sk, msg, cmsg, &ipc->sockc);
+ if (err)
+ return err;
continue;
}
diff --git a/net/ipv6/datagram.c b/net/ipv6/datagram.c
index 00d0c2903173a96571983216f2839a93059cad22..37874e2f30edf98f31e2a5097761143d507d5b95 100644
--- a/net/ipv6/datagram.c
+++ b/net/ipv6/datagram.c
@@ -746,8 +746,9 @@ int ip6_datagram_send_ctl(struct net *net, struct sock *sk,
}
if (cmsg->cmsg_level == SOL_SOCKET) {
- if (__sock_cmsg_send(sk, msg, cmsg, sockc))
- return -EINVAL;
+ err = __sock_cmsg_send(sk, msg, cmsg, sockc);
+ if (err)
+ return err;
continue;
}
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH net-next] sock: propagate __sock_cmsg_send() error
2016-05-13 13:14 ` [PATCH net-next] sock: propagate __sock_cmsg_send() error Eric Dumazet
@ 2016-05-13 13:29 ` Soheil Hassas Yeganeh
2016-05-16 17:46 ` David Miller
1 sibling, 0 replies; 7+ messages in thread
From: Soheil Hassas Yeganeh @ 2016-05-13 13:29 UTC (permalink / raw)
To: Eric Dumazet
Cc: Soheil Hassas Yeganeh, David Miller, netdev, Eric Dumazet,
Maciej Żenczykowski, Willem de Bruijn
On Fri, May 13, 2016 at 9:14 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> __sock_cmsg_send() might return different error codes, not only -EINVAL.
>
> Fixes: 24025c465f77 ("ipv4: process socket-level control messages in IPv4")
> Fixes: ad1e46a83716 ("ipv6: process socket-level control messages in IPv6")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Soheil Hassas Yeganeh <soheil@google.com>
Acked-by: Soheil Hassas Yeganeh <soheil@google.com>
> ---
> net/ipv4/ip_sockglue.c | 5 +++--
> net/ipv6/datagram.c | 5 +++--
> 2 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/net/ipv4/ip_sockglue.c b/net/ipv4/ip_sockglue.c
> index 5805762d7fc79a702f1d67e802992b822f66f000..71a52f4d4cffba2db9353f43dc817689bf4fab10 100644
> --- a/net/ipv4/ip_sockglue.c
> +++ b/net/ipv4/ip_sockglue.c
> @@ -247,8 +247,9 @@ int ip_cmsg_send(struct sock *sk, struct msghdr *msg, struct ipcm_cookie *ipc,
> }
> #endif
> if (cmsg->cmsg_level == SOL_SOCKET) {
> - if (__sock_cmsg_send(sk, msg, cmsg, &ipc->sockc))
> - return -EINVAL;
> + err = __sock_cmsg_send(sk, msg, cmsg, &ipc->sockc);
> + if (err)
> + return err;
> continue;
> }
>
> diff --git a/net/ipv6/datagram.c b/net/ipv6/datagram.c
> index 00d0c2903173a96571983216f2839a93059cad22..37874e2f30edf98f31e2a5097761143d507d5b95 100644
> --- a/net/ipv6/datagram.c
> +++ b/net/ipv6/datagram.c
> @@ -746,8 +746,9 @@ int ip6_datagram_send_ctl(struct net *net, struct sock *sk,
> }
>
> if (cmsg->cmsg_level == SOL_SOCKET) {
> - if (__sock_cmsg_send(sk, msg, cmsg, sockc))
> - return -EINVAL;
> + err = __sock_cmsg_send(sk, msg, cmsg, sockc);
> + if (err)
> + return err;
> continue;
> }
>
>
>
Thanks Eric for the fixes.
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH net-next] sock: propagate __sock_cmsg_send() error
2016-05-13 13:14 ` [PATCH net-next] sock: propagate __sock_cmsg_send() error Eric Dumazet
2016-05-13 13:29 ` Soheil Hassas Yeganeh
@ 2016-05-16 17:46 ` David Miller
1 sibling, 0 replies; 7+ messages in thread
From: David Miller @ 2016-05-16 17:46 UTC (permalink / raw)
To: eric.dumazet; +Cc: soheil.kdev, netdev, edumazet, maze, willemb, soheil
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Fri, 13 May 2016 06:14:37 -0700
> From: Eric Dumazet <edumazet@google.com>
>
> __sock_cmsg_send() might return different error codes, not only -EINVAL.
>
> Fixes: 24025c465f77 ("ipv4: process socket-level control messages in IPv4")
> Fixes: ad1e46a83716 ("ipv6: process socket-level control messages in IPv6")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Soheil Hassas Yeganeh <soheil@google.com>
Applied.
^ permalink raw reply [flat|nested] 7+ messages in thread