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

[Hannes Frederic Sowa <hannes@stressinduktion.org>]

On Do, 2014-07-31 at 16:34 +0800, Duan Jiong wrote:
> 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 *.

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