Subject: Re: [PATCH] net: fix the counter ICMP_MIB_INERRORS/ICMP6_MIB_INERRORS

From: Duan Jiong <[email-masked]>
Date: Thu, 31 Jul 2014 16:34:07 +0800

On 07/31/2014 03:52 PM, Hannes Frederic Sowa wrote:
> On Do, 2014-07-31 at 10:58 +0800, Duan Jiong wrote:
>> On 07/30/2014 09:45 PM, Hannes Frederic Sowa wrote:
>>> On Di, 2014-07-29 at 16:19 +0800, Duan Jiong wrote:
>>>> 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...
>>>
>>>> ---
>>>>  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 *.

Thanks,
  Duan 

> 
>>>>  }
>>>>  
>>>>  /*
>>>> diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
>>>> index 7092ff7..f13d297 100644
>>>> --- a/net/ipv6/udp.c
>>>> +++ b/net/ipv6/udp.c
>>>> @@ -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);
>>>
>>> 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.
> 
> Counting errors here is fine. And because you are have rcu read lock the
> __in6_dev_get is also fine.
> 
> Bye,
> Hannes
> 
> 
> .
>