[PATCH] USB: Fix configuration selection issues introduced in v4.20.0

[PATCH] USB: Fix configuration selection issues introduced in v4.20.0

[Nikolay Yakimov]

Commit f13912d3f014a introduced changes to the usb_choose_configuration function
to better support USB Audio UAC3-compatible devices. However, there are a few
problems with this patch. First of all, it adds new "if" clauses in the middle
of an existing "if"/"else if" tree, which obviously breaks pre-existing logic.
Secondly, since it continues iterating over configurations in one of the branches,
other code in the loop can choose an unintended configuration. Finally,
if an audio device's first configuration is UAC3-compatible, and there
are multiple UAC3 configurations, the second one would be chosen, due to
the first configuration never being checked for UAC3-compatibility.

Commit ff2a8c532c14 tries to fix the second issue, but it goes about it in a
somewhat unnecessarily convoluted way, in my opinion, and does nothing
to fix the first or the last one.

This patch tries to rectify problems described by essentially rewriting
code introduced in f13912d3f014a. Notice the code was moved to *before*
the "if"/"else if" tree.

Signed-off-by: Nikolay Yakimov <root@livid.pp.ru>
---
 drivers/usb/core/generic.c | 44 ++++++++++++++++++++++----------------
 1 file changed, 25 insertions(+), 19 deletions(-)

diff --git a/drivers/usb/core/generic.c b/drivers/usb/core/generic.c
index f713cecc1f41..1ac9c1e5f773 100644
--- a/drivers/usb/core/generic.c
+++ b/drivers/usb/core/generic.c
@@ -118,6 +118,31 @@ int usb_choose_configuration(struct usb_device *udev)
 			continue;
 		}
 
