On Do, 2014-07-31 at 16:34 +0800, Duan Jiong wrote:

On 07/31/2014 03:52 PM, Hannes Frederic Sowa wrote:

quoted

On Do, 2014-07-31 at 10:58 +0800, Duan Jiong wrote:

quoted

On 07/30/2014 09:45 PM, Hannes Frederic Sowa wrote:

quoted

On Di, 2014-07-29 at 16:19 +0800, Duan Jiong wrote:

quoted

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

---
 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(-)

diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
index 42b7bcf..092400e 100644
--- a/net/ipv4/icmp.c
+++ b/net/ipv4/icmp.c

@@ -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);
 

diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 77cccda..715cf6b 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c

@@ -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) {

diff --git a/net/ipv6/icmp.c b/net/ipv6/icmp.c
index f6c84a6..8ca3cc0 100644
--- a/net/ipv6/icmp.c
+++ b/net/ipv6/icmp.c

@@ -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);

... 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.

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