net: fix the counter ICMP_MIB_INERRORS/ICMP6_MIB_INERRORS

PATCH Superseded Duan Jiong 3 participants 10 messages Jul 29, 2014 – Aug 1, 2014

Subsystems (6)

Networking Network drivers IPv4/IPv6 TCP +2 more subsystems

lore

Share & tools

lore

Duan Jiong

author

2014-07-29 08:20 UTC #0 lore raw

PATCH 1/1 net: fix the counter ICMP_MIB_INERRORS/ICMP6_MIB_INERRORS

When dealing with ICMPv[46] Error Message, function icmp_socket_deliver() and icmpv6_notify() do some valid checks on packet's length, but then some protocols check packet's length redaudantly. So remove those duplicated statements, and increase counter ICMP_MIB_INERRORS/ICMP6_MIB_INERRORS in function icmp_socket_deliver() and icmpv6_notify() respectively.

In addition, add missed counter in udp6/udplite6 when socket is NULL.

Signed-off-by Duan Jiong <[email protected]>

---
 net/ipv4/icmp.c     |  4 +++-
 net/ipv4/tcp_ipv4.c |  5 -----
 net/ipv6/icmp.c     | 11 ++++++++---
 net/ipv6/udp.c      |  8 ++++++--
 net/sctp/input.c    |  5 -----
 5 files changed, 17 insertions(+), 16 deletions(-)

net/ipv4/icmp.c +3 -1

index 42b7bcf..092400e 100644
@@ -663,8 +663,10 @@ static void icmp_socket_deliver(struct sk_buff *skb, u32 info)
 	/* Checkin full IP header plus 8 bytes of protocol to
 	 * avoid additional coding at protocol handlers.
 	 */
-	if (!pskb_may_pull(skb, iph->ihl * 4 + 8))
+	if (!pskb_may_pull(skb, iph->ihl * 4 + 8)) {
+		ICMP_INC_STATS_BH(dev_net(skb->dev), ICMP_MIB_INERRORS);
 		return;
+	}
 
 	raw_icmp_error(skb, protocol, info);

net/ipv4/tcp_ipv4.c +0 -5

index 77cccda..715cf6b 100644
@@ -342,11 +342,6 @@ void tcp_v4_err(struct sk_buff *icmp_skb, u32 info)
 	int err;
 	struct net *net = dev_net(icmp_skb->dev);
 
