[PATCH net 0/2] r8152: Fix a regression with usbguard

[PATCH net 0/2] r8152: Fix a regression with usbguard

[Maxim Mikityanskiy]

Introduction of r8152-cfgselector broke hotplug of Realtek USB NICs on
machines that use usbguard. These patches are supposed to fix it.

Tested on RTL8153 (0bda:8153) that has two configuration descriptors:
vendor and CDC.

P.S. I'm not sure whether it's supposed to go through the USB or netdev
tree, therefore submitting to both mailing lists and marking for "net",
but please advise.

Maxim Mikityanskiy (2):
  USB: Allow usb_device_driver to override usb_choose_configuration
  r8152: Switch to using choose_configuration

 drivers/net/usb/r8152.c    | 18 +++++++++---------
 drivers/usb/core/generic.c | 10 ++++++++++
 include/linux/usb.h        |  3 +++
 3 files changed, 22 insertions(+), 9 deletions(-)

-- 
2.43.0

[PATCH net 1/2] USB: Allow usb_device_driver to override usb_choose_configuration

[Maxim Mikityanskiy]

usb_choose_configuration is called in two cases: on probe and on
authorization. If a usb_device_driver wants to override the default
configuration (like r8152 does to choose the vendor config), it can do
that on probe, but it has no control over what happens on authorization.
This breaks the intention on machines that use usbguard (all devices are
not authorized by default, and the permitted ones get the authorization
a moment later), because a wrong configuration ends up being selected.

Allow usb_device_driver to override usb_choose_configuration
specifically.

Signed-off-by: Maxim Mikityanskiy <maxtram95@gmail.com>
---
 drivers/usb/core/generic.c | 10 ++++++++++
 include/linux/usb.h        |  3 +++
 2 files changed, 13 insertions(+)

diff --git a/drivers/usb/core/generic.c b/drivers/usb/core/generic.c
index 740342a2812a..a1bc4f875d37 100644
--- a/drivers/usb/core/generic.c
+++ b/drivers/usb/core/generic.c
@@ -59,10 +59,20 @@ int usb_choose_configuration(struct usb_device *udev)
 	int num_configs;
 	int insufficient_power = 0;
 	struct usb_host_config *c, *best;
+	struct usb_device_driver *udriver = NULL;
 
 	if (usb_device_is_owned(udev))
 		return 0;
 
+	if (udev->dev.driver) {
+		udriver = to_usb_device_driver(udev->dev.driver);
+		if (udriver->choose_configuration) {
+			i = udriver->choose_configuration(udev);
+			if (i != -EOPNOTSUPP)
+				return max(i, -1);
+		}
+	}
+
 	best = NULL;
 	c = udev->config;
 	num_configs = udev->descriptor.bNumConfigurations;
