Re: [PATCH net-next] pppoe: new ioctl to extract per-channel stats

[Guillaume Nault <gnault@redhat.com>]

On Thu, Mar 26, 2020 at 11:32:30AM +0100, Simon Chopin wrote:
> The PPP subsystem uses the abstractions of channels and units, the
> latter being an aggregate of the former, exported to userspace as a
> single network interface.  As such, it keeps traffic statistics at the
> unit level, but there are no statistics on the individual channels,
> partly because most PPP units only have one channel.
> 
> However, it is sometimes useful to have statistics at the channel level,
> for instance to monitor multilink PPP connections. Such statistics
> already exist for PPPoL2TP via the PPPIOCGL2TPSTATS ioctl, this patch
> introduces a very similar mechanism for PPPoE via a new
> PPPIOCGPPPOESTATS ioctl.
> 
I'd rather recomment _not_ using multilink PPP over PPPoE (or L2TP, or
any form of overlay network). But apart from that, I find the
description misleading. PPPoE is not a PPP channel, it _transports_ a
channel. PPPoE might not even be associated with a channel at all,
like in the PPPOX_RELAY case. In short PPPoE stats aren't channel's
stats. If the objective it to get channels stats, then this needs to be
implemented in ppp_generic.c. If what you really want is PPPoE stats,
then see my comments below.

> @@ -395,6 +405,10 @@ static int pppoe_rcv_core(struct sock *sk, struct sk_buff *skb)
>  			goto abort_kfree;
>  	}
>  
> +	stats = sk_pppox(po)->sk_user_data;
> +	atomic_long_inc(&stats->rx_packets);
> +	atomic_long_add(len, &stats->rx_bytes);
> +
>  	return NET_RX_SUCCESS;
>  
You probably need to add counter(s) for the error paths too.

> @@ -549,6 +563,8 @@ static int pppoe_create(struct net *net, struct socket *sock, int kern)
>  	sk->sk_family		= PF_PPPOX;
>  	sk->sk_protocol		= PX_PROTO_OE;
>  
> +	sk->sk_user_data = kzalloc(sizeof(struct pppoe_stats), GFP_KERNEL);
> +
Missing error check.

But please don't use ->sk_user_data for that. We have enough problems
with this pointer, let's not add users that don't actually need it.
See https://lore.kernel.org/netdev/20180117.142538.1972806008716856078.davem@davemloft.net/
for some details.
You can store the counters inside the socket instead.

> @@ -950,6 +993,8 @@ static int __pppoe_xmit(struct sock *sk, struct sk_buff *skb)
>  			po->pppoe_pa.remote, NULL, data_len);
>  
>  	dev_queue_xmit(skb);
> +	atomic_long_inc(&stats->tx_packets);
> +	atomic_long_add(data_len, &stats->tx_bytes);
>  	return 1;
>  
Again, you probably need to count errors too.