| Message ID | 20200705150225.21500-1-flameeyes@flameeyes.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usbmon: expose the usbmon structures and constants as an UAPI header. | expand |
On Sun, 5 Jul 2020 16:02:25 +0100 Diego Elio Pettenò <flameeyes@flameeyes.com> 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. I am not sure I follow the logic in the confusion claim above. Since the applications define their own headers, they do not use the kernel source at all. Therefore, there can be no question about any exceptions, and no confusion. But perhaps I'm missing a turn here. Feel free to educate me. As for the "duplication", I do not see it as harmful in any way. And they do not re-declare, they declare. You're trying to create a centralized language definition that did not exist previously. > +++ b/Documentation/usb/usbmon.rst > - struct usbmon_packet { > + struct mon_bin_hdr { > u64 id; /* 0: URB ID - from submission to callback */ It was named that specifically in order to underscore that it's not the actual code. > +++ b/include/uapi/linux/usb/mon.h > +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */ Yeah, that. You created the confusion first, and then resolved it with licensing amendments. > +/* ioctl macros */ > +#define MON_IOC_MAGIC 0x92 > + > +#define MON_IOCQ_URB_LEN _IO(MON_IOC_MAGIC, 1) Okay. Our documentation refers to _IO(), which is a system macro, so we're not entirely self-containing. I can see an opening for some sophistry here. Fortunately, include/uapi/asm-generic/ioctl.h already includes that "GPL-2.0 WITH Linux-syscall-note" thing, so I think we're clear in usbmon. -- Pete
Hi "Diego,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on usb/usb-testing]
[also build test ERROR on v5.8-rc4 next-20200703]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Diego-Elio-Petten/usbmon-expose-the-usbmon-structures-and-constants-as-an-UAPI-header/20200705-230320
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
config: powerpc-randconfig-r026-20200706 (attached as .config)
compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project a378c0449507e00e96534ff9ce9034e185425182)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install powerpc cross compiling tool for clang build
# apt-get install binutils-powerpc-linux-gnu
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=powerpc
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
>> error: include/uapi/linux/usb/mon.h: leak CONFIG_COMPAT to user-space
make[2]: *** [scripts/Makefile.headersinst:63: usr/include/linux/usb/mon.h] Error 1
make[2]: Target '__headers' not remade because of errors.
make[1]: *** [Makefile:1258: headers] Error 2
make[1]: Target 'prepare' not remade because of errors.
make: *** [Makefile:185: __sub-make] Error 2
make: Target 'prepare' not remade because of errors.
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On Mon, Jul 6, 2020, at 04:51, Pete Zaitcev wrote: > I am not sure I follow the logic in the confusion claim above. Since the > applications define their own headers, they do not use the kernel source > at all. Therefore, there can be no question about any exceptions, and no > confusion. But perhaps I'm missing a turn here. Feel free to educate me. Most applications rely on one or another kernel header — this has been the norm for Linux kernel interface users for many years. The UAPI headers are installed on systems and wrapped explicitly for this reason. > As for the "duplication", I do not see it as harmful in any way. > And they do not re-declare, they declare. You're trying to create a > centralized language definition that did not exist previously. It is harmful, because these structures *need* to be exactly synchronised or the calls will fail. Or the data will be garbage. Not exposing them on UAPI just doesn't make it as obvious, but it is an issue needing to be addressed. Furthermore there's the problem of whether having to pick up the structures that are defined in a piece of source code that is purported to not be covered by the syscall exception — that would cause require making the userspace implementations of usbmon a derived work of the kernel, which in turn would make them GPL-2. > > +++ b/include/uapi/linux/usb/mon.h > > +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */ > > Yeah, that. You created the confusion first, and then resolved it > with licensing amendments. I didn't create the confusion. I stumbled across the confusion. Either the structure definition is not covered by the syscall exception, or it isn't. If it is covered, then we're all good: anyone can include the header, or copy it, or copy part of it, or redefine the structures in whichever language they prefer, and it's not a derived work of the kernel. If it is, then we have a bit of an issue, as libpcap is not considering itself a derived work of Linux and contains a copy of the same structures. So either we say that Paolo had the ability to release it under both licenses, or we're going to give a headache to libpcap. If we accept that Paolo released these structures under both licenses, then the syscall exception can be applied, if nothing else by sleight-of-hand by taking the MIT licensed version and restricting it. > Okay. Our documentation refers to _IO(), which is a system macro, > so we're not entirely self-containing. I can see an opening for > some sophistry here. > > Fortunately, include/uapi/asm-generic/ioctl.h already includes > that "GPL-2.0 WITH Linux-syscall-note" thing, so I think we're > clear in usbmon. You _also_ need the structure definition, or at least its size, for the _IO macro to be usable. Which is why it's important that syscall-level interfaces are made available in UAPI headers with exceptions. Now that does mean that mon_bin_hdr doesn't strictly need to be there…. but what's the point of not defining the structure that you _need_ to have to be able to read the data you're requesting? So in short as for "Why all this?" Because without this change, making a new userspace implementation of usbmon requires straight-out copying code from the kernel in not-quite-clearcut licensing environment. Exposing this on an UAPI header should address most of that, without tying your hands any more than already present for compatibility, and without diluting the licensing of the structures more than it is right now. Unless you plan to argue that libpcap is violating the kernel license, but I think that'd be making usbmon toxic enough that I wouldn't be the only one walking away. Anything else, beside not feeling that we need this? Diego
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@vger.kernel.org > Cc: Pete Zaitcev <zaitcev@redhat.com> > Cc: Paolo Abeni <pabeni@redhat.com> > Cc: Kris Katterjohn <katterjohn@gmail.com> > Cc: Greg KH <gregkh@linuxfoundation.org> > Signed-off-by: Diego Elio Pettenò <flameeyes@flameeyes.com> > --- > 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@email.it) > - * Copyright (C) 2006,2007 Pete Zaitcev (zaitcev@redhat.com) > + * Copyright (C) 2006 Paolo Abeni <pabeni@redhat.com> You can't change someone's copyright lines, sorry. > + * Copyright (C) 2006,2007 Pete Zaitcev <zaitcev@redhat.com> 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
On Mon, Jul 6, 2020, at 11:30, Greg KH wrote: > You can't change someone's copyright lines, sorry. Then I hope Paolo can send a follow-up patch updating his email address — email.it doesn't seem to exist anymore/doesn't seem to have his address anymore. > Put the copyright notices back in here. Done. > And you see the kbuild error for this, please fix that up. This should be fixed now by dropping the conditional, which AIUI is the right thing to do in this case? I couldn't find any documentation referencing what to do with CONFIG_COMPAT for headers, but I assume that anything optionally available should be exposed in UAPI and worst case fail at runtime. > 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. Done, and in the documentation. > Does this cross the user/kernel boundry? Ick, no wonder we need compat > fixups :( I fear so. The usbmon interface is fairly long-living, and I don't think it matches most of the best practices of today. I keep telling myself I should spend more time trying to figure out what I can improve overall, because I fear this is better served by having a good thought on the design, and be replaced. But that's a topic for another thread. Although if someone is interested in whiteboarding an alternative design, I'd be happy to start such a thread or have an offline chat. My current castle-in-the-sky would involve integrating BPF so that capture filters can be applied — and I think that the modern way to stream data from the kernel to userspace would be netlink?
On Mon, 06 Jul 2020 09:32:09 +0100 Diego Elio Pettenò <flameeyes@flameeyes.com> wrote: > Most applications rely on one or another kernel header — this has been > the norm for Linux kernel interface users for many years. Fine, I never said what you're proposing is without precedent. > > As for the "duplication", I do not see it as harmful in any way. > > And they do not re-declare, they declare. You're trying to create a > > centralized language definition that did not exist previously. > > It is harmful, because these structures *need* to be exactly > synchronised or the calls will fail. Or the data will be garbage. They are already synchronized today. No calls are failing and data isn't garbage. So you're not proving anything here. > Either the structure definition is not covered by the syscall exception, > or it isn't. > If it is, then we have a bit of an issue, as libpcap is not considering > itself a derived work of Linux and contains a copy of the same structures. > [...] You obviously mised "not", but hey. The important part is, do not see Paolo doing anything wrong. He has his own definitions, and if he chose to use the same field names, it's complately fine by me, as a copyright holder. Your attempts to drag libpcap into this are laughable. > You _also_ need the structure definition, or at least its size, > for the _IO macro to be usable. The question of _IO is a fine point. However, since the handcrafted application level structs are guaranteed to be the same size, it seems moot to me. > Now that does mean that mon_bin_hdr doesn't strictly need to be > there…. but what's the point of not defining the structure that > you _need_ to have to be able to read the data you're requesting? The point is to avoid licensing complications. Usbmon is much older than "syscall exception". > So in short as for "Why all this?" > > Because without this change, making a new userspace implementation > of usbmon requires straight-out copying code from the kernel in > not-quite-clearcut licensing environment. False. Making a new application for usbmon requires reading the documentation. It does not requires copying any code from the kernel. It is your choice to copy that code. I see that you got Greg to agree to your change in principle, so I am okay with providing a fixed binary struct for your convenience. Post a patch that satisfies him and I'll ack it. But please leave the falsehoods about structs being necessary out of it. Just don't put them into comments or the commit message. Can you do at least that much? -- Pete
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@email.it) - * Copyright (C) 2006,2007 Pete Zaitcev (zaitcev@redhat.com) + * Copyright (C) 2006 Paolo Abeni <pabeni@redhat.com> + * Copyright (C) 2006,2007 Pete Zaitcev <zaitcev@redhat.com> */ #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 + */ + +#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 + +/* 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) */ +}; + +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 + +/* + * 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 */ +}; + +#endif /* __UAPI_LINUX_USB_MON_H */
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. Cc: linux-usb@vger.kernel.org Cc: Pete Zaitcev <zaitcev@redhat.com> Cc: Paolo Abeni <pabeni@redhat.com> Cc: Kris Katterjohn <katterjohn@gmail.com> Cc: Greg KH <gregkh@linuxfoundation.org> Signed-off-by: Diego Elio Pettenò <flameeyes@flameeyes.com> --- 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