-
Notifications
You must be signed in to change notification settings - Fork 905
usb-linux: Support larger usbmon ring buffer limit #1555
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
This enables full utilization of the increased 64 MiB usbmon BUFF_MAX limit on kernels that have it.
Thank you for preparing this change. It would be very helpful to say in the comments which Linux kernel version is supposed to implement the increased buffer limit. |
It was submitted today (2025-09-15), so it's more like "which Linux kernel version you expect to implement the increased buffer limit", without any guarantee that it'll be in that version (or if it'll be in any kernel version; nobody's commented on it yet, much less approved it yet). The code forces the size to be >= the minimum size, so a |
This makes the maximum snapshot length for USB captures much larger. Libpcap currently imposes the limit on the snapshot length it considers valid in capture file, in an attempt to avoid overly-large snapshot length causing code to eat a huge chunk of address space for the packet buffer; this might not be too much of an issue on 64-bit systems, other than maybe eating large chunks of swap space, but on a 32-bit system it could eat a large chunk of the address space, such that memory allocations after allocating the buffer fail. Wireshark has a similar scheme for that. My Brilliant Idea(TM) of making the maximum snapshot length be link-layer-type-dependent may have been well-intentioned, but I'm no longer sure it made any sense to just give a few link-layer types special dispensation, given that you're not guaranteed not to be reading a capture with one of those link-layer types. The current maximum for D-Bus captures is the largest maximum we have now, and it's 128 MB, which is twice |
Yeah, I submitted this MR before getting anything merged in mainline for two reasons. First reason is, I had an earlier conversation with the USB subsystem maintainer where I proposed that this change be made and his response suggested he would likely accept a patch provided I can confirm that it actually works for my use case (which I did). The second is, once merged into mainline, it's going to take a very long time before most users see the change in their kernel, so I wanted to make sure my patching efforts throughout the dependency tree (kernel, libpcap, and Wireshark) were all happening in parallel. Incidentally, my patch was merged into the
Correct, and I'm not actually 100% certain if the approach I've taken here is the correct one, so if you have a better idea, please let me know! It certainly feels very weird to be copy-pasting and hard-coding kernel constants in userspace code. In theory, any future kernel could change those constants without warning (like the one that just got accepted into It also feels a bit weird to silently upgrade/downgrade the max buffer size depending on what libpcap detects the kernel supports, without telling the library user (since they now also have to infer both what the kernel supports and what libpcap supports). Granted, libpcap currently silently downgrades the buffer size anyways, so the way my patch works isn't a change from current behavior, but it's still a bit weird to me. But I don't know of any "smarter" way to do this without also changing the libpcap API, which I understand is supposed to be stable. Also, to be perfectly honest, I have no idea what the point of forcing a maximum snaplen is, other than to preemptively enforce constraints of the kernel. And because I didn't understand the purpose of this limit, I retained this functionality in my patch. But I felt like it might be better to remove that extra limit it since the code that does the kernel buffer size detection already enforces a maximum snaplen, and adding another limitation in the code feels arbitrary (and tripped me up when I was trying to figure out why my changes to Finally, for context on my perspectives, I never use libpcap directly--I exclusively perform USB captures using Wireshark, primarily on 64-bit systems, but probably might also do so on some 32-bit ARM systems (mini PCs and Chromebooks, each with 1-4 GB of memory). And if memory usage is a concern, why not let the API user decide? Since I assume they're going to have a better idea of how much memory usage their system has available for capture buffers than libpcap can infer. Please note though that I have no idea how the API works since I've never used it, so maybe this is already the case? Anyways, I'm mostly just thinking out loud here. And once again, if you have any suggestions on better ways to do what I'm trying to accomplish, I'll be happy to hear them! |
I don't have any better idea, "This makes the maximum snapshot length for USB captures much larger." means "libpcap and Wireshark will need to be changed as a result", not "it's a problem".
Yes - you might want to send a patch that adds new ioctls to query the minimum and maximum buffer sizes. Oh, and another patch to move the ioctl codes and appropriate data structures from drivers/usb/mon/mon_bin.c and put them into a header file in include/uapi/linux/usb, so that all of this is exported to userland rather than having to be copied and pasted into code that uses the USB monitor stuff.
Libpcap does the same thing for the BPF capture mechanism on *BSD/macOS/AIX/Solaris 11, so it's not as if this is something unique to the USB monitor code.
Why does the library user have to infer anything? The whole point of libpcap is to hide details such as that from the library user; most programs shouldn't need to set the buffer size, they should just let the library pick the default (if the default isn't appropriate for most captures, it should be fixed to be appropriate), with an API to request a different size if the default isn't big enough.
As is the case when capturing on a *BSD/macOS/AIX/Solaris 11 network interface.
To avoid capture files with a snapshot length that causes code that reads capture files to allocate a Really Huge Buffer, which on a 32-bit machine could succeed but not leave enough address space for the next allocation, and even on a 64-bit machine might either require allocation of a large chunk of swap space as backing store or cause some sort of overcommit error at run time.
There are multiple parts of the code that handle kernel buffers - it's in most if no all files with pcap-XXX.c names. It's not unique to the USB monitor. Note also that it's directly limiting the snapshot length, i.e. the maximum size of a single packet. How that affects the buffer size is platform-dependent and capture-device-dependent.
That's the concern in programs reading the capture files; the kernel itself will presumably limit the kernel buffer size. |
I've just learned that my kernel changes have been merged into mainline, and we'll see the increased max buffer size in the 6.18 release! 🎉 @guyharris Thank you for answering my questions. Is there anything I need to change to get this PR merged? If big changes need to be made I'll be happy to make them, I just need to know the "what" and "where". |
I've recently submitted a patch to the Linux kernel to increase the maximum usbmon ring buffer size from 1200 KiB to 64 MiB. libpcap needs to be made aware of this larger limit in order for applications that use it to be able to capture larger, multi-MB URBs.
This PR adds that support.