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: <alpine.LFD.2.20.1804112208190.2618@ja.home.ssi.bg>
Date:   Wed, 11 Apr 2018 22:37:07 +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 Tue, 10 Apr 2018, Stephen Suryaputra wrote:

> This is enhanced from the proposed patch by Igor Maravic in 2011 to
> support per interface IPv4 stats. The enhancement is mainly adding a
> kernel configuration option CONFIG_IP_IFSTATS_TABLE.
> 
> Signed-off-by: Stephen Suryaputra <ssuryaextr@...il.com>
> ---

...

> diff --git a/net/ipv4/ip_forward.c b/net/ipv4/ip_forward.c
> index b54b948..bb9be11 100644
> --- a/net/ipv4/ip_forward.c
> +++ b/net/ipv4/ip_forward.c
> @@ -66,8 +66,8 @@ static int ip_forward_finish(struct net *net, struct sock *sk, struct sk_buff *s
>  {
>  	struct ip_options *opt	= &(IPCB(skb)->opt);
>  
> -	__IP_INC_STATS(net, IPSTATS_MIB_OUTFORWDATAGRAMS);
> -	__IP_ADD_STATS(net, IPSTATS_MIB_OUTOCTETS, skb->len);
> +	__IP_INC_STATS(net, skb_dst(skb)->dev, IPSTATS_MIB_OUTFORWDATAGRAMS);
> +	__IP_ADD_STATS(net, skb_dst(skb)->dev, IPSTATS_MIB_OUTOCTETS, skb->len);
>  
>  	if (unlikely(opt->optlen))
>  		ip_forward_options(skb);
> @@ -121,7 +121,7 @@ int ip_forward(struct sk_buff *skb)
>  	IPCB(skb)->flags |= IPSKB_FORWARDED;
>  	mtu = ip_dst_mtu_maybe_forward(&rt->dst, true);
>  	if (ip_exceeds_mtu(skb, mtu)) {
> -		IP_INC_STATS(net, IPSTATS_MIB_FRAGFAILS);
> +		IP_INC_STATS(net, rt->dst.dev, IPSTATS_MIB_FRAGFAILS);
>  		icmp_send(skb, ICMP_DEST_UNREACH, ICMP_FRAG_NEEDED,
>  			  htonl(mtu));
>  		goto drop;
> @@ -158,7 +158,7 @@ int ip_forward(struct sk_buff *skb)
>  
>  too_many_hops:
>  	/* Tell the sender its packet died... */
> -	__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.

>  	icmp_send(skb, ICMP_TIME_EXCEEDED, ICMP_EXC_TTL, 0);
>  drop:
>  	kfree_skb(skb);

...

> 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'.

>  			icmp_send(skb, ICMP_TIME_EXCEEDED, ICMP_EXC_TTL, 0);
>  			return false;
>  		}

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

Regards

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