* [PATCH net] ip: report the original address of ICMP messages
@ 2015-06-23 5:34 Julian Anastasov
2015-06-23 17:57 ` Willem de Bruijn
2015-06-24 7:49 ` David Miller
0 siblings, 2 replies; 7+ messages in thread
From: Julian Anastasov @ 2015-06-23 5:34 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Willem de Bruijn
ICMP messages can trigger ICMP and local errors. In this case
serr->port is 0 and starting from Linux 4.0 we do not return
the original target address to the error queue readers.
Add function to define which errors provide addr_offset.
With this fix my ping command is not silent anymore.
Fixes: c247f0534cc5 ("ip: fix error queue empty skb handling")
Signed-off-by: Julian Anastasov <ja@ssi.bg>
---
net/ipv4/ip_sockglue.c | 11 ++++++++++-
net/ipv6/datagram.c | 12 +++++++++++-
2 files changed, 21 insertions(+), 2 deletions(-)
diff --git a/net/ipv4/ip_sockglue.c b/net/ipv4/ip_sockglue.c
index 7cfb089..6ddde89 100644
--- a/net/ipv4/ip_sockglue.c
+++ b/net/ipv4/ip_sockglue.c
@@ -432,6 +432,15 @@ void ip_local_error(struct sock *sk, int err, __be32 daddr, __be16 port, u32 inf
kfree_skb(skb);
}
+/* For some errors we have valid addr_offset even with zero payload and
+ * zero port. Also, addr_offset should be supported if port is set.
+ */
+static inline bool ipv4_datagram_support_addr(struct sock_exterr_skb *serr)
+{
+ return serr->ee.ee_origin == SO_EE_ORIGIN_ICMP ||
+ serr->ee.ee_origin == SO_EE_ORIGIN_LOCAL || serr->port;
+}
+
/* IPv4 supports cmsg on all imcp errors and some timestamps
*
* Timestamp code paths do not initialize the fields expected by cmsg:
@@ -498,7 +507,7 @@ int ip_recv_error(struct sock *sk, struct msghdr *msg, int len, int *addr_len)
serr = SKB_EXT_ERR(skb);
- if (sin && serr->port) {
+ if (sin && ipv4_datagram_support_addr(serr)) {
sin->sin_family = AF_INET;
sin->sin_addr.s_addr = *(__be32 *)(skb_network_header(skb) +
serr->addr_offset);
diff --git a/net/ipv6/datagram.c b/net/ipv6/datagram.c
index 762a58c..62d908e 100644
--- a/net/ipv6/datagram.c
+++ b/net/ipv6/datagram.c
@@ -325,6 +325,16 @@ void ipv6_local_rxpmtu(struct sock *sk, struct flowi6 *fl6, u32 mtu)
kfree_skb(skb);
}
+/* For some errors we have valid addr_offset even with zero payload and
+ * zero port. Also, addr_offset should be supported if port is set.
+ */
+static inline bool ipv6_datagram_support_addr(struct sock_exterr_skb *serr)
+{
+ return serr->ee.ee_origin == SO_EE_ORIGIN_ICMP6 ||
+ serr->ee.ee_origin == SO_EE_ORIGIN_ICMP ||
+ serr->ee.ee_origin == SO_EE_ORIGIN_LOCAL || serr->port;
+}
+
/* IPv6 supports cmsg on all origins aside from SO_EE_ORIGIN_LOCAL.
*
* At one point, excluding local errors was a quick test to identify icmp/icmp6
@@ -389,7 +399,7 @@ int ipv6_recv_error(struct sock *sk, struct msghdr *msg, int len, int *addr_len)
serr = SKB_EXT_ERR(skb);
- if (sin && serr->port) {
+ if (sin && ipv6_datagram_support_addr(serr)) {
const unsigned char *nh = skb_network_header(skb);
sin->sin6_family = AF_INET6;
sin->sin6_flowinfo = 0;
--
1.9.3
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH net] ip: report the original address of ICMP messages
2015-06-23 5:34 [PATCH net] ip: report the original address of ICMP messages Julian Anastasov
@ 2015-06-23 17:57 ` Willem de Bruijn
2015-06-23 18:55 ` Julian Anastasov
2015-06-24 7:49 ` David Miller
1 sibling, 1 reply; 7+ messages in thread
From: Willem de Bruijn @ 2015-06-23 17:57 UTC (permalink / raw)
To: Julian Anastasov; +Cc: David Miller, Network Development
> ICMP messages can trigger ICMP and local errors. In this case
> serr->port is 0 and starting from Linux 4.0 we do not return
> the original target address to the error queue readers.
> Add function to define which errors provide addr_offset.
> With this fix my ping command is not silent anymore.
>
> Fixes: c247f0534cc5 ("ip: fix error queue empty skb handling")
> Signed-off-by: Julian Anastasov <ja@ssi.bg>
Acked-by: Willem de Bruijn <willemb@google.com>
The ping utility from iputils relies on cmsg IP_RECVERR to get
the source address on icmp errors, not on msg_name, so that
worked for me both before and after applying the patch. I used
`ping -t 2 $hostname` to trigger a TTL exceeded. Perhaps you used
a different tool or test? In any case, the change looks fine to me.
Thanks for preparing the patch.
> ---
> net/ipv4/ip_sockglue.c | 11 ++++++++++-
> net/ipv6/datagram.c | 12 +++++++++++-
> 2 files changed, 21 insertions(+), 2 deletions(-)
>
> diff --git a/net/ipv4/ip_sockglue.c b/net/ipv4/ip_sockglue.c
> index 7cfb089..6ddde89 100644
> --- a/net/ipv4/ip_sockglue.c
> +++ b/net/ipv4/ip_sockglue.c
> @@ -432,6 +432,15 @@ void ip_local_error(struct sock *sk, int err, __be32 daddr, __be16 port, u32 inf
> kfree_skb(skb);
> }
>
> +/* For some errors we have valid addr_offset even with zero payload and
> + * zero port. Also, addr_offset should be supported if port is set.
> + */
> +static inline bool ipv4_datagram_support_addr(struct sock_exterr_skb *serr)
> +{
> + return serr->ee.ee_origin == SO_EE_ORIGIN_ICMP ||
> + serr->ee.ee_origin == SO_EE_ORIGIN_LOCAL || serr->port;
> +}
> +
> /* IPv4 supports cmsg on all imcp errors and some timestamps
> *
> * Timestamp code paths do not initialize the fields expected by cmsg:
> @@ -498,7 +507,7 @@ int ip_recv_error(struct sock *sk, struct msghdr *msg, int len, int *addr_len)
>
> serr = SKB_EXT_ERR(skb);
>
> - if (sin && serr->port) {
> + if (sin && ipv4_datagram_support_addr(serr)) {
> sin->sin_family = AF_INET;
> sin->sin_addr.s_addr = *(__be32 *)(skb_network_header(skb) +
> serr->addr_offset);
> diff --git a/net/ipv6/datagram.c b/net/ipv6/datagram.c
> index 762a58c..62d908e 100644
> --- a/net/ipv6/datagram.c
> +++ b/net/ipv6/datagram.c
> @@ -325,6 +325,16 @@ void ipv6_local_rxpmtu(struct sock *sk, struct flowi6 *fl6, u32 mtu)
> kfree_skb(skb);
> }
>
> +/* For some errors we have valid addr_offset even with zero payload and
> + * zero port. Also, addr_offset should be supported if port is set.
> + */
> +static inline bool ipv6_datagram_support_addr(struct sock_exterr_skb *serr)
> +{
> + return serr->ee.ee_origin == SO_EE_ORIGIN_ICMP6 ||
> + serr->ee.ee_origin == SO_EE_ORIGIN_ICMP ||
> + serr->ee.ee_origin == SO_EE_ORIGIN_LOCAL || serr->port;
> +}
> +
> /* IPv6 supports cmsg on all origins aside from SO_EE_ORIGIN_LOCAL.
> *
> * At one point, excluding local errors was a quick test to identify icmp/icmp6
> @@ -389,7 +399,7 @@ int ipv6_recv_error(struct sock *sk, struct msghdr *msg, int len, int *addr_len)
>
> serr = SKB_EXT_ERR(skb);
>
> - if (sin && serr->port) {
> + if (sin && ipv6_datagram_support_addr(serr)) {
> const unsigned char *nh = skb_network_header(skb);
> sin->sin6_family = AF_INET6;
> sin->sin6_flowinfo = 0;
> --
> 1.9.3
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net] ip: report the original address of ICMP messages
2015-06-23 17:57 ` Willem de Bruijn
@ 2015-06-23 18:55 ` Julian Anastasov
2015-06-23 19:08 ` Willem de Bruijn
0 siblings, 1 reply; 7+ messages in thread
From: Julian Anastasov @ 2015-06-23 18:55 UTC (permalink / raw)
To: Willem de Bruijn; +Cc: David Miller, Network Development
Hello,
On Tue, 23 Jun 2015, Willem de Bruijn wrote:
> The ping utility from iputils relies on cmsg IP_RECVERR to get
> the source address on icmp errors, not on msg_name, so that
> worked for me both before and after applying the patch. I used
> `ping -t 2 $hostname` to trigger a TTL exceeded. Perhaps you used
> a different tool or test? In any case, the change looks fine to me.
> Thanks for preparing the patch.
# rpm -qf `which ping`
iputils-20140519-1.fc20.i686
It is using IP_RECVERR and may be relying only on
original address returned in msg_name from MSG_ERRQUEUE.
May be it ignores the error if original address is missing
or is different.
I didn't checked the ping source but I noticed that
it does not print anything on the read from raw socket, only
on IP_RECVERR. I noticed it when my network connection
failed. The incoming ICMP packet from my router was
DEST_UNREACH(type=3), HOST_UNREACH(code=1) but simple
REJECT rule in OUTPUT can do the same.
Regards
--
Julian Anastasov <ja@ssi.bg>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net] ip: report the original address of ICMP messages
2015-06-23 18:55 ` Julian Anastasov
@ 2015-06-23 19:08 ` Willem de Bruijn
2015-06-23 20:06 ` Julian Anastasov
0 siblings, 1 reply; 7+ messages in thread
From: Willem de Bruijn @ 2015-06-23 19:08 UTC (permalink / raw)
To: Julian Anastasov; +Cc: David Miller, Network Development
On Tue, Jun 23, 2015 at 2:55 PM, Julian Anastasov <ja@ssi.bg> wrote:
>
> Hello,
>
> On Tue, 23 Jun 2015, Willem de Bruijn wrote:
>
>> The ping utility from iputils relies on cmsg IP_RECVERR to get
>> the source address on icmp errors, not on msg_name, so that
>> worked for me both before and after applying the patch. I used
>> `ping -t 2 $hostname` to trigger a TTL exceeded. Perhaps you used
>> a different tool or test? In any case, the change looks fine to me.
>> Thanks for preparing the patch.
>
>
> # rpm -qf `which ping`
> iputils-20140519-1.fc20.i686
>
> It is using IP_RECVERR and may be relying only on
> original address returned in msg_name from MSG_ERRQUEUE.
It's not very important, in that even if ping does not use this
msg_name, another tool very well might. But from ping.c in that
srpm:
res = recvmsg(icmp_sock, &msg, MSG_ERRQUEUE|MSG_DONTWAIT);
...
e = NULL;
for (cmsg = CMSG_FIRSTHDR(&msg); cmsg; cmsg = CMSG_NXTHDR(&msg, cmsg)) {
if (cmsg->cmsg_level == SOL_IP) {
if (cmsg->cmsg_type == IP_RECVERR)
e = (struct sock_extended_err *)CMSG_DATA(cmsg);
}
}
...
} else if (e->ee_origin == SO_EE_ORIGIN_ICMP) {
struct sockaddr_in *sin = (struct sockaddr_in*)(e+1);
...
printf("From %s icmp_seq=%u ",
pr_addr(sin->sin_addr.s_addr), ntohs(icmph.un.echo.sequence));
> May be it ignores the error if original address is missing
> or is different.
>
> I didn't checked the ping source but I noticed that
> it does not print anything on the read from raw socket, only
> on IP_RECVERR. I noticed it when my network connection
> failed. The incoming ICMP packet from my router was
> DEST_UNREACH(type=3), HOST_UNREACH(code=1) but simple
> REJECT rule in OUTPUT can do the same.
>
> Regards
>
> --
> Julian Anastasov <ja@ssi.bg>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net] ip: report the original address of ICMP messages
2015-06-23 19:08 ` Willem de Bruijn
@ 2015-06-23 20:06 ` Julian Anastasov
2015-06-23 20:23 ` Willem de Bruijn
0 siblings, 1 reply; 7+ messages in thread
From: Julian Anastasov @ 2015-06-23 20:06 UTC (permalink / raw)
To: Willem de Bruijn; +Cc: David Miller, Network Development
Hello,
On Tue, 23 Jun 2015, Willem de Bruijn wrote:
> > It is using IP_RECVERR and may be relying only on
> > original address returned in msg_name from MSG_ERRQUEUE.
>
> It's not very important, in that even if ping does not use this
> msg_name, another tool very well might. But from ping.c in that
> srpm:
>
> res = recvmsg(icmp_sock, &msg, MSG_ERRQUEUE|MSG_DONTWAIT);
> ...
> e = NULL;
> for (cmsg = CMSG_FIRSTHDR(&msg); cmsg; cmsg = CMSG_NXTHDR(&msg, cmsg)) {
> if (cmsg->cmsg_level == SOL_IP) {
> if (cmsg->cmsg_type == IP_RECVERR)
> e = (struct sock_extended_err *)CMSG_DATA(cmsg);
> }
> }
> ...
> } else if (e->ee_origin == SO_EE_ORIGIN_ICMP) {
> struct sockaddr_in *sin = (struct sockaddr_in*)(e+1);
> ...
It is used, it compares the embedded original address
from error message here (msg.msg_name = (void*)&target):
target.sin_addr.s_addr != whereto.sin_addr.s_addr
and it does not match because kernel failed to fill the
msg_name buffer. In fact, kernel returns msg_namelen=0 but
it is not checked here, so may be ping.c compares random memory.
> printf("From %s icmp_seq=%u ",
> pr_addr(sin->sin_addr.s_addr), ntohs(icmph.un.echo.sequence));
What you are referring here is the offender address (sin),
the sender of the ICMP error, it is just printed...
Regards
--
Julian Anastasov <ja@ssi.bg>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net] ip: report the original address of ICMP messages
2015-06-23 20:06 ` Julian Anastasov
@ 2015-06-23 20:23 ` Willem de Bruijn
0 siblings, 0 replies; 7+ messages in thread
From: Willem de Bruijn @ 2015-06-23 20:23 UTC (permalink / raw)
To: Julian Anastasov; +Cc: David Miller, Network Development
On Tue, Jun 23, 2015 at 4:06 PM, Julian Anastasov <ja@ssi.bg> wrote:
>
> Hello,
>
> On Tue, 23 Jun 2015, Willem de Bruijn wrote:
>
>> > It is using IP_RECVERR and may be relying only on
>> > original address returned in msg_name from MSG_ERRQUEUE.
>>
>> It's not very important, in that even if ping does not use this
>> msg_name, another tool very well might. But from ping.c in that
>> srpm:
>>
>> res = recvmsg(icmp_sock, &msg, MSG_ERRQUEUE|MSG_DONTWAIT);
>> ...
>> e = NULL;
>> for (cmsg = CMSG_FIRSTHDR(&msg); cmsg; cmsg = CMSG_NXTHDR(&msg, cmsg)) {
>> if (cmsg->cmsg_level == SOL_IP) {
>> if (cmsg->cmsg_type == IP_RECVERR)
>> e = (struct sock_extended_err *)CMSG_DATA(cmsg);
>> }
>> }
>> ...
>> } else if (e->ee_origin == SO_EE_ORIGIN_ICMP) {
>> struct sockaddr_in *sin = (struct sockaddr_in*)(e+1);
>> ...
>
> It is used, it compares the embedded original address
> from error message here (msg.msg_name = (void*)&target):
>
> target.sin_addr.s_addr != whereto.sin_addr.s_addr
>
> and it does not match because kernel failed to fill the
> msg_name buffer. In fact, kernel returns msg_namelen=0 but
> it is not checked here, so may be ping.c compares random memory.
That makes sense.
>
>> printf("From %s icmp_seq=%u ",
>> pr_addr(sin->sin_addr.s_addr), ntohs(icmph.un.echo.sequence));
>
> What you are referring here is the offender address (sin),
> the sender of the ICMP error, it is just printed...
>
>
> Regards
>
> --
> Julian Anastasov <ja@ssi.bg>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net] ip: report the original address of ICMP messages
2015-06-23 5:34 [PATCH net] ip: report the original address of ICMP messages Julian Anastasov
2015-06-23 17:57 ` Willem de Bruijn
@ 2015-06-24 7:49 ` David Miller
1 sibling, 0 replies; 7+ messages in thread
From: David Miller @ 2015-06-24 7:49 UTC (permalink / raw)
To: ja; +Cc: netdev, willemb
From: Julian Anastasov <ja@ssi.bg>
Date: Tue, 23 Jun 2015 08:34:39 +0300
> ICMP messages can trigger ICMP and local errors. In this case
> serr->port is 0 and starting from Linux 4.0 we do not return
> the original target address to the error queue readers.
> Add function to define which errors provide addr_offset.
> With this fix my ping command is not silent anymore.
>
> Fixes: c247f0534cc5 ("ip: fix error queue empty skb handling")
> Signed-off-by: Julian Anastasov <ja@ssi.bg>
Applied and queued up for -stable, thanks.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2015-06-24 7:37 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-06-23 5:34 [PATCH net] ip: report the original address of ICMP messages Julian Anastasov
2015-06-23 17:57 ` Willem de Bruijn
2015-06-23 18:55 ` Julian Anastasov
2015-06-23 19:08 ` Willem de Bruijn
2015-06-23 20:06 ` Julian Anastasov
2015-06-23 20:23 ` Willem de Bruijn
2015-06-24 7:49 ` David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).