On Mon, Jul 06, 2020 at 01:15:09PM +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.
>
> 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 | 64 ++++++++++-----------
> drivers/usb/mon/mon_bin.c | 92 +-----------------------------
> include/uapi/linux/usb/mon.h | 105 +++++++++++++++++++++++++++++++++++
> 3 files changed, 139 insertions(+), 122 deletions(-)
> create mode 100644 include/uapi/linux/usb/mon.h
>
> diff --git a/Documentation/usb/usbmon.rst b/Documentation/usb/usbmon.rst
> index b0bd51080799..da18c5543b2f 100644
> --- a/Documentation/usb/usbmon.rst
> +++ b/Documentation/usb/usbmon.rst
> @@ -211,36 +211,38 @@ 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)::
> -
> - struct usbmon_packet {
> - 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) */
> - unsigned char epnum; /* Endpoint number and transfer direction */
> - unsigned char devnum; /* Device address */
> - u16 busnum; /* 12: Bus number */
> - char flag_setup; /* 14: Same as text */
> - char flag_data; /* 15: Same as text; Binary zero is OK. */
> - s64 ts_sec; /* 16: gettimeofday */
> - s32 ts_usec; /* 24: gettimeofday */
> - int status; /* 28: */
> - unsigned int length; /* 32: Length of data (submitted or actual) */
> - unsigned int len_cap; /* 36: Delivered length */
> - union { /* 40: */
> - unsigned char setup[SETUP_LEN]; /* Only for Control S-type */
> +The overall architecture of the API is about the same as the one above, only the
Why change that line?
> +events are delivered in binary format. The structures and constants are defined
> +in linux/usb/mon.h.
Not your new uapi file location?
> +
> +Each event is sent in the following structure::
> +
> + struct mon_bin_hdr {
> + u64 id; /* 0: URB ID - from submission to callback */
__u64, right? Same for the rest of this file...
> + u8 type; /* 8: Same as text; extensible. */
> + u8 xfer_type; /* ISO (0), Intr, Control, Bulk (3) */
> + u8 epnum; /* Endpoint number and transfer direction */
> + u8 devnum; /* Device address */
> + u16 busnum; /* 12: Bus number */
> + s8 flag_setup; /* 14: Same as text */
> + s8 flag_data; /* 15: Same as text; Binary zero is OK. */
> + s64 ts_sec; /* 16: gettimeofday */
> + s32 ts_usec; /* 24: gettimeofday */
> + s32 status; /* 28: */
> + u32 length; /* 32: Length of data (submitted or actual) */
> + u32 len_cap; /* 36: Delivered length */
> + union { /* 40: */
> + u8 setup[MON_USB_SETUP_LEN]; /* Only for Control S-type */
> struct iso_rec { /* Only for ISO */
> - int error_count;
> - int numdesc;
> + s32 error_count;
> + s32 numdesc;
> } iso;
> } s;
> - int interval; /* 48: Only for Interrupt and ISO */
> - int start_frame; /* 52: For ISO */
> - unsigned int xfer_flags; /* 56: copy of URB's transfer_flags */
> - unsigned int ndesc; /* 60: Actual number of ISO descriptors */
> - }; /* 64 total length */
> + s32 interval; /* 48: Only for Interrupt and ISO */
> + s32 start_frame;/* 52: For ISO */
> + u32 xfer_flags; /* 56: copy of URB's transfer_flags */
> + u32 ndesc; /* 60: Actual number of ISO descriptors */
> + }; /* 64 total length */
>
> These events can be received from a character device by reading with read(2),
> with an ioctl(2), or by accessing the buffer with mmap. However, read(2)
> @@ -313,9 +315,9 @@ This ioctl is primarily used when the application accesses the buffer
> with mmap(2). Its argument is a pointer to the following structure::
>
> struct mon_mfetch_arg {
> - uint32_t *offvec; /* Vector of events fetched */
> - uint32_t nfetch; /* Number of events to fetch (out: fetched) */
> - uint32_t nflush; /* Number of events to flush */
> + u32 *offvec; /* Vector of events fetched */
> + u32 nfetch; /* Number of events to fetch (out: fetched) */
> + u32 nflush; /* Number of events to flush */
Again, __u32
> };
>
> The ioctl operates in 3 stages.
> @@ -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:
> -
Why drop this? If you want to clean up the documentation, wonderful,
but again, don't do that in the same patch.
Remember, each patch can only do ONE thing.
> 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..e1a7e69a3b0c 100644
> --- a/drivers/usb/mon/mon_bin.c
> +++ b/drivers/usb/mon/mon_bin.c
> @@ -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
> @@ -396,7 +308,7 @@ static inline char mon_bin_get_setup(unsigned char *setupb,
>
> if (urb->setup_packet == NULL)
> return 'Z';
> - memcpy(setupb, urb->setup_packet, SETUP_LEN);
> + memcpy(setupb, urb->setup_packet, MON_USB_SETUP_LEN);
> return 0;
> }
>
> diff --git a/include/uapi/linux/usb/mon.h b/include/uapi/linux/usb/mon.h
> new file mode 100644
> index 000000000000..376f93acdde2
> --- /dev/null
> +++ b/include/uapi/linux/usb/mon.h
> @@ -0,0 +1,105 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +/*
> + * USB Monitoring (usbmon) definitions
> + *
> + * See documentation in Documentation/usb/usbmon.rst.
> + *
> + * Copyright (C) 2006 Paolo Abeni (paolo.abeni@xxxxxxxx)
> + * Copyright (C) 2006,2007 Pete Zaitcev (zaitcev@xxxxxxxxxx)
> + */
> +
> +#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)
> +
> +/* ioctl structures */
> +
> +/* 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) */
is size_t a value we can pass across user/kernel boundry? Are you sure
this isn't __kernel_size_t?
thanks,
greg k-h