diff --git a/include/linux/usb.h b/include/linux/usb.h
index 8c61643acd49..4f59b10f2fdd 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -1259,6 +1259,8 @@ struct usb_driver {
  *	device.  If it is, probe returns zero and uses dev_set_drvdata()
  *	to associate driver-specific data with the device.  If unwilling
  *	to manage the device, return a negative errno value.
+ * @choose_configuration: Called from usb_choose_configuration, allows the
+ *	driver to override the default configuration.
  * @disconnect: Called when the device is no longer accessible, usually
  *	because it has been (or is being) disconnected or the driver's
  *	module is being unloaded.
@@ -1283,6 +1285,7 @@ struct usb_device_driver {
 
 	bool (*match) (struct usb_device *udev);
 	int (*probe) (struct usb_device *udev);
+	int (*choose_configuration) (struct usb_device *udev);
 	void (*disconnect) (struct usb_device *udev);
 
 	int (*suspend) (struct usb_device *udev, pm_message_t message);
-- 
2.43.0

[PATCH net 2/2] r8152: Switch to using choose_configuration

[Maxim Mikityanskiy]

With the introduction of r8152-cfgselector, the following regression
appeared on machines that use usbguard: the netdev appears only when the
USB device is inserted the first time (before the module is loaded), but
on the second and next insertions no netdev is registered.

It happens because the device is probed as unauthorized, and usbguard
gives it an authorization a moment later. If the module is not loaded,
it's normally loaded slower than the authorization is given, and
everything works. If the module is already loaded, the cfgselector's
probe function runs first, but then usb_authorize_device kicks in and
changes the configuration to something chosen by the standard
usb_choose_configuration. rtl8152_probe refuses to probe non-vendor
configurations, and the user ends up without a netdev.

The previous commit added possibility to override
usb_choose_configuration. Use it to fix the bug and pick the right
configuration on both probe and authorization.

Fixes: ec51fbd1b8a2 ("r8152: add USB device driver for config selection")
Signed-off-by: Maxim Mikityanskiy <maxtram95@gmail.com>
---
 drivers/net/usb/r8152.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 9bf2140fd0a1..f0ac31a94f3c 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -10070,6 +10070,11 @@ static struct usb_driver rtl8152_driver = {
 };
 
 static int rtl8152_cfgselector_probe(struct usb_device *udev)
+{
+	return 0;
+}
+
+static int rtl8152_cfgselector_choose_configuration(struct usb_device *udev)
 {
 	struct usb_host_config *c;
 	int i, num_configs;
@@ -10078,7 +10083,7 @@ static int rtl8152_cfgselector_probe(struct usb_device *udev)
 	 * driver supports it.
 	 */
 	if (__rtl_get_hw_ver(udev) == RTL_VER_UNKNOWN)
-		return 0;
+		return -EOPNOTSUPP;
 
 	/* The vendor mode is not always config #1, so to find it out. */
 	c = udev->config;
@@ -10094,20 +10099,15 @@ static int rtl8152_cfgselector_probe(struct usb_device *udev)
 	}
 
 	if (i == num_configs)
-		return -ENODEV;
-
-	if (usb_set_configuration(udev, c->desc.bConfigurationValue)) {
-		dev_err(&udev->dev, "Failed to set configuration %d\n",
-			c->desc.bConfigurationValue);
-		return -ENODEV;
-	}
+		return -EOPNOTSUPP;
 
-	return 0;
+	return c->desc.bConfigurationValue;
 }
 
 static struct usb_device_driver rtl8152_cfgselector_driver = {
 	.name =		MODULENAME "-cfgselector",
 	.probe =	rtl8152_cfgselector_probe,
+	.choose_configuration = rtl8152_cfgselector_choose_configuration,
 	.id_table =	rtl8152_table,
 	.generic_subclass = 1,
 	.supports_autosuspend = 1,
-- 
2.43.0

Re: [PATCH net 2/2] r8152: Switch to using choose_configuration

[Grant Grundler]

On Sat, Dec 23, 2023 at 3:36 PM Maxim Mikityanskiy <maxtram95@gmail.com> wrote:
>
> With the introduction of r8152-cfgselector, the following regression
> appeared on machines that use usbguard: the netdev appears only when the
> USB device is inserted the first time (before the module is loaded), but
> on the second and next insertions no netdev is registered.
>
> It happens because the device is probed as unauthorized, and usbguard
> gives it an authorization a moment later. If the module is not loaded,
> it's normally loaded slower than the authorization is given, and
> everything works. If the module is already loaded, the cfgselector's
> probe function runs first, but then usb_authorize_device kicks in and
> changes the configuration to something chosen by the standard
> usb_choose_configuration. rtl8152_probe refuses to probe non-vendor
> configurations, and the user ends up without a netdev.
>
> The previous commit added possibility to override
> usb_choose_configuration. Use it to fix the bug and pick the right
> configuration on both probe and authorization.
>
> Fixes: ec51fbd1b8a2 ("r8152: add USB device driver for config selection")
> Signed-off-by: Maxim Mikityanskiy <maxtram95@gmail.com>

Maxim,
Does this solve the same problem that Doug Anderson posted patches for
a few weeks ago?

https://lore.kernel.org/all/20231201183113.343256-1-dianders@chromium.org/

Those went through the USB tree, so that probably explains why you
didn't see them in the net.git tree.

cheers,
grant

> ---
>  drivers/net/usb/r8152.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index 9bf2140fd0a1..f0ac31a94f3c 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c
> @@ -10070,6 +10070,11 @@ static struct usb_driver rtl8152_driver = {
>  };
>
>  static int rtl8152_cfgselector_probe(struct usb_device *udev)
> +{
> +       return 0;
> +}
> +
> +static int rtl8152_cfgselector_choose_configuration(struct usb_device *udev)
>  {
>         struct usb_host_config *c;
>         int i, num_configs;
> @@ -10078,7 +10083,7 @@ static int rtl8152_cfgselector_probe(struct usb_device *udev)
>          * driver supports it.
>          */
>         if (__rtl_get_hw_ver(udev) == RTL_VER_UNKNOWN)
> -               return 0;
> +               return -EOPNOTSUPP;
>
>         /* The vendor mode is not always config #1, so to find it out. */
>         c = udev->config;
> @@ -10094,20 +10099,15 @@ static int rtl8152_cfgselector_probe(struct usb_device *udev)
>         }
>
>         if (i == num_configs)
> -               return -ENODEV;
> -
> -       if (usb_set_configuration(udev, c->desc.bConfigurationValue)) {
> -               dev_err(&udev->dev, "Failed to set configuration %d\n",
> -                       c->desc.bConfigurationValue);
> -               return -ENODEV;
> -       }
> +               return -EOPNOTSUPP;
>
> -       return 0;
> +       return c->desc.bConfigurationValue;
>  }
>
>  static struct usb_device_driver rtl8152_cfgselector_driver = {
>         .name =         MODULENAME "-cfgselector",
>         .probe =        rtl8152_cfgselector_probe,
> +       .choose_configuration = rtl8152_cfgselector_choose_configuration,
>         .id_table =     rtl8152_table,
>         .generic_subclass = 1,
>         .supports_autosuspend = 1,
> --
> 2.43.0
>

Re: [PATCH net 2/2] r8152: Switch to using choose_configuration

[Grant Grundler]

On Mon, Dec 25, 2023 at 11:18 PM Grant Grundler <grundler@chromium.org> wrote:
>
> On Sat, Dec 23, 2023 at 3:36 PM Maxim Mikityanskiy <maxtram95@gmail.com> wrote:
> >
> > With the introduction of r8152-cfgselector, the following regression
> > appeared on machines that use usbguard: the netdev appears only when the
> > USB device is inserted the first time (before the module is loaded), but
> > on the second and next insertions no netdev is registered.
> >
> > It happens because the device is probed as unauthorized, and usbguard
> > gives it an authorization a moment later. If the module is not loaded,
> > it's normally loaded slower than the authorization is given, and
> > everything works. If the module is already loaded, the cfgselector's
> > probe function runs first, but then usb_authorize_device kicks in and
> > changes the configuration to something chosen by the standard
> > usb_choose_configuration. rtl8152_probe refuses to probe non-vendor
> > configurations, and the user ends up without a netdev.
> >
> > The previous commit added possibility to override
> > usb_choose_configuration. Use it to fix the bug and pick the right
> > configuration on both probe and authorization.
> >
> > Fixes: ec51fbd1b8a2 ("r8152: add USB device driver for config selection")
> > Signed-off-by: Maxim Mikityanskiy <maxtram95@gmail.com>
>
> Maxim,
> Does this solve the same problem that Doug Anderson posted patches for
> a few weeks ago?
>
> https://lore.kernel.org/all/20231201183113.343256-1-dianders@chromium.org/
>
> Those went through the USB tree, so that probably explains why you
> didn't see them in the net.git tree.

Specifically, usb.git usb-next branch:
"r8152: Choose our USB config with choose_configuration() rather than probe()"
https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git/commit/drivers/net/usb/r8152.c?h=usb-next&id=aa4f2b3e418e8673e55145de8b8016a7a9920306

Given this has a fixes tag, and two people have now tracked this down
and posted fixes for this problem, is there any chance of this landing
in 6.7?

And it looks like Maxim's change could be rebased on top of Doug's
patch and clean up a few more things. Do I see that correctly?

cheers,
grant

>
> cheers,
> grant
>
> > ---
> >  drivers/net/usb/r8152.c | 18 +++++++++---------
> >  1 file changed, 9 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> > index 9bf2140fd0a1..f0ac31a94f3c 100644
> > --- a/drivers/net/usb/r8152.c
> > +++ b/drivers/net/usb/r8152.c
> > @@ -10070,6 +10070,11 @@ static struct usb_driver rtl8152_driver = {
> >  };
> >
> >  static int rtl8152_cfgselector_probe(struct usb_device *udev)
> > +{
> > +       return 0;
> > +}
> > +
> > +static int rtl8152_cfgselector_choose_configuration(struct usb_device *udev)
> >  {
> >         struct usb_host_config *c;
> >         int i, num_configs;
> > @@ -10078,7 +10083,7 @@ static int rtl8152_cfgselector_probe(struct usb_device *udev)
> >          * driver supports it.
> >          */
> >         if (__rtl_get_hw_ver(udev) == RTL_VER_UNKNOWN)
> > -               return 0;
> > +               return -EOPNOTSUPP;
> >
> >         /* The vendor mode is not always config #1, so to find it out. */
> >         c = udev->config;
> > @@ -10094,20 +10099,15 @@ static int rtl8152_cfgselector_probe(struct usb_device *udev)
> >         }
> >
> >         if (i == num_configs)
> > -               return -ENODEV;
> > -
> > -       if (usb_set_configuration(udev, c->desc.bConfigurationValue)) {
> > -               dev_err(&udev->dev, "Failed to set configuration %d\n",
> > -                       c->desc.bConfigurationValue);
> > -               return -ENODEV;
> > -       }
> > +               return -EOPNOTSUPP;
> >
> > -       return 0;
> > +       return c->desc.bConfigurationValue;
> >  }
> >
> >  static struct usb_device_driver rtl8152_cfgselector_driver = {
> >         .name =         MODULENAME "-cfgselector",
> >         .probe =        rtl8152_cfgselector_probe,
> > +       .choose_configuration = rtl8152_cfgselector_choose_configuration,
> >         .id_table =     rtl8152_table,
> >         .generic_subclass = 1,
> >         .supports_autosuspend = 1,
> > --
> > 2.43.0
> >

Re: [PATCH net 2/2] r8152: Switch to using choose_configuration

[Maxim Mikityanskiy]

On Mon, 25 Dec 2023 at 23:42:08 -0800, Grant Grundler wrote:
> On Mon, Dec 25, 2023 at 11:18 PM Grant Grundler <grundler@chromium.org> wrote:
> >
> > On Sat, Dec 23, 2023 at 3:36 PM Maxim Mikityanskiy <maxtram95@gmail.com> wrote:
> > >
> > > With the introduction of r8152-cfgselector, the following regression
> > > appeared on machines that use usbguard: the netdev appears only when the
> > > USB device is inserted the first time (before the module is loaded), but
> > > on the second and next insertions no netdev is registered.
> > >
> > > It happens because the device is probed as unauthorized, and usbguard
> > > gives it an authorization a moment later. If the module is not loaded,
> > > it's normally loaded slower than the authorization is given, and
> > > everything works. If the module is already loaded, the cfgselector's
> > > probe function runs first, but then usb_authorize_device kicks in and
> > > changes the configuration to something chosen by the standard
> > > usb_choose_configuration. rtl8152_probe refuses to probe non-vendor
> > > configurations, and the user ends up without a netdev.
> > >
> > > The previous commit added possibility to override
> > > usb_choose_configuration. Use it to fix the bug and pick the right
> > > configuration on both probe and authorization.
> > >
> > > Fixes: ec51fbd1b8a2 ("r8152: add USB device driver for config selection")
> > > Signed-off-by: Maxim Mikityanskiy <maxtram95@gmail.com>
> >
> > Maxim,
> > Does this solve the same problem that Doug Anderson posted patches for
> > a few weeks ago?
> >
> > https://lore.kernel.org/all/20231201183113.343256-1-dianders@chromium.org/
> >
> > Those went through the USB tree, so that probably explains why you
> > didn't see them in the net.git tree.

Oh right, I didn't see these... Sorry for making noise then, Doug's
patches should solve my problem.

> Specifically, usb.git usb-next branch:
> "r8152: Choose our USB config with choose_configuration() rather than probe()"
> https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git/commit/drivers/net/usb/r8152.c?h=usb-next&id=aa4f2b3e418e8673e55145de8b8016a7a9920306
> 
> Given this has a fixes tag, and two people have now tracked this down
> and posted fixes for this problem, is there any chance of this landing
> in 6.7?
> 
> And it looks like Maxim's change could be rebased on top of Doug's
> patch and clean up a few more things. Do I see that correctly?

I only see some minor difference in error handling, not sure it's worth
chasing for.

> cheers,
> grant
> 
> >
> > cheers,
> > grant
> >
> > > ---
> > >  drivers/net/usb/r8152.c | 18 +++++++++---------
> > >  1 file changed, 9 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> > > index 9bf2140fd0a1..f0ac31a94f3c 100644
> > > --- a/drivers/net/usb/r8152.c
> > > +++ b/drivers/net/usb/r8152.c
> > > @@ -10070,6 +10070,11 @@ static struct usb_driver rtl8152_driver = {
> > >  };
> > >
> > >  static int rtl8152_cfgselector_probe(struct usb_device *udev)
> > > +{
> > > +       return 0;
> > > +}
> > > +
> > > +static int rtl8152_cfgselector_choose_configuration(struct usb_device *udev)
> > >  {
> > >         struct usb_host_config *c;
> > >         int i, num_configs;
> > > @@ -10078,7 +10083,7 @@ static int rtl8152_cfgselector_probe(struct usb_device *udev)
> > >          * driver supports it.
> > >          */
> > >         if (__rtl_get_hw_ver(udev) == RTL_VER_UNKNOWN)
> > > -               return 0;
> > > +               return -EOPNOTSUPP;
> > >
> > >         /* The vendor mode is not always config #1, so to find it out. */
> > >         c = udev->config;
> > > @@ -10094,20 +10099,15 @@ static int rtl8152_cfgselector_probe(struct usb_device *udev)
> > >         }
> > >
> > >         if (i == num_configs)
> > > -               return -ENODEV;
> > > -
> > > -       if (usb_set_configuration(udev, c->desc.bConfigurationValue)) {
> > > -               dev_err(&udev->dev, "Failed to set configuration %d\n",
> > > -                       c->desc.bConfigurationValue);
> > > -               return -ENODEV;
> > > -       }
> > > +               return -EOPNOTSUPP;
> > >
> > > -       return 0;
> > > +       return c->desc.bConfigurationValue;
> > >  }
> > >
> > >  static struct usb_device_driver rtl8152_cfgselector_driver = {
> > >         .name =         MODULENAME "-cfgselector",
> > >         .probe =        rtl8152_cfgselector_probe,
> > > +       .choose_configuration = rtl8152_cfgselector_choose_configuration,
> > >         .id_table =     rtl8152_table,
> > >         .generic_subclass = 1,
> > >         .supports_autosuspend = 1,
> > > --
> > > 2.43.0
> > >