On Sun, Jul 05, 2020 at 04:02:25PM +0100, Diego Elio Pettenò wrote:
> Previously any application wanting to implement the usbmon binary
> interfaces needed to re-declare the structures and constants, leading to
> structure duplication and confusion over whether these structures fall into
> the system call exception or not.
>
> Also update Paolo's address to match `net/core/dst_cache.c` as the previous
> one bounces.
When you say "also" that means it should be a separate patch. Please do
that for this one.
>
> Cc: linux-usb@xxxxxxxxxxxxxxx
> Cc: Pete Zaitcev <zaitcev@xxxxxxxxxx>
> Cc: Paolo Abeni <pabeni@xxxxxxxxxx>
> Cc: Kris Katterjohn <katterjohn@xxxxxxxxx>
> Cc: Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Diego Elio Pettenò <flameeyes@xxxxxxxxxxxxx>
> ---
> Documentation/usb/usbmon.rst | 12 ++---
> drivers/usb/mon/mon_bin.c | 94 ++------------------------------
> include/uapi/linux/usb/mon.h | 102 +++++++++++++++++++++++++++++++++++
> 3 files changed, 111 insertions(+), 97 deletions(-)
> create mode 100644 include/uapi/linux/usb/mon.h
>
> diff --git a/Documentation/usb/usbmon.rst b/Documentation/usb/usbmon.rst
> index b0bd51080799..cf98ea553ba1 100644
> --- a/Documentation/usb/usbmon.rst
> +++ b/Documentation/usb/usbmon.rst
> @@ -211,11 +211,13 @@ Bulk wrapper to a storage device at address 5::
> Raw binary format and API
> =========================
>
> -The overall architecture of the API is about the same as the one above,
> -only the events are delivered in binary format. Each event is sent in
> -the following structure (its name is made up, so that we can refer to it)::
> +The overall architecture of the API is about the same as the one above, only the
> +events are delivered in binary format. The structures and constants are defined
> +in linux/usb/mon.h.
>
> - struct usbmon_packet {
> +Each event is sent in the following structure::
> +
> + struct mon_bin_hdr {
> u64 id; /* 0: URB ID - from submission to callback */
> unsigned char type; /* 8: Same as text; extensible. */
> unsigned char xfer_type; /* ISO (0), Intr, Control, Bulk (3) */
> @@ -346,8 +348,6 @@ be polled with select(2) and poll(2). But lseek(2) does not work.
>
> * Memory-mapped access of the kernel buffer for the binary API
>
> -The basic idea is simple:
> -
> To prepare, map the buffer by getting the current size, then using mmap(2).
> Then, execute a loop similar to the one written in pseudo-code below::
>
> diff --git a/drivers/usb/mon/mon_bin.c b/drivers/usb/mon/mon_bin.c
> index f48a23adbc35..e914fd8d039e 100644
> --- a/drivers/usb/mon/mon_bin.c
> +++ b/drivers/usb/mon/mon_bin.c
> @@ -4,8 +4,8 @@
> *
> * This is a binary format reader.
> *
> - * Copyright (C) 2006 Paolo Abeni (paolo.abeni@xxxxxxxx)
> - * Copyright (C) 2006,2007 Pete Zaitcev (zaitcev@xxxxxxxxxx)
> + * Copyright (C) 2006 Paolo Abeni <pabeni@xxxxxxxxxx>
You can't change someone's copyright lines, sorry.
> + * Copyright (C) 2006,2007 Pete Zaitcev <zaitcev@xxxxxxxxxx>
Why this change?
> */
>
> #include <linux/kernel.h>
> @@ -23,34 +23,10 @@
> #include <linux/time64.h>
>
> #include <linux/uaccess.h>
> +#include <linux/usb/mon.h>
>
> #include "usb_mon.h"
>
> -/*
> - * Defined by USB 2.0 clause 9.3, table 9.2.
> - */
> -#define SETUP_LEN 8
> -
> -/* ioctl macros */
> -#define MON_IOC_MAGIC 0x92
> -
> -#define MON_IOCQ_URB_LEN _IO(MON_IOC_MAGIC, 1)
> -/* #2 used to be MON_IOCX_URB, removed before it got into Linus tree */
> -#define MON_IOCG_STATS _IOR(MON_IOC_MAGIC, 3, struct mon_bin_stats)
> -#define MON_IOCT_RING_SIZE _IO(MON_IOC_MAGIC, 4)
> -#define MON_IOCQ_RING_SIZE _IO(MON_IOC_MAGIC, 5)
> -#define MON_IOCX_GET _IOW(MON_IOC_MAGIC, 6, struct mon_bin_get)
> -#define MON_IOCX_MFETCH _IOWR(MON_IOC_MAGIC, 7, struct mon_bin_mfetch)
> -#define MON_IOCH_MFLUSH _IO(MON_IOC_MAGIC, 8)
> -/* #9 was MON_IOCT_SETAPI */
> -#define MON_IOCX_GETX _IOW(MON_IOC_MAGIC, 10, struct mon_bin_get)
> -
> -#ifdef CONFIG_COMPAT
> -#define MON_IOCX_GET32 _IOW(MON_IOC_MAGIC, 6, struct mon_bin_get32)
> -#define MON_IOCX_MFETCH32 _IOWR(MON_IOC_MAGIC, 7, struct mon_bin_mfetch32)
> -#define MON_IOCX_GETX32 _IOW(MON_IOC_MAGIC, 10, struct mon_bin_get32)
> -#endif
> -
> /*
> * Some architectures have enormous basic pages (16KB for ia64, 64KB for ppc).
> * But it's all right. Just use a simple way to make sure the chunk is never
> @@ -81,38 +57,6 @@
> #define BUFF_DFL CHUNK_ALIGN(300*1024)
> #define BUFF_MIN CHUNK_ALIGN(8*1024)
>
> -/*
> - * The per-event API header (2 per URB).
> - *
> - * This structure is seen in userland as defined by the documentation.
> - */
> -struct mon_bin_hdr {
> - u64 id; /* URB ID - from submission to callback */
> - unsigned char type; /* Same as in text API; extensible. */
> - unsigned char xfer_type; /* ISO, Intr, Control, Bulk */
> - unsigned char epnum; /* Endpoint number and transfer direction */
> - unsigned char devnum; /* Device address */
> - unsigned short busnum; /* Bus number */
> - char flag_setup;
> - char flag_data;
> - s64 ts_sec; /* ktime_get_real_ts64 */
> - s32 ts_usec; /* ktime_get_real_ts64 */
> - int status;
> - unsigned int len_urb; /* Length of data (submitted or actual) */
> - unsigned int len_cap; /* Delivered length */
> - union {
> - unsigned char setup[SETUP_LEN]; /* Only for Control S-type */
> - struct iso_rec {
> - int error_count;
> - int numdesc;
> - } iso;
> - } s;
> - int interval;
> - int start_frame;
> - unsigned int xfer_flags;
> - unsigned int ndesc; /* Actual number of ISO descriptors */
> -};
> -
> /*
> * ISO vector, packed into the head of data stream.
> * This has to take 16 bytes to make sure that the end of buffer
> @@ -125,38 +69,6 @@ struct mon_bin_isodesc {
> u32 _pad;
> };
>
> -/* per file statistic */
> -struct mon_bin_stats {
> - u32 queued;
> - u32 dropped;
> -};
> -
> -struct mon_bin_get {
> - struct mon_bin_hdr __user *hdr; /* Can be 48 bytes or 64. */
> - void __user *data;
> - size_t alloc; /* Length of data (can be zero) */
> -};
> -
> -struct mon_bin_mfetch {
> - u32 __user *offvec; /* Vector of events fetched */
> - u32 nfetch; /* Number of events to fetch (out: fetched) */
> - u32 nflush; /* Number of events to flush */
> -};
> -
> -#ifdef CONFIG_COMPAT
> -struct mon_bin_get32 {
> - u32 hdr32;
> - u32 data32;
> - u32 alloc32;
> -};
> -
> -struct mon_bin_mfetch32 {
> - u32 offvec32;
> - u32 nfetch32;
> - u32 nflush32;
> -};
> -#endif
> -
> /* Having these two values same prevents wrapping of the mon_bin_hdr */
> #define PKT_ALIGN 64
> #define PKT_SIZE 64
> diff --git a/include/uapi/linux/usb/mon.h b/include/uapi/linux/usb/mon.h
> new file mode 100644
> index 000000000000..265e0169e2ef
> --- /dev/null
> +++ b/include/uapi/linux/usb/mon.h
> @@ -0,0 +1,102 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +/*
> + * USB Monitoring (usbmon) definitions
Put the copyright notices back in here.
> + */
> +
> +#ifndef __UAPI_LINUX_USB_MON_H
> +#define __UAPI_LINUX_USB_MON_H
> +
> +/* ioctl macros */
> +#define MON_IOC_MAGIC 0x92
> +
> +#define MON_IOCQ_URB_LEN _IO(MON_IOC_MAGIC, 1)
> +/* #2 used to be MON_IOCX_URB, removed before it got into Linus tree */
> +#define MON_IOCG_STATS _IOR(MON_IOC_MAGIC, 3, struct mon_bin_stats)
> +#define MON_IOCT_RING_SIZE _IO(MON_IOC_MAGIC, 4)
> +#define MON_IOCQ_RING_SIZE _IO(MON_IOC_MAGIC, 5)
> +#define MON_IOCX_GET _IOW(MON_IOC_MAGIC, 6, struct mon_bin_get)
> +#define MON_IOCX_MFETCH _IOWR(MON_IOC_MAGIC, 7, struct mon_bin_mfetch)
> +#define MON_IOCH_MFLUSH _IO(MON_IOC_MAGIC, 8)
> +/* #9 was MON_IOCT_SETAPI */
> +#define MON_IOCX_GETX _IOW(MON_IOC_MAGIC, 10, struct mon_bin_get)
> +
> +#ifdef CONFIG_COMPAT
> +#define MON_IOCX_GET32 _IOW(MON_IOC_MAGIC, 6, struct mon_bin_get32)
> +#define MON_IOCX_MFETCH32 _IOWR(MON_IOC_MAGIC, 7, struct mon_bin_mfetch32)
> +#define MON_IOCX_GETX32 _IOW(MON_IOC_MAGIC, 10, struct mon_bin_get32)
> +#endif
Alignment?
And you see the kbuild error for this, please fix that up.
> +
> +/* ioctl structures */
> +
> +/* per file statistic */
> +struct mon_bin_stats {
> + u32 queued;
> + u32 dropped;
> +};
As you are making this a "real" uapi .h file, you have to use the "real"
data types as well. That means using __u32 and not u32. Same for all
others.
> +
> +struct mon_bin_get {
> + struct mon_bin_hdr __user *hdr; /* Can be 48 bytes or 64. */
> + void __user *data;
Does this cross the user/kernel boundry? Ick, no wonder we need compat
fixups :(
> + size_t alloc; /* Length of data (can be zero) */
> +};
> +
> +struct mon_bin_mfetch {
> + u32 __user *offvec; /* Vector of events fetched */
> + u32 nfetch; /* Number of events to fetch (out: fetched) */
> + u32 nflush; /* Number of events to flush */
> +};
> +
> +#ifdef CONFIG_COMPAT
> +struct mon_bin_get32 {
> + u32 hdr32;
> + u32 data32;
> + u32 alloc32;
> +};
> +
> +struct mon_bin_mfetch32 {
> + u32 offvec32;
> + u32 nfetch32;
> + u32 nflush32;
> +};
> +#endif
> +
> +/* Data format */
> +
> +/*
> + * Defined by USB 2.0 clause 9.3, table 9.2.
> + */
> +#define SETUP_LEN 8
Horrible global define, please use USB_MON_SETUP_LEN or something like
that.
> +
> +/*
> + * The per-event API header (2 per URB).
> + *
> + * This structure is seen in userland as defined by the documentation.
> + */
> +struct mon_bin_hdr {
> + u64 id; /* URB ID - from submission to callback */
> + unsigned char type; /* Same as in text API; extensible. */
> + unsigned char xfer_type; /* ISO, Intr, Control, Bulk */
> + unsigned char epnum; /* Endpoint number and transfer direction */
> + unsigned char devnum; /* Device address */
> + unsigned short busnum; /* Bus number */
> + char flag_setup;
> + char flag_data;
> + s64 ts_sec; /* ktime_get_real_ts64 */
> + s32 ts_usec; /* ktime_get_real_ts64 */
> + int status;
> + unsigned int len_urb; /* Length of data (submitted or actual) */
> + unsigned int len_cap; /* Delivered length */
Again, correct types please.
thanks,
greg k-h
