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] [day] [month] [year] [list]

Message-ID: <alpine.LFD.2.20.1804122204480.1827@ja.home.ssi.bg>
Date:   Thu, 12 Apr 2018 22:48:54 +0300 (EEST)
From:   Julian Anastasov <ja@....bg>
To:     Stephen Suryaputra <ssuryaextr@...il.com>
cc:     netdev@...r.kernel.org
Subject: Re: [PATCH net-next] Per interface IPv4 stats
 (CONFIG_IP_IFSTATS_TABLE)


	Hello,

On Thu, 12 Apr 2018, Stephen Suryaputra wrote:

> 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 */

	It looks like IPVS copied it from ip6_forward() but in
IPVS context it has its reason: we want ICMP to exit with
saddr=Virtual_IP. And we are at LOCAL_IN where there is no
output device like in ip6_forward(FORWARD) to use its source
address.

	So, IPVS is special (both input and output path) and needs:

IPv4: skb->dev ? : skb_dst(skb)->dev
IPv6 needs fix for IPVS stats in decrement_ttl:

	idev = skb->dev ? __in6_dev_get(skb->dev) : ip6_dst_idev(dst);
	...
	__IP6_INC_STATS(net, idev, IPSTATS_MIB_INHDRERRORS);

	Otherwise, stats will go to "lo" if ip6_dst_idev
is used for local input route.

	So, for accounting on input IPv4 path skb->dev should be
used, while for IPv6 some sites may prefer to feed icmpv6_send()
with output dst->dev as device containing the source address (skb->dev).
But this is unrelated to the stats.

>             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?

	I think so. ipv6_rcv() works with idev = __in6_dev_get(skb->dev)
but I don't know IPv6 well and whether ip6_dst_idev(skb_dst(skb))
is correct usage for input path. It should be correct for output
path, though.

Regards

--
Julian Anastasov <ja@....bg>