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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 31 Mar 2020 12:03:01 +0200
From:   Simon Chopin <s.chopin@...halink.fr>
To:     Arnd Bergmann <arnd@...db.de>
Cc:     Networking <netdev@...r.kernel.org>,
        "David S . Miller" <davem@...emloft.net>,
        Guillaume Nault <gnault@...hat.com>
Subject: Re: [PATCH net-next] pppoe: new ioctl to extract per-channel stats

Hello Arnd,

Le 26/03/2020 à 15:31, Arnd Bergmann a écrit :
> On Thu, Mar 26, 2020 at 2:48 PM Simon Chopin <s.chopin@...halink.fr> wrote:
>> Le 26/03/2020 à 11:42, Arnd Bergmann a écrit :
>>> The patch looks fine from from an interface design perspective,
>>> but I wonder if you could use a definition that matches the
>>> structure layout and command number for PPPIOCGL2TPSTATS
>>> exactly, rather than a "very similar mechanism" with a subset
>>> of the fields. You would clearly have to pass down a number of
>>> zero fields, but the implementation could be abstracted at a
>>> higher level later.
>>>
>>>       Arnd
>>
>> This sounds like a good idea, indeed. Is what follows what you had in mind ?
>> I'm not too sure about keeping the chan_priv field in this form, my knowledge
>> of alignment issues being relatively superficial. As I understand it, the matching
>> fields in l2tp_ioc_stats should always be packed to 8 bytes as they fall on natural
>> boundaries, but I might be wrong ?
>>
>> diff --git a/include/uapi/linux/ppp-ioctl.h b/include/uapi/linux/ppp-ioctl.h
>> index a0abc68eceb5..803cbe374fb2 100644
>> --- a/include/uapi/linux/ppp-ioctl.h
>> +++ b/include/uapi/linux/ppp-ioctl.h
>> @@ -79,14 +79,21 @@ struct pppol2tp_ioc_stats {
>>         __aligned_u64   rx_errors;
>>  };
>>
>> -/* For PPPIOCGPPPOESTATS */
>> -struct pppoe_ioc_stats {
>> +struct pppchan_ioc_stats {
>> +       __u8            chan_priv[8];
>>         __aligned_u64   tx_packets;
>>         __aligned_u64   tx_bytes;
>> +       __aligned_u64   tx_errors;
>>         __aligned_u64   rx_packets;
>>         __aligned_u64   rx_bytes;
>> +       __aligned_u64   rx_seq_discards;
>> +       __aligned_u64   rx_oos_packets;
>> +       __aligned_u64   rx_errors;
>>  };
>>
>> +_Static_assert(sizeof(struct pppol2tp_ioc_stats) == sizeof(struct pppchan_ioc_stats), "same size");
>> +_Static_assert((size_t)&((struct pppol2tp_ioc_stats *)0)->tx_packets == (size_t)&((struct pppchan_ioc_stats *)0)->tx_packets, "same offset");
> 
> Conceptually this is what I had in mind, but implementation-wise, I'd suggest
> only having a single structure definition, possibly with a #define like
> 
> #define pppoe_ioc_stats pppchan_ioc_stats
I'm assuming that'd be #define pppol2tp_stats pppchan_ioc_stats ?
> When having two struct definitions, I'd be slightly worried about
> the bitfield causing implementation-defined structure layout,
> so it seems better to only have one definition, even when you cannot
> avoid the bitfield that maybe should not have been used.
>
>>  /*
>>   * Ioctl definitions.
>>   */
>> @@ -123,7 +130,7 @@ struct pppoe_ioc_stats {
>>  #define PPPIOCATTCHAN  _IOW('t', 56, int)      /* attach to ppp channel */
>>  #define PPPIOCGCHAN    _IOR('t', 55, int)      /* get ppp channel number */
>>  #define PPPIOCGL2TPSTATS _IOR('t', 54, struct pppol2tp_ioc_stats)
>> -#define PPPIOCGPPPOESTATS _IOR('t', 53, struct pppoe_ioc_stats)
>> +#define PPPIOCGCHANSTATS _IOR('t', 54, struct pppchan_ioc_stats)
> 
> here I'd do
> 
> #define PPPIOCGCHANSTATS _IOR('t', 54, struct pppchan_ioc_stats)
> #define PPPIOCGL2TPSTATS PPPIOCGCHANSTATS

Thank you for your feedback. I'm probably going to implement a more
generic version at the generic PPP channel instead, though, as,
as noted by Guillaume, those statistics are not for the PPP channel
but for the layer underneath.

However, I'd like to be sure I understand your proposal here :
we'd use a generic pppchan_ioc_stats struct that would be identical
to the current pppol2tp_ioc_stats, including the 3 L2TP-specific fields,
so that we'd retain ABI and API compatibility, and we would simply
#define the current API to the new one?

Cheers,
Simon