| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 31 Jul 2014 10:58:28 +0800
From: Duan Jiong <duanj.fnst@...fujitsu.com>
To: Hannes Frederic Sowa <hannes@...essinduktion.org>
CC: David Miller <davem@...emloft.net>, <eric.dumazet@...il.com>,
netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH] net: fix the counter ICMP_MIB_INERRORS/ICMP6_MIB_INERRORS
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@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists