Build failure with Kernel >= 6.0: error: flexible array member ‘usbdevfs_urb::iso_frame_desc’ not at end of ‘struct usb_handle’

[mdartmann]

On Kernel 6.0-rc6 with gcc 12.1, building android-tools 33.0.3 fails with the following error:

/usr/lib/ccache/bin/g++ -DADB_HOST=1 -D_FILE_OFFSET_BITS=64 -D_GNU_SOURCE -I/home/mae/.cache/kiss/proc/121205/build/android-tools/build/vendor -I/home/mae/.cache/kiss/proc/121205/build/android-tools/vendor/base/libs/androidfw/include -I/home/mae/.cache/kiss/proc/121205/build/android-tools/vendor/boringssl/include -I/home/mae/.cache/kiss/proc/121205/build/android-tools/vendor/adb -I/home/mae/.cache/kiss/proc/121205/build/android-tools/vendor/adb/crypto/include -I/home/mae/.cache/kiss/proc/121205/build/android-tools/vendor/adb/pairing_auth/include -I/home/mae/.cache/kiss/proc/121205/build/android-tools/vendor/adb/pairing_connection/include -I/home/mae/.cache/kiss/proc/121205/build/android-tools/vendor/adb/tls/include -I/home/mae/.cache/kiss/proc/121205/build/android-tools/vendor/core/include -I/home/mae/.cache/kiss/proc/121205/build/android-tools/vendor/core/libcrypto_utils/include -I/home/mae/.cache/kiss/proc/121205/build/android-tools/vendor/core/libcutils/include -I/home/mae/.cache/kiss/proc/121205/build/android-tools/vendor/libbase/include -I/home/mae/.cache/kiss/proc/121205/build/android-tools/vendor/libziparchive/include -I/home/mae/.cache/kiss/proc/121205/build/android-tools/vendor/native/include -march=x86-64-v3 -mtune=generic -O3 -pipe -fno-plt -fexceptions -Wp,-D_FORTIFY_SOURCE=2 -Wformat -Wno-error=format-security -fstack-clash-protection -fcf-protection -Wp,-D_GLIBCXX_ASSERTIONS -O3 -DNDEBUG -Wno-attributes -std=gnu++20 -MD -MT vendor/CMakeFiles/libadb.dir/adb/client/usb_linux.cpp.o -MF vendor/CMakeFiles/libadb.dir/adb/client/usb_linux.cpp.o.d -o vendor/CMakeFiles/libadb.dir/adb/client/usb_linux.cpp.o -c /home/mae/.cache/kiss/proc/121205/build/android-tools/vendor/adb/client/usb_linux.cpp
In file included from /home/mae/.cache/kiss/proc/121205/build/android-tools/vendor/adb/client/usb_linux.cpp:28:
/usr/include/linux/usbdevice_fs.h:134:41: error: flexible array member ‘usbdevfs_urb::iso_frame_desc’ not at end of ‘struct usb_handle’
  134 |         struct usbdevfs_iso_packet_desc iso_frame_desc[];
      |                                         ^~~~~~~~~~~~~~
/home/mae/.cache/kiss/proc/121205/build/android-tools/vendor/adb/client/usb_linux.cpp:76:18: note: next member ‘usbdevfs_urb usb_handle::urb_out’ declared here
   76 |     usbdevfs_urb urb_out;
      |                  ^~~~~~~
/home/mae/.cache/kiss/proc/121205/build/android-tools/vendor/adb/client/usb_linux.cpp:61:8: note: in the definition of ‘struct usb_handle’
   61 | struct usb_handle {
      |        ^~~~~~~~~~

Build script:

cmake -B build -G Ninja \
    -D CMAKE_INSTALL_PREFIX=/usr \
    -D CMAKE_BUILD_TYPE=Release \
    -D CMAKE_C_FLAGS="${CFLAGS}" \
    -D CMAKE_CXX_FLAGS="${CXXFLAGS}"

sed -i "s|-Werror|-Wno-error|g" build/build.ninja

ninja -C build
ninja -C build install

Flags:

export CFLAGS='-march=x86-64-v3 -mtune=generic -O3 -pipe -fno-plt -fexceptions -Wp,-D_FORTIFY_SOURCE=2 -Wformat -Werror=format-security -fstack-clash-protection -fcf-protection'
export CXXFLAGS="$CFLAGS -Wp,-D_GLIBCXX_ASSERTIONS"
export LDFLAGS='-Wl,-O1,--sort-common,--as-needed,-z,relro,-z,now'

[pva]

The following commit in kernel causes build problem: https://github.com/torvalds/linux/commit/94dfc73e7cf4a31da66b8843f0b9283ddd6b8381

[anatol]

cc @GustavoARSilva

[GustavoARSilva]

Hi! I wonder if you ever need to make use of flex-array iso_frame_desc[] in your code...?

[GustavoARSilva]

This should fix the issue (applies to Linux mainline): https://git.kernel.org/pub/scm/linux/kernel/git/gustavoars/linux.git/commit/?h=testing/fam0-uapi-warnings https://godbolt.org/z/sWWKqvhqK

I'll wait for it to be build-tested by 0-day before sending it out.

[anatol]

I wonder if there is a way to patch struct usb_handle {} instead and make it compatible with both the old and the new struct definition?

[GustavoARSilva]

I wonder if there is a way to patch struct usb_handle {} instead and make it compatible with both the old and the new struct definition?

One way to patch that user-space code is to turn these two interior members

usbdevfs_urb urb_in;
usbdevfs_urb urb_out;

into pointers:

usbdevfs_urb *urb_in;
usbdevfs_urb *urb_out;

and of course change the related code, accordingly.

[GustavoARSilva]

I wonder if there is a way to patch struct usb_handle {} instead and make it compatible with both the old and the new struct definition?

One way to patch that user-space code is to turn these two interior members

usbdevfs_urb urb_in;
usbdevfs_urb urb_out;

into pointers:

usbdevfs_urb *urb_in;
usbdevfs_urb *urb_out;

and of course change the related code, accordingly.

See: https://godbolt.org/z/d679cb1xn

[anatol]

So I patched adb sources with the following changes, and it compiles fine with 5.19 kernel. The compiled adb allows to use an Android device via a USB cable; everything looks fine so far.

I am not an AOSP developer, so my testing coverage is probably shallow. Feedback/testing is very welcome.

diff --git a/client/usb_linux.cpp b/client/usb_linux.cpp
index 25a50bd1..fa0bbf36 100644
--- a/client/usb_linux.cpp
+++ b/client/usb_linux.cpp
@@ -72,8 +72,8 @@ struct usb_handle {
     unsigned zero_mask;
     unsigned writeable = 1;

-    usbdevfs_urb urb_in;
-    usbdevfs_urb urb_out;
+    usbdevfs_urb *urb_in;
+    usbdevfs_urb *urb_out;

     bool urb_in_busy = false;
     bool urb_out_busy = false;
@@ -304,7 +304,7 @@ static int usb_bulk_write(usb_handle* h, const void* data, int len) {
     std::unique_lock<std::mutex> lock(h->mutex);
     D("++ usb_bulk_write ++");

-    usbdevfs_urb* urb = &h->urb_out;
+    usbdevfs_urb* urb = h->urb_out;
     memset(urb, 0, sizeof(*urb));
     urb->type = USBDEVFS_URB_TYPE_BULK;
     urb->endpoint = h->ep_out;
@@ -343,7 +343,7 @@ static int usb_bulk_read(usb_handle* h, void* data, int len) {
     std::unique_lock<std::mutex> lock(h->mutex);
     D("++ usb_bulk_read ++");

-    usbdevfs_urb* urb = &h->urb_in;
+    usbdevfs_urb* urb = h->urb_in;
     memset(urb, 0, sizeof(*urb));
     urb->type = USBDEVFS_URB_TYPE_BULK;
     urb->endpoint = h->ep_in;
@@ -388,7 +388,7 @@ static int usb_bulk_read(usb_handle* h, void* data, int len) {
         }
         D("[ urb @%p status = %d, actual = %d ]", out, out->status, out->actual_length);

-        if (out == &h->urb_in) {
+        if (out == h->urb_in) {
             D("[ reap urb - IN complete ]");
             h->urb_in_busy = false;
             if (urb->status != 0) {
@@ -397,7 +397,7 @@ static int usb_bulk_read(usb_handle* h, void* data, int len) {
             }
             return urb->actual_length;
         }
-        if (out == &h->urb_out) {
+        if (out == h->urb_out) {
             D("[ reap urb - OUT compelete ]");
             h->urb_out_busy = false;
             h->cv.notify_all();
@@ -501,10 +501,10 @@ void usb_kick(usb_handle* h) {
             ** but this ensures that a reader blocked on REAPURB
             ** will get unblocked
             */
-            ioctl(h->fd, USBDEVFS_DISCARDURB, &h->urb_in);
-            ioctl(h->fd, USBDEVFS_DISCARDURB, &h->urb_out);
-            h->urb_in.status = -ENODEV;
-            h->urb_out.status = -ENODEV;
+            ioctl(h->fd, USBDEVFS_DISCARDURB, h->urb_in);
+            ioctl(h->fd, USBDEVFS_DISCARDURB, h->urb_out);
+            h->urb_in->status = -ENODEV;
+            h->urb_out->status = -ENODEV;
             h->urb_in_busy = false;
             h->urb_out_busy = false;
             h->cv.notify_all();
@@ -520,6 +520,8 @@ int usb_close(usb_handle* h) {

     D("-- usb close %p (fd = %d) --", h, h->fd);

+    delete h->urb_in;
+    delete h->urb_out;
     delete h;

     return 0;
@@ -573,6 +575,8 @@ static void register_device(const char* dev_name, const char* dev_path, unsigned
     usb->ep_out = ep_out;
     usb->zero_mask = zero_mask;
     usb->max_packet_size = max_packet_size;
+    usb->urb_in = new usbdevfs_urb;
+    usb->urb_out = new usbdevfs_urb;

     // Initialize mark so we don't get garbage collected after the device scan.
     usb->mark = true;

[GustavoARSilva]

So I patched adb sources with the following changes, and it compiles fine with 5.19 kernel. The compiled adb allows to use an Android device via a USB cable; everything looks fine so far.

Just to point out that the change in UAPI landed in kernel 6.0, not 5.19.

I am not an AOSP developer, so my testing coverage is probably shallow. Feedback/testing is very welcome.

Acked-by: Gustavo A. R. Silva gustavoars@kernel.org

Thanks!

diff --git a/client/usb_linux.cpp b/client/usb_linux.cpp
index 25a50bd1..fa0bbf36 100644
--- a/client/usb_linux.cpp
+++ b/client/usb_linux.cpp
@@ -72,8 +72,8 @@ struct usb_handle {
     unsigned zero_mask;
     unsigned writeable = 1;

-    usbdevfs_urb urb_in;
-    usbdevfs_urb urb_out;
+    usbdevfs_urb *urb_in;
+    usbdevfs_urb *urb_out;

     bool urb_in_busy = false;
     bool urb_out_busy = false;
@@ -304,7 +304,7 @@ static int usb_bulk_write(usb_handle* h, const void* data, int len) {
     std::unique_lock<std::mutex> lock(h->mutex);
     D("++ usb_bulk_write ++");

-    usbdevfs_urb* urb = &h->urb_out;
+    usbdevfs_urb* urb = h->urb_out;
     memset(urb, 0, sizeof(*urb));
     urb->type = USBDEVFS_URB_TYPE_BULK;
     urb->endpoint = h->ep_out;
@@ -343,7 +343,7 @@ static int usb_bulk_read(usb_handle* h, void* data, int len) {
     std::unique_lock<std::mutex> lock(h->mutex);
     D("++ usb_bulk_read ++");

-    usbdevfs_urb* urb = &h->urb_in;
+    usbdevfs_urb* urb = h->urb_in;
     memset(urb, 0, sizeof(*urb));
     urb->type = USBDEVFS_URB_TYPE_BULK;
     urb->endpoint = h->ep_in;
@@ -388,7 +388,7 @@ static int usb_bulk_read(usb_handle* h, void* data, int len) {
         }
         D("[ urb @%p status = %d, actual = %d ]", out, out->status, out->actual_length);

-        if (out == &h->urb_in) {
+        if (out == h->urb_in) {
             D("[ reap urb - IN complete ]");
             h->urb_in_busy = false;
             if (urb->status != 0) {
@@ -397,7 +397,7 @@ static int usb_bulk_read(usb_handle* h, void* data, int len) {
             }
             return urb->actual_length;
         }
-        if (out == &h->urb_out) {
+        if (out == h->urb_out) {
             D("[ reap urb - OUT compelete ]");
             h->urb_out_busy = false;
             h->cv.notify_all();
@@ -501,10 +501,10 @@ void usb_kick(usb_handle* h) {
             ** but this ensures that a reader blocked on REAPURB
             ** will get unblocked
             */
-            ioctl(h->fd, USBDEVFS_DISCARDURB, &h->urb_in);
-            ioctl(h->fd, USBDEVFS_DISCARDURB, &h->urb_out);
-            h->urb_in.status = -ENODEV;
-            h->urb_out.status = -ENODEV;
+            ioctl(h->fd, USBDEVFS_DISCARDURB, h->urb_in);
+            ioctl(h->fd, USBDEVFS_DISCARDURB, h->urb_out);
+            h->urb_in->status = -ENODEV;
+            h->urb_out->status = -ENODEV;
             h->urb_in_busy = false;
             h->urb_out_busy = false;
             h->cv.notify_all();
@@ -520,6 +520,8 @@ int usb_close(usb_handle* h) {

     D("-- usb close %p (fd = %d) --", h, h->fd);

+    delete h->urb_in;
+    delete h->urb_out;
     delete h;

     return 0;
@@ -573,6 +575,8 @@ static void register_device(const char* dev_name, const char* dev_path, unsigned
     usb->ep_out = ep_out;
     usb->zero_mask = zero_mask;
     usb->max_packet_size = max_packet_size;
+    usb->urb_in = new usbdevfs_urb;
+    usb->urb_out = new usbdevfs_urb;

     // Initialize mark so we don't get garbage collected after the device scan.
     usb->mark = true;

[anatol]

I sent the change upstream https://android-review.googlesource.com/c/platform/packages/modules/adb/+/2247053

[GustavoARSilva]

I sent the change upstream https://android-review.googlesource.com/c/platform/packages/modules/adb/+/2247053

Great. :)

Thanks!

[anatol]

@GustavoARSilva a small nitpick, it is a good idea to update the documentation here as well https://github.com/torvalds/linux/blame/master/Documentation/driver-api/usb/URB.rst#L85