-	if (icmp_skb->len < (iph->ihl << 2) + 8) {
-		ICMP_INC_STATS_BH(net, ICMP_MIB_INERRORS);
-		return;
-	}
-
 	sk = inet_lookup(net, &tcp_hashinfo, iph->daddr, th->dest,
 			iph->saddr, th->source, inet_iif(icmp_skb));
 	if (!sk) {

net/ipv6/icmp.c +8 -3

index f6c84a6..8ca3cc0 100644
@@ -626,9 +626,10 @@ void icmpv6_notify(struct sk_buff *skb, u8 type, u8 code, __be32 info)
 	int inner_offset;
 	__be16 frag_off;
 	u8 nexthdr;
+	struct net *net = dev_net(skb->dev);
 
 	if (!pskb_may_pull(skb, sizeof(struct ipv6hdr)))
-		return;
+		goto out;
 
 	nexthdr = ((struct ipv6hdr *)skb->data)->nexthdr;
 	if (ipv6_ext_hdr(nexthdr)) {

@@ -636,14 +637,14 @@ void icmpv6_notify(struct sk_buff *skb, u8 type, u8 code, __be32 info)
 		inner_offset = ipv6_skip_exthdr(skb, sizeof(struct ipv6hdr),
 						&nexthdr, &frag_off);
 		if (inner_offset<0)
-			return;
+			goto out;
 	} else {
 		inner_offset = sizeof(struct ipv6hdr);
 	}
 
 	/* Checkin header including 8 bytes of inner protocol header. */
 	if (!pskb_may_pull(skb, inner_offset+8))
-		return;
+		goto out;
 
 	/* BUGGG_FUTURE: we should try to parse exthdrs in this packet.
 	   Without this we will not able f.e. to make source routed

@@ -659,6 +660,10 @@ void icmpv6_notify(struct sk_buff *skb, u8 type, u8 code, __be32 info)
 	rcu_read_unlock();
 
 	raw6_icmp_error(skb, nexthdr, type, code, inner_offset, info);
+	return;
+
+out:
+	ICMP6_INC_STATS_BH(net, __in6_dev_get(skb->dev), ICMP6_MIB_INERRORS);
 }
 
 /*

net/ipv6/udp.c +6 -2

index 7092ff7..f13d297 100644
@@ -534,11 +534,15 @@ void __udp6_lib_err(struct sk_buff *skb, struct inet6_skb_parm *opt,
 	struct udphdr *uh = (struct udphdr*)(skb->data+offset);
 	struct sock *sk;
 	int err;
+	struct net *net = dev_net(skb->dev);
 
-	sk = __udp6_lib_lookup(dev_net(skb->dev), daddr, uh->dest,
+	sk = __udp6_lib_lookup(net, daddr, uh->dest,
 			       saddr, uh->source, inet6_iif(skb), udptable);
-	if (sk == NULL)
+	if (sk == NULL) {
+		ICMP6_INC_STATS_BH(net, __in6_dev_get(skb->dev),
+				   ICMP6_MIB_INERRORS);
 		return;
+	}
 
 	if (type == ICMPV6_PKT_TOOBIG) {
 		if (!ip6_sk_accept_pmtu(sk))

net/sctp/input.c +0 -5

index f2e2cbd..c1b9912 100644
@@ -575,11 +575,6 @@ void sctp_v4_err(struct sk_buff *skb, __u32 info)
 	int err;
 	struct net *net = dev_net(skb->dev);
 
-	if (skb->len < ihlen + 8) {
-		ICMP_INC_STATS_BH(net, ICMP_MIB_INERRORS);
-		return;
-	}
-
 	/* Fix up skb to look at the embedded net header. */
 	saveip = skb->network_header;
 	savesctp = skb->transport_header;

Signature · 1 line

1.8.3.1

Hannes Frederic Sowa

2014-07-30 13:45 UTC #1 lore raw

Quoted 7 lines · Duan Jiong

When dealing with ICMPv[46] Error Message, function icmp_socket_deliver() and icmpv6_notify() do some valid checks on packet's length, but then some protocols check packet's length redaudantly. So remove those duplicated statements, and increase counter ICMP_MIB_INERRORS/ICMP6_MIB_INERRORS in function icmp_socket_deliver() and icmpv6_notify() respectively.
In addition, add missed counter in udp6/udplite6 when socket is NULL.

I am ok with adding the error counters, but...

Quoted 74 lines

---
 net/ipv4/icmp.c     |  4 +++-
 net/ipv4/tcp_ipv4.c |  5 -----
 net/ipv6/icmp.c     | 11 ++++++++---
 net/ipv6/udp.c      |  8 ++++++--
 net/sctp/input.c    |  5 -----
 5 files changed, 17 insertions(+), 16 deletions(-)

net/ipv4/icmp.c +3 -1

index 42b7bcf..092400e 100644
@@ -663,8 +663,10 @@ static void icmp_socket_deliver(struct sk_buff *skb, u32 info)
 	/* Checkin full IP header plus 8 bytes of protocol to
 	 * avoid additional coding at protocol handlers.
 	 */
-	if (!pskb_may_pull(skb, iph->ihl * 4 + 8))
+	if (!pskb_may_pull(skb, iph->ihl * 4 + 8)) {
+		ICMP_INC_STATS_BH(dev_net(skb->dev), ICMP_MIB_INERRORS);
 		return;
+	}
 
 	raw_icmp_error(skb, protocol, info);

net/ipv4/tcp_ipv4.c +0 -5

index 77cccda..715cf6b 100644
@@ -342,11 +342,6 @@ void tcp_v4_err(struct sk_buff *icmp_skb, u32 info)
 	int err;
 	struct net *net = dev_net(icmp_skb->dev);
 
-	if (icmp_skb->len < (iph->ihl << 2) + 8) {
-		ICMP_INC_STATS_BH(net, ICMP_MIB_INERRORS);
-		return;
-	}
-
 	sk = inet_lookup(net, &tcp_hashinfo, iph->daddr, th->dest,
 			iph->saddr, th->source, inet_iif(icmp_skb));
 	if (!sk) {

net/ipv6/icmp.c +4 -3

index f6c84a6..8ca3cc0 100644
@@ -626,9 +626,10 @@ void icmpv6_notify(struct sk_buff *skb, u8 type, u8 code, __be32 info)
 	int inner_offset;
 	__be16 frag_off;
 	u8 nexthdr;
+	struct net *net = dev_net(skb->dev);
 
 	if (!pskb_may_pull(skb, sizeof(struct ipv6hdr)))
-		return;
+		goto out;
 
 	nexthdr = ((struct ipv6hdr *)skb->data)->nexthdr;
 	if (ipv6_ext_hdr(nexthdr)) {

@@ -636,14 +637,14 @@ void icmpv6_notify(struct sk_buff *skb, u8 type, u8 code, __be32 info)
 		inner_offset = ipv6_skip_exthdr(skb, sizeof(struct ipv6hdr),
 						&nexthdr, &frag_off);
 		if (inner_offset<0)
-			return;
+			goto out;
 	} else {
 		inner_offset = sizeof(struct ipv6hdr);
 	}
 
 	/* Checkin header including 8 bytes of inner protocol header. */
 	if (!pskb_may_pull(skb, inner_offset+8))
-		return;
+		goto out;
 
 	/* BUGGG_FUTURE: we should try to parse exthdrs in this packet.
 	   Without this we will not able f.e. to make source routed

net/ipv6/icmp.c void icmpv6_notify(struct sk_buff *skb, u8 type, u8 code, __be32 info) · line 666

Quoted 7 lines

net/ipv6/icmp.c +4 -0

@@ -659,6 +660,10 @@ void icmpv6_notify(struct sk_buff *skb, u8 type, u8 code, __be32 info)
 	rcu_read_unlock();
 
 	raw6_icmp_error(skb, nexthdr, type, code, inner_offset, info);
+	return;
+
+out:
+	ICMP6_INC_STATS_BH(net, __in6_dev_get(skb->dev), ICMP6_MIB_INERRORS);

... please don't use __in6_dev_get in BH without rcu, but inet6_iif. You risk a lockdep error otherwise. You don't have rtnl locked and no RCU read lock.

Quoted 10 lines

net/ipv6/icmp.c +0 -0

 }
 
 /*

net/ipv6/udp.c +0 -0

index 7092ff7..f13d297 100644
@@ -534,11 +534,15 @@ void __udp6_lib_err(struct sk_buff *skb, struct inet6_skb_parm *opt,
 	struct udphdr *uh = (struct udphdr*)(skb->data+offset);
 	struct sock *sk;

net/ipv6/udp.c void __udp6_lib_err(struct sk_buff *skb, struct inet6_skb_parm *opt, · line 543

Quoted 10 lines

net/ipv6/udp.c +5 -2

 	int err;
+	struct net *net = dev_net(skb->dev);
 
-	sk = __udp6_lib_lookup(dev_net(skb->dev), daddr, uh->dest,
+	sk = __udp6_lib_lookup(net, daddr, uh->dest,
 			       saddr, uh->source, inet6_iif(skb), udptable);
-	if (sk == NULL)
+	if (sk == NULL) {
+		ICMP6_INC_STATS_BH(net, __in6_dev_get(skb->dev),
+				   ICMP6_MIB_INERRORS);

It is not really necessary here, as you like.

Bye,
Hannes

Duan Jiong

author

2014-07-31 02:58 UTC #2 lore raw

Quoted 87 lines · Hannes Frederic Sowa

Quoted 7 lines · Duan Jiong

When dealing with ICMPv[46] Error Message, function icmp_socket_deliver() and icmpv6_notify() do some valid checks on packet's length, but then some protocols check packet's length redaudantly. So remove those duplicated statements, and increase counter ICMP_MIB_INERRORS/ICMP6_MIB_INERRORS in function icmp_socket_deliver() and icmpv6_notify() respectively.
In addition, add missed counter in udp6/udplite6 when socket is NULL.

I am ok with adding the error counters, but...

Quoted 76 lines

---
 net/ipv4/icmp.c     |  4 +++-
 net/ipv4/tcp_ipv4.c |  5 -----
 net/ipv6/icmp.c     | 11 ++++++++---
 net/ipv6/udp.c      |  8 ++++++--
 net/sctp/input.c    |  5 -----
 5 files changed, 17 insertions(+), 16 deletions(-)

net/ipv4/icmp.c +3 -1

index 42b7bcf..092400e 100644
@@ -663,8 +663,10 @@ static void icmp_socket_deliver(struct sk_buff *skb, u32 info)
 	/* Checkin full IP header plus 8 bytes of protocol to
 	 * avoid additional coding at protocol handlers.
 	 */
-	if (!pskb_may_pull(skb, iph->ihl * 4 + 8))
+	if (!pskb_may_pull(skb, iph->ihl * 4 + 8)) {
+		ICMP_INC_STATS_BH(dev_net(skb->dev), ICMP_MIB_INERRORS);
 		return;
+	}
 
 	raw_icmp_error(skb, protocol, info);

net/ipv4/tcp_ipv4.c +0 -5

index 77cccda..715cf6b 100644
@@ -342,11 +342,6 @@ void tcp_v4_err(struct sk_buff *icmp_skb, u32 info)
 	int err;
 	struct net *net = dev_net(icmp_skb->dev);
 
-	if (icmp_skb->len < (iph->ihl << 2) + 8) {
-		ICMP_INC_STATS_BH(net, ICMP_MIB_INERRORS);
-		return;
-	}
-
 	sk = inet_lookup(net, &tcp_hashinfo, iph->daddr, th->dest,
 			iph->saddr, th->source, inet_iif(icmp_skb));
 	if (!sk) {

net/ipv6/icmp.c +4 -3

index f6c84a6..8ca3cc0 100644
@@ -626,9 +626,10 @@ void icmpv6_notify(struct sk_buff *skb, u8 type, u8 code, __be32 info)
 	int inner_offset;
 	__be16 frag_off;
 	u8 nexthdr;
+	struct net *net = dev_net(skb->dev);
 
 	if (!pskb_may_pull(skb, sizeof(struct ipv6hdr)))
-		return;
+		goto out;
 
 	nexthdr = ((struct ipv6hdr *)skb->data)->nexthdr;
 	if (ipv6_ext_hdr(nexthdr)) {

@@ -636,14 +637,14 @@ void icmpv6_notify(struct sk_buff *skb, u8 type, u8 code, __be32 info)
 		inner_offset = ipv6_skip_exthdr(skb, sizeof(struct ipv6hdr),
 						&nexthdr, &frag_off);
 		if (inner_offset<0)
-			return;
+			goto out;
 	} else {
 		inner_offset = sizeof(struct ipv6hdr);
 	}
 
 	/* Checkin header including 8 bytes of inner protocol header. */
 	if (!pskb_may_pull(skb, inner_offset+8))
-		return;
+		goto out;
 
 	/* BUGGG_FUTURE: we should try to parse exthdrs in this packet.
 	   Without this we will not able f.e. to make source routed

@@ -659,6 +660,10 @@ void icmpv6_notify(struct sk_buff *skb, u8 type, u8 code, __be32 info)
 	rcu_read_unlock();

Quoted 10 lines

net/ipv6/icmp.c void icmpv6_notify(struct sk_buff *skb, u8 type, u8 code, __be32 info) · line 666
Quoted 5 lines
net/ipv6/icmp.c +4 -0
 	raw6_icmp_error(skb, nexthdr, type, code, inner_offset, info);
+	return;
+
+out:
+	ICMP6_INC_STATS_BH(net, __in6_dev_get(skb->dev), ICMP6_MIB_INERRORS);
... please don't use __in6_dev_get in BH without rcu, but inet6_iif. You risk a lockdep error otherwise. You don't have rtnl locked and no RCU read lock.

May be we can use in6_dev_get instead of __in6_dev_get, and use in6_dev_put to release the reference later.

Quoted 13 lines

net/ipv6/icmp.c +0 -0

 }
 
 /*

net/ipv6/udp.c +1 -0

index 7092ff7..f13d297 100644
@@ -534,11 +534,15 @@ void __udp6_lib_err(struct sk_buff *skb, struct inet6_skb_parm *opt,
 	struct udphdr *uh = (struct udphdr*)(skb->data+offset);
 	struct sock *sk;
 	int err;
+	struct net *net = dev_net(skb->dev);

Quoted 10 lines

net/ipv6/udp.c void __udp6_lib_err(struct sk_buff *skb, struct inet6_skb_parm *opt, · line 543
Quoted 7 lines
net/ipv6/udp.c +4 -2
-	sk = __udp6_lib_lookup(dev_net(skb->dev), daddr, uh->dest,
+	sk = __udp6_lib_lookup(net, daddr, uh->dest,
 			       saddr, uh->source, inet6_iif(skb), udptable);
-	if (sk == NULL)
+	if (sk == NULL) {
+		ICMP6_INC_STATS_BH(net, __in6_dev_get(skb->dev),
+				   ICMP6_MIB_INERRORS);
It is not really necessary here, as you like.

I add it just because i saw the same statements in dccp6/sctp6/tcp6 error handler.

Thanks,
Duan

Quoted 5 lines

Bye,
Hannes
.

Hannes Frederic Sowa

2014-07-31 07:52 UTC #3 lore raw

Quoted 99 lines · Duan Jiong

Quoted 87 lines · Hannes Frederic Sowa

Quoted 7 lines · Duan Jiong

When dealing with ICMPv[46] Error Message, function icmp_socket_deliver() and icmpv6_notify() do some valid checks on packet's length, but then some protocols check packet's length redaudantly. So remove those duplicated statements, and increase counter ICMP_MIB_INERRORS/ICMP6_MIB_INERRORS in function icmp_socket_deliver() and icmpv6_notify() respectively.
In addition, add missed counter in udp6/udplite6 when socket is NULL.

I am ok with adding the error counters, but...

Quoted 76 lines

---
 net/ipv4/icmp.c     |  4 +++-
 net/ipv4/tcp_ipv4.c |  5 -----
 net/ipv6/icmp.c     | 11 ++++++++---
 net/ipv6/udp.c      |  8 ++++++--
 net/sctp/input.c    |  5 -----
 5 files changed, 17 insertions(+), 16 deletions(-)

net/ipv4/icmp.c +3 -1

index 42b7bcf..092400e 100644
@@ -663,8 +663,10 @@ static void icmp_socket_deliver(struct sk_buff *skb, u32 info)
 	/* Checkin full IP header plus 8 bytes of protocol to
 	 * avoid additional coding at protocol handlers.
 	 */
-	if (!pskb_may_pull(skb, iph->ihl * 4 + 8))
+	if (!pskb_may_pull(skb, iph->ihl * 4 + 8)) {
+		ICMP_INC_STATS_BH(dev_net(skb->dev), ICMP_MIB_INERRORS);
 		return;
+	}
 
 	raw_icmp_error(skb, protocol, info);

net/ipv4/tcp_ipv4.c +0 -5

index 77cccda..715cf6b 100644
@@ -342,11 +342,6 @@ void tcp_v4_err(struct sk_buff *icmp_skb, u32 info)
 	int err;
 	struct net *net = dev_net(icmp_skb->dev);
 
-	if (icmp_skb->len < (iph->ihl << 2) + 8) {
-		ICMP_INC_STATS_BH(net, ICMP_MIB_INERRORS);
-		return;
-	}
-
 	sk = inet_lookup(net, &tcp_hashinfo, iph->daddr, th->dest,
 			iph->saddr, th->source, inet_iif(icmp_skb));
 	if (!sk) {

net/ipv6/icmp.c +4 -3

index f6c84a6..8ca3cc0 100644
@@ -626,9 +626,10 @@ void icmpv6_notify(struct sk_buff *skb, u8 type, u8 code, __be32 info)
 	int inner_offset;
 	__be16 frag_off;
 	u8 nexthdr;
+	struct net *net = dev_net(skb->dev);
 
 	if (!pskb_may_pull(skb, sizeof(struct ipv6hdr)))
-		return;
+		goto out;
 
 	nexthdr = ((struct ipv6hdr *)skb->data)->nexthdr;
 	if (ipv6_ext_hdr(nexthdr)) {

@@ -636,14 +637,14 @@ void icmpv6_notify(struct sk_buff *skb, u8 type, u8 code, __be32 info)
 		inner_offset = ipv6_skip_exthdr(skb, sizeof(struct ipv6hdr),
 						&nexthdr, &frag_off);
 		if (inner_offset<0)
-			return;
+			goto out;
 	} else {
 		inner_offset = sizeof(struct ipv6hdr);
 	}
 
 	/* Checkin header including 8 bytes of inner protocol header. */
 	if (!pskb_may_pull(skb, inner_offset+8))
-		return;
+		goto out;
 
 	/* BUGGG_FUTURE: we should try to parse exthdrs in this packet.
 	   Without this we will not able f.e. to make source routed

@@ -659,6 +660,10 @@ void icmpv6_notify(struct sk_buff *skb, u8 type, u8 code, __be32 info)
 	rcu_read_unlock();

Quoted 10 lines

net/ipv6/icmp.c void icmpv6_notify(struct sk_buff *skb, u8 type, u8 code, __be32 info) · line 666
Quoted 5 lines
net/ipv6/icmp.c +4 -0
 	raw6_icmp_error(skb, nexthdr, type, code, inner_offset, info);
+	return;
+
+out:
+	ICMP6_INC_STATS_BH(net, __in6_dev_get(skb->dev), ICMP6_MIB_INERRORS);
... please don't use __in6_dev_get in BH without rcu, but inet6_iif. You risk a lockdep error otherwise. You don't have rtnl locked and no RCU read lock.

Quoted 2 lines

May be we can use in6_dev_get instead of __in6_dev_get, and use in6_dev_put to release the reference later.

Sure, you can. You can also protect the update of the counter with an rcu read lock, but you won't find anything easier than inet6_iif. ;)

Quoted 24 lines

Quoted 13 lines

net/ipv6/icmp.c +0 -0

 }
 
 /*

net/ipv6/udp.c +1 -0

index 7092ff7..f13d297 100644
@@ -534,11 +534,15 @@ void __udp6_lib_err(struct sk_buff *skb, struct inet6_skb_parm *opt,
 	struct udphdr *uh = (struct udphdr*)(skb->data+offset);
 	struct sock *sk;
 	int err;
+	struct net *net = dev_net(skb->dev);

Quoted 10 lines

net/ipv6/udp.c void __udp6_lib_err(struct sk_buff *skb, struct inet6_skb_parm *opt, · line 543
Quoted 7 lines
net/ipv6/udp.c +4 -2
-	sk = __udp6_lib_lookup(dev_net(skb->dev), daddr, uh->dest,
+	sk = __udp6_lib_lookup(net, daddr, uh->dest,
 			       saddr, uh->source, inet6_iif(skb), udptable);
-	if (sk == NULL)
+	if (sk == NULL) {
+		ICMP6_INC_STATS_BH(net, __in6_dev_get(skb->dev),
+				   ICMP6_MIB_INERRORS);
It is not really necessary here, as you like.

Quoted 2 lines

I add it just because i saw the same statements in dccp6/sctp6/tcp6 error handler.

Counting errors here is fine. And because you are have rcu read lock the __in6_dev_get is also fine.

Bye,
Hannes

Duan Jiong

author

2014-07-31 08:34 UTC #4 lore raw

Quoted 100 lines · Hannes Frederic Sowa

Quoted 99 lines · Duan Jiong

Quoted 97 lines · Hannes Frederic Sowa

Quoted 7 lines · Duan Jiong

When dealing with ICMPv[46] Error Message, function icmp_socket_deliver() and icmpv6_notify() do some valid checks on packet's length, but then some protocols check packet's length redaudantly. So remove those duplicated statements, and increase counter ICMP_MIB_INERRORS/ICMP6_MIB_INERRORS in function icmp_socket_deliver() and icmpv6_notify() respectively.
In addition, add missed counter in udp6/udplite6 when socket is NULL.

I am ok with adding the error counters, but...

Quoted 74 lines

---
 net/ipv4/icmp.c     |  4 +++-
 net/ipv4/tcp_ipv4.c |  5 -----
 net/ipv6/icmp.c     | 11 ++++++++---
 net/ipv6/udp.c      |  8 ++++++--
 net/sctp/input.c    |  5 -----
 5 files changed, 17 insertions(+), 16 deletions(-)

net/ipv4/icmp.c +3 -1

index 42b7bcf..092400e 100644
@@ -663,8 +663,10 @@ static void icmp_socket_deliver(struct sk_buff *skb, u32 info)
 	/* Checkin full IP header plus 8 bytes of protocol to
 	 * avoid additional coding at protocol handlers.
 	 */
-	if (!pskb_may_pull(skb, iph->ihl * 4 + 8))
+	if (!pskb_may_pull(skb, iph->ihl * 4 + 8)) {
+		ICMP_INC_STATS_BH(dev_net(skb->dev), ICMP_MIB_INERRORS);
 		return;
+	}
 
 	raw_icmp_error(skb, protocol, info);

net/ipv4/tcp_ipv4.c +0 -5

index 77cccda..715cf6b 100644
@@ -342,11 +342,6 @@ void tcp_v4_err(struct sk_buff *icmp_skb, u32 info)
 	int err;
 	struct net *net = dev_net(icmp_skb->dev);
 
-	if (icmp_skb->len < (iph->ihl << 2) + 8) {
-		ICMP_INC_STATS_BH(net, ICMP_MIB_INERRORS);
-		return;
-	}
-
 	sk = inet_lookup(net, &tcp_hashinfo, iph->daddr, th->dest,
 			iph->saddr, th->source, inet_iif(icmp_skb));
 	if (!sk) {

net/ipv6/icmp.c +4 -3

index f6c84a6..8ca3cc0 100644
@@ -626,9 +626,10 @@ void icmpv6_notify(struct sk_buff *skb, u8 type, u8 code, __be32 info)
 	int inner_offset;
 	__be16 frag_off;
 	u8 nexthdr;
+	struct net *net = dev_net(skb->dev);
 
 	if (!pskb_may_pull(skb, sizeof(struct ipv6hdr)))
-		return;
+		goto out;
 
 	nexthdr = ((struct ipv6hdr *)skb->data)->nexthdr;
 	if (ipv6_ext_hdr(nexthdr)) {

@@ -636,14 +637,14 @@ void icmpv6_notify(struct sk_buff *skb, u8 type, u8 code, __be32 info)
 		inner_offset = ipv6_skip_exthdr(skb, sizeof(struct ipv6hdr),
 						&nexthdr, &frag_off);
 		if (inner_offset<0)
-			return;
+			goto out;
 	} else {
 		inner_offset = sizeof(struct ipv6hdr);
 	}
 
 	/* Checkin header including 8 bytes of inner protocol header. */
 	if (!pskb_may_pull(skb, inner_offset+8))
-		return;
+		goto out;
 
 	/* BUGGG_FUTURE: we should try to parse exthdrs in this packet.
 	   Without this we will not able f.e. to make source routed

net/ipv6/icmp.c void icmpv6_notify(struct sk_buff *skb, u8 type, u8 code, __be32 info) · line 666

Quoted 7 lines

net/ipv6/icmp.c +4 -0

@@ -659,6 +660,10 @@ void icmpv6_notify(struct sk_buff *skb, u8 type, u8 code, __be32 info)
 	rcu_read_unlock();
 
 	raw6_icmp_error(skb, nexthdr, type, code, inner_offset, info);
+	return;
+
+out:
+	ICMP6_INC_STATS_BH(net, __in6_dev_get(skb->dev), ICMP6_MIB_INERRORS);

... please don't use __in6_dev_get in BH without rcu, but inet6_iif. You risk a lockdep error otherwise. You don't have rtnl locked and no RCU read lock.

Quoted 5 lines

Quoted 2 lines
May be we can use in6_dev_get instead of __in6_dev_get, and use in6_dev_put to release the reference later.
Sure, you can. You can also protect the update of the counter with an rcu read lock, but you won't find anything easier than inet6_iif. ;)

Are you sure using inet6_iif is fine?

function inet6_iif() return int, but we need struct inet6_dev *.

Thanks,
Duan

Quoted 37 lines

Quoted 25 lines
Quoted 14 lines
Quoted 14 lines
net/ipv6/icmp.c +0 -0
 }
 
 /*
net/ipv6/udp.c +1 -0
index 7092ff7..f13d297 100644
@@ -534,11 +534,15 @@ void __udp6_lib_err(struct sk_buff *skb, struct inet6_skb_parm *opt,
 	struct udphdr *uh = (struct udphdr*)(skb->data+offset);
 	struct sock *sk;
 	int err;
+	struct net *net = dev_net(skb->dev);
 
Quoted 10 lines
net/ipv6/udp.c void __udp6_lib_err(struct sk_buff *skb, struct inet6_skb_parm *opt, · line 543
Quoted 7 lines
net/ipv6/udp.c +4 -2
-	sk = __udp6_lib_lookup(dev_net(skb->dev), daddr, uh->dest,
+	sk = __udp6_lib_lookup(net, daddr, uh->dest,
 			       saddr, uh->source, inet6_iif(skb), udptable);
-	if (sk == NULL)
+	if (sk == NULL) {
+		ICMP6_INC_STATS_BH(net, __in6_dev_get(skb->dev),
+				   ICMP6_MIB_INERRORS);
It is not really necessary here, as you like.
Quoted 2 lines
I add it just because i saw the same statements in dccp6/sctp6/tcp6 error handler.
Counting errors here is fine. And because you are have rcu read lock the __in6_dev_get is also fine.
Bye,
Hannes
.

Hannes Frederic Sowa

2014-07-31 09:11 UTC #5 lore raw

Quoted 104 lines · Duan Jiong

Quoted 103 lines · Hannes Frederic Sowa

Quoted 101 lines · Duan Jiong

Quoted 87 lines · Hannes Frederic Sowa

Quoted 7 lines · Duan Jiong

When dealing with ICMPv[46] Error Message, function icmp_socket_deliver() and icmpv6_notify() do some valid checks on packet's length, but then some protocols check packet's length redaudantly. So remove those duplicated statements, and increase counter ICMP_MIB_INERRORS/ICMP6_MIB_INERRORS in function icmp_socket_deliver() and icmpv6_notify() respectively.
In addition, add missed counter in udp6/udplite6 when socket is NULL.

I am ok with adding the error counters, but...

Quoted 76 lines

---
 net/ipv4/icmp.c     |  4 +++-
 net/ipv4/tcp_ipv4.c |  5 -----
 net/ipv6/icmp.c     | 11 ++++++++---
 net/ipv6/udp.c      |  8 ++++++--
 net/sctp/input.c    |  5 -----
 5 files changed, 17 insertions(+), 16 deletions(-)

net/ipv4/icmp.c +3 -1

index 42b7bcf..092400e 100644
@@ -663,8 +663,10 @@ static void icmp_socket_deliver(struct sk_buff *skb, u32 info)
 	/* Checkin full IP header plus 8 bytes of protocol to
 	 * avoid additional coding at protocol handlers.
 	 */
-	if (!pskb_may_pull(skb, iph->ihl * 4 + 8))
+	if (!pskb_may_pull(skb, iph->ihl * 4 + 8)) {
+		ICMP_INC_STATS_BH(dev_net(skb->dev), ICMP_MIB_INERRORS);
 		return;
+	}
 
 	raw_icmp_error(skb, protocol, info);

net/ipv4/tcp_ipv4.c +0 -5

index 77cccda..715cf6b 100644
@@ -342,11 +342,6 @@ void tcp_v4_err(struct sk_buff *icmp_skb, u32 info)
 	int err;
 	struct net *net = dev_net(icmp_skb->dev);
 
-	if (icmp_skb->len < (iph->ihl << 2) + 8) {
-		ICMP_INC_STATS_BH(net, ICMP_MIB_INERRORS);
-		return;
-	}
-
 	sk = inet_lookup(net, &tcp_hashinfo, iph->daddr, th->dest,
 			iph->saddr, th->source, inet_iif(icmp_skb));
 	if (!sk) {

net/ipv6/icmp.c +4 -3

index f6c84a6..8ca3cc0 100644
@@ -626,9 +626,10 @@ void icmpv6_notify(struct sk_buff *skb, u8 type, u8 code, __be32 info)
 	int inner_offset;
 	__be16 frag_off;
 	u8 nexthdr;
+	struct net *net = dev_net(skb->dev);
 
 	if (!pskb_may_pull(skb, sizeof(struct ipv6hdr)))
-		return;
+		goto out;
 
 	nexthdr = ((struct ipv6hdr *)skb->data)->nexthdr;
 	if (ipv6_ext_hdr(nexthdr)) {

@@ -636,14 +637,14 @@ void icmpv6_notify(struct sk_buff *skb, u8 type, u8 code, __be32 info)
 		inner_offset = ipv6_skip_exthdr(skb, sizeof(struct ipv6hdr),
 						&nexthdr, &frag_off);
 		if (inner_offset<0)
-			return;
+			goto out;
 	} else {
 		inner_offset = sizeof(struct ipv6hdr);
 	}
 
 	/* Checkin header including 8 bytes of inner protocol header. */
 	if (!pskb_may_pull(skb, inner_offset+8))
-		return;
+		goto out;
 
 	/* BUGGG_FUTURE: we should try to parse exthdrs in this packet.
 	   Without this we will not able f.e. to make source routed

@@ -659,6 +660,10 @@ void icmpv6_notify(struct sk_buff *skb, u8 type, u8 code, __be32 info)
 	rcu_read_unlock();

Quoted 10 lines

net/ipv6/icmp.c void icmpv6_notify(struct sk_buff *skb, u8 type, u8 code, __be32 info) · line 666
Quoted 5 lines
net/ipv6/icmp.c +4 -0
 	raw6_icmp_error(skb, nexthdr, type, code, inner_offset, info);
+	return;
+
+out:
+	ICMP6_INC_STATS_BH(net, __in6_dev_get(skb->dev), ICMP6_MIB_INERRORS);
... please don't use __in6_dev_get in BH without rcu, but inet6_iif. You risk a lockdep error otherwise. You don't have rtnl locked and no RCU read lock.

May be we can use in6_dev_get instead of __in6_dev_get, and use in6_dev_put to release the reference later.

Quoted 6 lines

Quoted 2 lines
Sure, you can. You can also protect the update of the counter with an rcu read lock, but you won't find anything easier than inet6_iif. ;)
Are you sure using inet6_iif is fine?
function inet6_iif() return int, but we need struct inet6_dev *.

No, sorry, you are correct, my fault. ;)

Somehow I thought *6_INC_STATS just take an interface index.

I would just propose to rcu lock this section like:

rcu_read_lock();
ICMP6_INC_STATS_BH(net, __in6_dev_get(skb->dev), ICMP6_MIB_INERRORS); rcu_read_unlock();

If you look at the invocations of icmpv6_notify, they are all rcu read lock protected, so your initial patch already was perfectly correct, just the rcu_read_unlock() in the context of the change and the use of __in6_dev_get alerted me.

As we have a nested rcu_read_lock/unlock invocation just above in icmpv6_notify, I would still add the rcu_read_lock as noted above for documentation/review purposes (or remove the rcu_read_lock from the err_handler dispatch part of icmpv6_notify).

Thank you,
Hannes

Duan Jiong

author

2014-07-31 09:55 UTC #6 lore raw

PATCH v2 1/1 net: fix the counter ICMP_MIB_INERRORS/ICMP6_MIB_INERRORS

In reply to Duan Jiong

In addition, add missed counter in udp6/udplite6 when socket is NULL.

Signed-off-by Duan Jiong <[email protected]>

---

v2: remove nested rcu_read_lock from icmpv6_notify

 net/ipv4/icmp.c     |  4 +++-
 net/ipv4/tcp_ipv4.c |  5 -----
 net/ipv6/icmp.c     | 13 ++++++++-----
 net/ipv6/udp.c      |  8 ++++++--
 net/sctp/input.c    |  5 -----
 5 files changed, 17 insertions(+), 18 deletions(-)

net/ipv4/icmp.c +3 -1

index 42b7bcf..092400e 100644
@@ -663,8 +663,10 @@ static void icmp_socket_deliver(struct sk_buff *skb, u32 info)
 	/* Checkin full IP header plus 8 bytes of protocol to
 	 * avoid additional coding at protocol handlers.
 	 */
-	if (!pskb_may_pull(skb, iph->ihl * 4 + 8))
+	if (!pskb_may_pull(skb, iph->ihl * 4 + 8)) {
+		ICMP_INC_STATS_BH(dev_net(skb->dev), ICMP_MIB_INERRORS);
 		return;
+	}
 
 	raw_icmp_error(skb, protocol, info);

net/ipv4/tcp_ipv4.c +0 -5

index 77cccda..715cf6b 100644
@@ -342,11 +342,6 @@ void tcp_v4_err(struct sk_buff *icmp_skb, u32 info)
 	int err;
 	struct net *net = dev_net(icmp_skb->dev);
 
-	if (icmp_skb->len < (iph->ihl << 2) + 8) {
-		ICMP_INC_STATS_BH(net, ICMP_MIB_INERRORS);
-		return;
-	}
-
 	sk = inet_lookup(net, &tcp_hashinfo, iph->daddr, th->dest,
 			iph->saddr, th->source, inet_iif(icmp_skb));
 	if (!sk) {

net/ipv6/icmp.c +8 -5

index f6c84a6..06ba3e5 100644
@@ -626,9 +626,10 @@ void icmpv6_notify(struct sk_buff *skb, u8 type, u8 code, __be32 info)
 	int inner_offset;
 	__be16 frag_off;
 	u8 nexthdr;
+	struct net *net = dev_net(skb->dev);
 
 	if (!pskb_may_pull(skb, sizeof(struct ipv6hdr)))
-		return;
+		goto out;
 
 	nexthdr = ((struct ipv6hdr *)skb->data)->nexthdr;
 	if (ipv6_ext_hdr(nexthdr)) {

@@ -636,14 +637,14 @@ void icmpv6_notify(struct sk_buff *skb, u8 type, u8 code, __be32 info)
 		inner_offset = ipv6_skip_exthdr(skb, sizeof(struct ipv6hdr),
 						&nexthdr, &frag_off);
 		if (inner_offset<0)
-			return;
+			goto out;
 	} else {
 		inner_offset = sizeof(struct ipv6hdr);
 	}
 
 	/* Checkin header including 8 bytes of inner protocol header. */
 	if (!pskb_may_pull(skb, inner_offset+8))
-		return;
+		goto out;
 
 	/* BUGGG_FUTURE: we should try to parse exthdrs in this packet.
 	   Without this we will not able f.e. to make source routed

@@ -652,13 +653,15 @@ void icmpv6_notify(struct sk_buff *skb, u8 type, u8 code, __be32 info)
 	   --ANK (980726)
 	 */
 
-	rcu_read_lock();
 	ipprot = rcu_dereference(inet6_protos[nexthdr]);
 	if (ipprot && ipprot->err_handler)
 		ipprot->err_handler(skb, NULL, type, code, inner_offset, info);
-	rcu_read_unlock();
 
 	raw6_icmp_error(skb, nexthdr, type, code, inner_offset, info);
+	return;
+
+out:
+	ICMP6_INC_STATS_BH(net, __in6_dev_get(skb->dev), ICMP6_MIB_INERRORS);
 }
 
 /*

net/ipv6/udp.c +6 -2

index 7092ff7..f13d297 100644
@@ -534,11 +534,15 @@ void __udp6_lib_err(struct sk_buff *skb, struct inet6_skb_parm *opt,
 	struct udphdr *uh = (struct udphdr*)(skb->data+offset);
 	struct sock *sk;
 	int err;
+	struct net *net = dev_net(skb->dev);
 
-	sk = __udp6_lib_lookup(dev_net(skb->dev), daddr, uh->dest,
+	sk = __udp6_lib_lookup(net, daddr, uh->dest,
 			       saddr, uh->source, inet6_iif(skb), udptable);
-	if (sk == NULL)
+	if (sk == NULL) {
+		ICMP6_INC_STATS_BH(net, __in6_dev_get(skb->dev),
+				   ICMP6_MIB_INERRORS);
 		return;
+	}
 
 	if (type == ICMPV6_PKT_TOOBIG) {
 		if (!ip6_sk_accept_pmtu(sk))

net/sctp/input.c +0 -5

index f2e2cbd..c1b9912 100644
@@ -575,11 +575,6 @@ void sctp_v4_err(struct sk_buff *skb, __u32 info)
 	int err;
 	struct net *net = dev_net(skb->dev);
 
-	if (skb->len < ihlen + 8) {
-		ICMP_INC_STATS_BH(net, ICMP_MIB_INERRORS);
-		return;
-	}
-
 	/* Fix up skb to look at the embedded net header. */
 	saveip = skb->network_header;
 	savesctp = skb->transport_header;

Signature · 1 line

1.8.3.1

Hannes Frederic Sowa

Acked-by on v2 1/1

2014-07-31 21:05 UTC #7 lore raw

Quoted 6 lines · Duan Jiong

When dealing with ICMPv[46] Error Message, function icmp_socket_deliver() and icmpv6_notify() do some valid checks on packet's length, but then some protocols check packet's length redaudantly. So remove those duplicated statements, and increase counter ICMP_MIB_INERRORS/ICMP6_MIB_INERRORS in function icmp_socket_deliver() and icmpv6_notify() respectively.

Quoted 6 lines

In addition, add missed counter in udp6/udplite6 when socket is NULL.
Signed-off-by Duan Jiong <[email protected]>
---
v2: remove nested rcu_read_lock from icmpv6_notify

Acked-by Hannes Frederic Sowa <[email protected]>

IPv4 handler can also be cleaned up. ;)

Thanks!

Duan Jiong

author

2014-08-01 01:09 UTC #8 lore raw

Quoted 9 lines · Hannes Frederic Sowa

Quoted 8 lines · Duan Jiong
When dealing with ICMPv[46] Error Message, function icmp_socket_deliver() and icmpv6_notify() do some valid checks on packet's length, but then some protocols check packet's length redaudantly. So remove those duplicated statements, and increase counter ICMP_MIB_INERRORS/ICMP6_MIB_INERRORS in function icmp_socket_deliver() and icmpv6_notify() respectively.
In addition, add missed counter in udp6/udplite6 when socket is NULL.

Quoted 8 lines

Quoted 4 lines
Signed-off-by Duan Jiong <[email protected]>
---
v2: remove nested rcu_read_lock from icmpv6_notify
Acked-by Hannes Frederic Sowa <[email protected]>
IPv4 handler can also be cleaned up. ;)

Ok, i will make another patch soon. :)

Thanks,
Duan

Quoted 4 lines

Thanks!

David Miller

maintainer Applied

2014-08-01 05:04 UTC #9 lore raw

In reply to Duan Jiong

From: Duan Jiong <[email protected]>
Date: Thu, 31 Jul 2014 17:54:32 +0800

Quoted 7 lines

When dealing with ICMPv[46] Error Message, function icmp_socket_deliver() and icmpv6_notify() do some valid checks on packet's length, but then some protocols check packet's length redaudantly. So remove those duplicated statements, and increase counter ICMP_MIB_INERRORS/ICMP6_MIB_INERRORS in function icmp_socket_deliver() and icmpv6_notify() respectively.

Quoted 6 lines

In addition, add missed counter in udp6/udplite6 when socket is NULL.
Signed-off-by Duan Jiong <[email protected]>
---
v2: remove nested rcu_read_lock from icmpv6_notify

Applied to net-next, thanks.