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]

Date:   Thu, 26 Mar 2020 15:31:45 +0100
From:   Arnd Bergmann <arnd@...db.de>
To:     Simon Chopin <s.chopin@...halink.fr>
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

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

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

       Arnd