+		/*
+		 * Select first configuration as default for audio so that
+		 * devices that don't comply with UAC3 protocol are supported.
+		 * But, still iterate through other configurations and
+		 * select UAC3 compliant config if present.
+		 */
+		if (desc && is_audio(desc)) {
+			/* Always prefer the first found UAC3 config */
+			if (is_uac3_config(desc)) {
+				best = c;
+				break;
+			}
+
+			/* If there is no UAC3 config, prefer the first config */
+			else if (i == 0)
+				best = c;
+
+			/* Unconditional continue, because the rest of the code
+			 * in the loop is irrelevant for audio devices, and
+			 * because it can reassign best, which for audio devices
+			 * we don't want.
+			 */
+			continue;
+		}
+
 		/* When the first config's first interface is one of Microsoft's
 		 * pet nonstandard Ethernet-over-USB protocols, ignore it unless
 		 * this kernel has enabled the necessary host side driver.
@@ -132,25 +157,6 @@ int usb_choose_configuration(struct usb_device *udev)
 #endif
 		}
 
-		/*
-		 * Select first configuration as default for audio so that
-		 * devices that don't comply with UAC3 protocol are supported.
-		 * But, still iterate through other configurations and
-		 * select UAC3 compliant config if present.
-		 */
-		if (i == 0 && num_configs > 1 && desc && is_audio(desc)) {
-			best = c;
-			continue;
-		}
-
-		if (i > 0 && desc && is_audio(desc)) {
-			if (is_uac3_config(desc)) {
-				best = c;
-				break;
-			}
-			continue;
-		}
-
 		/* From the remaining configs, choose the first one whose
 		 * first interface is for a non-vendor-specific class.
 		 * Reason: Linux is more likely to have a class driver
-- 
2.20.1

Re: [PATCH] USB: Fix configuration selection issues introduced in v4.20.0

[Greg Kroah-Hartman]

On Tue, Jan 15, 2019 at 07:13:54PM +0300, Nikolay Yakimov wrote:
> Commit f13912d3f014a introduced changes to the usb_choose_configuration function
> to better support USB Audio UAC3-compatible devices. However, there are a few
> problems with this patch. First of all, it adds new "if" clauses in the middle
> of an existing "if"/"else if" tree, which obviously breaks pre-existing logic.
> Secondly, since it continues iterating over configurations in one of the branches,
> other code in the loop can choose an unintended configuration. Finally,
> if an audio device's first configuration is UAC3-compatible, and there
> are multiple UAC3 configurations, the second one would be chosen, due to
> the first configuration never being checked for UAC3-compatibility.
> 
> Commit ff2a8c532c14 tries to fix the second issue, but it goes about it in a
> somewhat unnecessarily convoluted way, in my opinion, and does nothing
> to fix the first or the last one.
> 
> This patch tries to rectify problems described by essentially rewriting
> code introduced in f13912d3f014a. Notice the code was moved to *before*
> the "if"/"else if" tree.
> 
> Signed-off-by: Nikolay Yakimov <root@livid.pp.ru>
> ---
>  drivers/usb/core/generic.c | 44 ++++++++++++++++++++++----------------
>  1 file changed, 25 insertions(+), 19 deletions(-)

Were you able to test this on one of the devices that ff2a8c532c14
("usbcore: Select only first configuration for non-UAC3 compliant
devices") was created to fix?

thanks,

greg k-h

Re: [PATCH] USB: Fix configuration selection issues introduced in v4.20.0

[Nikolay Yakimov]

tue, 15 jan. 2019 at 19:53, Greg Kroah-Hartman <gregkh@linuxfoundation.org>:
> Were you able to test this on one of the devices that ff2a8c532c14
> ("usbcore: Select only first configuration for non-UAC3 compliant
> devices") was created to fix?

Yes, owning such a device (non-UAC3 with multiple configurations) was
the motivation for investigating in the first place (since it was
broken on v4.20.0 and v4.20.1). So the proposed patch is tested
against it.

In fact, I only discovered ff2a8c532c14 ("usbcore: Select only first
configuration for non-UAC3 compliant devices") when preparing to send
in the patch (it's not in any released kernel yet)

>
> thanks,
>
> greg k-h

RE: [PATCH] USB: Fix configuration selection issues introduced in v4.20.0

[Gopal, Saranya]

Hi Yakimov,

As per UAC3 configuration, the first configuration will always be backward-compatible.
So, there cannot be any UAC3-compatible device which has first configuration as UAC3.
And secondly, the commit ff2a8c532c14 does not break the pre-existing logic.
I also thought so much about moving the code above Ethernet check while preparing ff2a8c532c14.
But, then, it doesn't break the existing logic and so decided against moving it.

Thanks,
Saranya
> -----Original Message-----
> From: Nikolay Yakimov [mailto:root@livid.pp.ru]
> Sent: Tuesday, January 15, 2019 9:44 PM
> To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Nikolay Yakimov <root@livid.pp.ru>; Gopal, Saranya
> <saranya.gopal@intel.com>; linux-usb@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: [PATCH] USB: Fix configuration selection issues introduced in v4.20.0
> 
> Commit f13912d3f014a introduced changes to the usb_choose_configuration
> function
> to better support USB Audio UAC3-compatible devices. However, there are a
> few
> problems with this patch. First of all, it adds new "if" clauses in the middle
> of an existing "if"/"else if" tree, which obviously breaks pre-existing logic.
> Secondly, since it continues iterating over configurations in one of the branches,
> other code in the loop can choose an unintended configuration. Finally,
> if an audio device's first configuration is UAC3-compatible, and there
> are multiple UAC3 configurations, the second one would be chosen, due to
> the first configuration never being checked for UAC3-compatibility.
> 
> Commit ff2a8c532c14 tries to fix the second issue, but it goes about it in a
> somewhat unnecessarily convoluted way, in my opinion, and does nothing
> to fix the first or the last one.
> 
> This patch tries to rectify problems described by essentially rewriting
> code introduced in f13912d3f014a. Notice the code was moved to *before*
> the "if"/"else if" tree.
> 
> Signed-off-by: Nikolay Yakimov <root@livid.pp.ru>
> ---
>  drivers/usb/core/generic.c | 44 ++++++++++++++++++++++----------------
>  1 file changed, 25 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/usb/core/generic.c b/drivers/usb/core/generic.c
> index f713cecc1f41..1ac9c1e5f773 100644
> --- a/drivers/usb/core/generic.c
> +++ b/drivers/usb/core/generic.c
> @@ -118,6 +118,31 @@ int usb_choose_configuration(struct usb_device
> *udev)
>  			continue;
>  		}
> 
> +		/*
> +		 * Select first configuration as default for audio so that
> +		 * devices that don't comply with UAC3 protocol are supported.
> +		 * But, still iterate through other configurations and
> +		 * select UAC3 compliant config if present.
> +		 */
> +		if (desc && is_audio(desc)) {
> +			/* Always prefer the first found UAC3 config */
> +			if (is_uac3_config(desc)) {
> +				best = c;
> +				break;
> +			}
> +
> +			/* If there is no UAC3 config, prefer the first config */
> +			else if (i == 0)
> +				best = c;
> +
> +			/* Unconditional continue, because the rest of the code
> +			 * in the loop is irrelevant for audio devices, and
> +			 * because it can reassign best, which for audio devices
> +			 * we don't want.
> +			 */
> +			continue;
> +		}
> +
>  		/* When the first config's first interface is one of Microsoft's
>  		 * pet nonstandard Ethernet-over-USB protocols, ignore it
> unless
>  		 * this kernel has enabled the necessary host side driver.
> @@ -132,25 +157,6 @@ int usb_choose_configuration(struct usb_device
> *udev)
>  #endif
>  		}
> 
> -		/*
> -		 * Select first configuration as default for audio so that
> -		 * devices that don't comply with UAC3 protocol are supported.
> -		 * But, still iterate through other configurations and
> -		 * select UAC3 compliant config if present.
> -		 */
> -		if (i == 0 && num_configs > 1 && desc && is_audio(desc)) {
> -			best = c;
> -			continue;
> -		}
> -
> -		if (i > 0 && desc && is_audio(desc)) {
> -			if (is_uac3_config(desc)) {
> -				best = c;
> -				break;
> -			}
> -			continue;
> -		}
> -
>  		/* From the remaining configs, choose the first one whose
>  		 * first interface is for a non-vendor-specific class.
>  		 * Reason: Linux is more likely to have a class driver
> --
> 2.20.1

Re: [PATCH] USB: Fix configuration selection issues introduced in v4.20.0

[Nikolay Yakimov]

On Wed, 16 Jan 2019 at 07:24, Gopal, Saranya <saranya.gopal@intel.com> wrote:
>
> Hi Yakimov,
>
> As per UAC3 configuration, the first configuration will always be backward-compatible.
> So, there cannot be any UAC3-compatible device which has first configuration as UAC3.

Thanks for the clarification. I would argue though that not all
devices might adhere to the specification, so it's still probably a
good idea to check all configurations "just in case". I will not press
this point though.

> And secondly, the commit ff2a8c532c14 does not break the pre-existing logic.
> I also thought so much about moving the code above Ethernet check while preparing ff2a8c532c14.
> But, then, it doesn't break the existing logic and so decided against moving it.

It does seem to change the logic for RNDIS devices with multiple
configurations when RNDIS kernel module is enabled.

Consider an RNDIS device with multiple configurations (num_configs > 1)

Before f13912d3f014a, the following sequence would be executed:

1. i == 0.

if (i == 0 && num_configs > 1 && desc &&
        (is_rndis(desc) || is_activesync(desc))) {
    best = c;
}
else ...

The condition is true, so this branch is executed. The first
configuration is selected. Since it's the last condition (all the
remaining code in the loop body is in the else branch), the loop will
continue onto the next iteration.

2. i == 1.

if (i == 0 && num_configs > 1 && desc &&
        (is_rndis(desc) || is_activesync(desc))) {
    ...
}
else if (udev->descriptor.bDeviceClass !=
                    USB_CLASS_VENDOR_SPEC &&
            (desc && desc->bInterfaceClass !=
                    USB_CLASS_VENDOR_SPEC)) {
    best = c;
    break;
} else ...

If the second configuration is not vendor-specific, the first "else
if" branch would be executed, and the second configuration is
selected. The loop is exited on break.

After ff2a8c532c14, the following sequence would be executed:

1. i == 0.

if (i == 0 && num_configs > 1 && desc &&
        (is_rndis(desc) || is_activesync(desc))) {
    best = c;
}

The condition is true, so this branch is executed. The first
configuration is selected, but the loop body continues execution.

2. Still i == 0.

if (i == 0 && num_configs > 1 && desc && is_audio(desc)) {
    ...
}

A redundant check is preformed to check if the device is an audio
device. We already know it isn't.

3. Still i == 0.

if (i > 0 && desc && is_audio(desc)) {
    ...
}
else ...

A redundant check is preformed to check if (i > 0). We already know it isn't.

4. Still i == 0.

else if (udev->descriptor.bDeviceClass !=
                    USB_CLASS_VENDOR_SPEC &&
            (desc && desc->bInterfaceClass !=
                    USB_CLASS_VENDOR_SPEC)) {
    best = c;
    break;
}
else ...

The first "else if" condition is checked. Unless bDeviceClass is
USB_CLASS_VENDOR_SPEC (correct me if I'm wrong, but in most cases I
think it wouldn't be), that condition evaluates as true. The
configuration is selected (redundantly), and the loop is exited on
break.

The result is this:
Before f13912d3f014a, if an RNDIS device has non-vendor-specific
configurations after the first one, that one would be selected.
After ff2a8c532c14, the first configuration would always be selected
for RNDIS devices. Besides, there are several redundant checks in this
case, which is, if nothing else, confusing.

Hopefully I've explained my point clearly enough.

>
> Thanks,
> Saranya

RE: [PATCH] USB: Fix configuration selection issues introduced in v4.20.0

[Gopal, Saranya]

> The result is this:
> Before f13912d3f014a, if an RNDIS device has non-vendor-specific
> configurations after the first one, that one would be selected.
> After ff2a8c532c14, the first configuration would always be selected
> for RNDIS devices. Besides, there are several redundant checks in this
> case, which is, if nothing else, confusing.
> 
> Hopefully I've explained my point clearly enough.

Agreed. Thanks for pointing it out. I somehow missed this :(

Thanks,
Saranya

Re: [PATCH] USB: Fix configuration selection issues introduced in v4.20.0

[Nikolay Yakimov]

Hey there.

Sorry to nag, but it seems we came to the conclusion that there's
indeed a problem with the current code, and then collectively decided
that we're done. Can I do something to expedite this? Are there any
issues with my proposed patch? If there are, what can I do to fix
those? Alternatively, perhaps it would be easier/faster if a bona fide
kernel developer could fix problems described in this thread (well,
actually, I believe we're down to one somewhat pressing problem, which
could be fixed just by moving some code around)


On Thu, 17 Jan 2019 at 06:53, Gopal, Saranya <saranya.gopal@intel.com> wrote:
>
> > The result is this:
> > Before f13912d3f014a, if an RNDIS device has non-vendor-specific
> > configurations after the first one, that one would be selected.
> > After ff2a8c532c14, the first configuration would always be selected
> > for RNDIS devices. Besides, there are several redundant checks in this
> > case, which is, if nothing else, confusing.
> >
> > Hopefully I've explained my point clearly enough.
>
> Agreed. Thanks for pointing it out. I somehow missed this :(
>
> Thanks,
> Saranya