From mboxrd@z Thu Jan 1 00:00:00 1970 From: Duan Jiong Subject: Re: [PATCH] net: fix the counter ICMP_MIB_INERRORS/ICMP6_MIB_INERRORS Date: Thu, 31 Jul 2014 10:58:28 +0800 Message-ID: <53D9B0D4.8040909@cn.fujitsu.com> References: <53D75922.2090106@cn.fujitsu.com> <1406727930.13572.5.camel@localhost> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit Cc: David Miller , , netdev To: Hannes Frederic Sowa Return-path: Received: from cn.fujitsu.com ([59.151.112.132]:39730 "EHLO heian.cn.fujitsu.com" rhost-flags-OK-FAIL-OK-FAIL) by vger.kernel.org with ESMTP id S1755433AbaGaC6p (ORCPT ); Wed, 30 Jul 2014 22:58:45 -0400 In-Reply-To: <1406727930.13572.5.camel@localhost> Sender: netdev-owner@vger.kernel.org List-ID: 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. >> } >> >> /* >> 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. Thanks, Duan > Bye, > Hannes > > . >