| Message ID | 53D75922.2090106@cn.fujitsu.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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. > } > > /* > 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. Bye, Hannes -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 > > . > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. ;) > >> } > >> > >> /* > >> 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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 > > > . > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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); } /* 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); return; + } if (type == ICMPV6_PKT_TOOBIG) { if (!ip6_sk_accept_pmtu(sk)) diff --git a/net/sctp/input.c b/net/sctp/input.c index f2e2cbd..c1b9912 100644 --- a/net/sctp/input.c +++ b/net/sctp/input.c @@ -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;
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 <duanj.fnst@cn.fujitsu.com> --- 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(-)