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: <20200331104901.GA24576@pc-3.home>
Date:   Tue, 31 Mar 2020 12:49:01 +0200
From:   Guillaume Nault <gnault@...hat.com>
To:     Simon Chopin <s.chopin@...halink.fr>
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

On Tue, Mar 31, 2020 at 11:27:23AM +0200, Simon Chopin wrote:
> 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.
> 
Then I guess getting statistics of the PPP channel is more appropriate.
It might be possible to move the ppp_link_stats structure from
struct ppp to struct ppp_file, so that it could be used for channels
and for units. If necessary, the structure can probably be extended to
record more statistics.

> >> @@ -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 ?
> 
It should be fine, as long as it's clearly indicated in the subject.