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

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: <629de420-b433-0868-eccc-6feb9b43da7c@alphalink.fr>
Date:   Tue, 31 Mar 2020 11:27:23 +0200
From:   Simon Chopin <s.chopin@...halink.fr>
To:     Guillaume Nault <gnault@...hat.com>
Cc:     netdev@...r.kernel.org, "David S . Miller" <davem@...emloft.net>,
        Arnd Bergmann <arnd@...db.de>
Subject: Re: [PATCH net-next] pppoe: new ioctl to extract per-channel stats

Hello Guillaume,

Le 26/03/2020 à 15:38, Guillaume Nault a écrit :
> 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.

Thank you for your feedback
I indeed want some statistics over PPP channels, notably over L2TP and
PPPoE. Since a mechanism already existed for the former, I thought
it simpler to implement the same for the latter, but your point makes sense:
those subsystems operate below the PPP layer, with extra control packets
and header overhead.

<snip>

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

Thank you for the pointers. I'll pay attention to error paths in any further
version, and in any case will drop the sk_user_data use.

Would it be allright to post new patches as RFC before net-next opens in
order to get further feedback, or is that frowned upon ?

Cheers,
Simon