netdev - Re: [PATCH net-next] Per interface IPv4 stats (CONFIG_IP_IFSTATS

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHapkUg=XpRqL3y5nMAv+bR_dpCVgLY1Nj28Z1v86j=oM0y2mA@mail.gmail.com>
Date:   Thu, 12 Apr 2018 11:58:24 -0400
From:   Stephen Suryaputra <ssuryaextr@...il.com>
To:     Julian Anastasov <ja@....bg>
Cc:     netdev@...r.kernel.org
Subject: Re: [PATCH net-next] Per interface IPv4 stats (CONFIG_IP_IFSTATS_TABLE)

Thanks for the feedbacks. Please see the detail below:

On Wed, Apr 11, 2018 at 3:37 PM, Julian Anastasov <ja@....bg> wrote:
[snip]
>> -     __IP_INC_STATS(net, IPSTATS_MIB_INHDRERRORS);
>> +     __IP_INC_STATS(net, skb_dst(skb)->dev, IPSTATS_MIB_INHDRERRORS);
>
>         May be skb->dev if we want to account it to the
> input device.
>
Yes. I'm about to make change it but see the next one.

[snip]
>> diff --git a/net/netfilter/ipvs/ip_vs_xmit.c b/net/netfilter/ipvs/ip_vs_xmit.c
>> index 4527921..32bd3af 100644
>> --- a/net/netfilter/ipvs/ip_vs_xmit.c
>> +++ b/net/netfilter/ipvs/ip_vs_xmit.c
>> @@ -286,7 +286,7 @@ static inline bool decrement_ttl(struct netns_ipvs *ipvs,
>>       {
>>               if (ip_hdr(skb)->ttl <= 1) {
>>                       /* Tell the sender its packet died... */
>> -                     __IP_INC_STATS(net, IPSTATS_MIB_INHDRERRORS);
>> +                     __IP_INC_STATS(net, skb_dst(skb)->dev, IPSTATS_MIB_INHDRERRORS);
>
>         At this point, skb_dst(skb) can be:
>
> - input route at LOCAL_IN => dst->dev is "lo", skb->dev = input_device
> - output route at LOCAL_OUT => dst->dev is output_device, skb->dev = NULL
>
>         We should see this error on LOCAL_IN but better to be
> safe: use 'skb->dev ? : skb_dst(skb)->dev' instead of just
> 'skb_dst(skb)->dev'.
>
This follows v6 implementation in the same function:

#ifdef CONFIG_IP_VS_IPV6
    if (skb_af == AF_INET6) {
        struct dst_entry *dst = skb_dst(skb);

        /* check and decrement ttl */
        if (ipv6_hdr(skb)->hop_limit <= 1) {
            /* Force OUTPUT device used as source address */
            skb->dev = dst->dev;
            icmpv6_send(skb, ICMPV6_TIME_EXCEED,
                    ICMPV6_EXC_HOPLIMIT, 0);
            __IP6_INC_STATS(net, ip6_dst_idev(dst),
                    IPSTATS_MIB_INHDRERRORS);

            return false;
        }

        /* don't propagate ttl change to cloned packets */
        if (!skb_make_writable(skb, sizeof(struct ipv6hdr)))
            return false;

        ipv6_hdr(skb)->hop_limit--;
    } else
#endif

[snip]
>
>         The patch probably has other errors, for example,
> using rt->dst.dev (lo) when rt->dst.error != 0 in ip_error,
> may be 'dev' should be used instead...

Same also here. Examples are ip6_forward and ip6_pkt_drop.

I think it's better be counted in the input device for them also. Thoughts?

Regards,
Stephen.