[PATCH v1] usb: kbd: destroy device after console is stopped

[PATCH v1] usb: kbd: destroy device after console is stopped

[Andy Shevchenko]

In case of IOMUX enabled it assumes that console devices in the list
are available to get them stopped properly via ->stop() callback.
However, the USB keyboard driver violates this assumption and tries
to play tricks so the device get destroyed while being listed as
an active console.

Swap the order of device deregistration and IOMUX update to avoid
the use-after-free.

Fixes: 3cbcb2892809 ("usb: Fix usb_kbd_deregister when console-muxing is used")
Fixes: 8a8348703081 ("dm: usb: Add a remove() method for USB keyboards")
Reported-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v2: Nicolas, can you test this one instead of yours?
 common/usb_kbd.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/common/usb_kbd.c b/common/usb_kbd.c
index b316807844b1..e09d9f7794c8 100644
--- a/common/usb_kbd.c
+++ b/common/usb_kbd.c
@@ -617,12 +617,12 @@ int usb_kbd_deregister(int force)
 	if (dev) {
 		usb_kbd_dev = (struct usb_device *)dev->priv;
 		data = usb_kbd_dev->privptr;
-		if (stdio_deregister_dev(dev, force) != 0)
-			return 1;
 #if CONFIG_IS_ENABLED(CONSOLE_MUX)
 		if (iomux_doenv(stdin, env_get("stdin")) != 0)
 			return 1;
 #endif
+		if (stdio_deregister_dev(dev, force) != 0)
+			return 1;
 #ifdef CONFIG_SYS_USB_EVENT_POLL_VIA_INT_QUEUE
 		destroy_int_queue(usb_kbd_dev, data->intq);
 #endif
@@ -660,16 +660,16 @@ static int usb_kbd_remove(struct udevice *dev)
 		goto err;
 	}
 	data = udev->privptr;
-	if (stdio_deregister_dev(sdev, true)) {
-		ret = -EPERM;
-		goto err;
-	}
 #if CONFIG_IS_ENABLED(CONSOLE_MUX)
 	if (iomux_doenv(stdin, env_get("stdin"))) {
 		ret = -ENOLINK;
 		goto err;
 	}
 #endif
+	if (stdio_deregister_dev(sdev, true)) {
+		ret = -EPERM;
+		goto err;
+	}
 #ifdef CONFIG_SYS_USB_EVENT_POLL_VIA_INT_QUEUE
 	destroy_int_queue(udev, data->intq);
 #endif
-- 
2.29.2

[PATCH v1] usb: kbd: destroy device after console is stopped

[Nicolas Saenz Julienne]

Hi Andy,

On Thu, 2021-01-28 at 18:55 +0200, Andy Shevchenko wrote:
> In case of IOMUX enabled it assumes that console devices in the list
> are available to get them stopped properly via ->stop() callback.
> However, the USB keyboard driver violates this assumption and tries
> to play tricks so the device get destroyed while being listed as
> an active console.
> 
> Swap the order of device deregistration and IOMUX update to avoid
> the use-after-free.
> 
> Fixes: 3cbcb2892809 ("usb: Fix usb_kbd_deregister when console-muxing is used")
> Fixes: 8a8348703081 ("dm: usb: Add a remove() method for USB keyboards")
> Reported-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> v2: Nicolas, can you test this one instead of yours?

Sadly this doesn't seem to work, and breaks a bunch of other tests in the
process. You can try it yourself by running: './test/py/test.py --bd sandbox
--build'

Regards,
Nicolas

> ?common/usb_kbd.c | 12 ++++++------
> ?1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/common/usb_kbd.c b/common/usb_kbd.c
> index b316807844b1..e09d9f7794c8 100644
> --- a/common/usb_kbd.c
> +++ b/common/usb_kbd.c
> @@ -617,12 +617,12 @@ int usb_kbd_deregister(int force)
> ?	if (dev) {
> ?		usb_kbd_dev = (struct usb_device *)dev->priv;
> ?		data = usb_kbd_dev->privptr;
> -		if (stdio_deregister_dev(dev, force) != 0)
> -			return 1;
> ?#if CONFIG_IS_ENABLED(CONSOLE_MUX)
> ?		if (iomux_doenv(stdin, env_get("stdin")) != 0)
> ?			return 1;
> ?#endif
> +		if (stdio_deregister_dev(dev, force) != 0)
> +			return 1;
> ?#ifdef CONFIG_SYS_USB_EVENT_POLL_VIA_INT_QUEUE
> ?		destroy_int_queue(usb_kbd_dev, data->intq);
> ?#endif
> @@ -660,16 +660,16 @@ static int usb_kbd_remove(struct udevice *dev)
> ?		goto err;
> ?	}
> ?	data = udev->privptr;
> -	if (stdio_deregister_dev(sdev, true)) {
> -		ret = -EPERM;
> -		goto err;
> -	}
> ?#if CONFIG_IS_ENABLED(CONSOLE_MUX)
> ?	if (iomux_doenv(stdin, env_get("stdin"))) {
> ?		ret = -ENOLINK;
> ?		goto err;
> ?	}
> ?#endif
> +	if (stdio_deregister_dev(sdev, true)) {
> +		ret = -EPERM;
> +		goto err;
> +	}
> ?#ifdef CONFIG_SYS_USB_EVENT_POLL_VIA_INT_QUEUE
> ?	destroy_int_queue(udev, data->intq);
> ?#endif



-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: This is a digitally signed message part
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210128/829347a0/attachment.sig>

[PATCH v1] usb: kbd: destroy device after console is stopped

[Andy Shevchenko]

On Thu, Jan 28, 2021 at 06:19:56PM +0100, Nicolas Saenz Julienne wrote:
> Hi Andy,
> 
> On Thu, 2021-01-28 at 18:55 +0200, Andy Shevchenko wrote:
> > In case of IOMUX enabled it assumes that console devices in the list
> > are available to get them stopped properly via ->stop() callback.
> > However, the USB keyboard driver violates this assumption and tries
> > to play tricks so the device get destroyed while being listed as
> > an active console.
> > 
> > Swap the order of device deregistration and IOMUX update to avoid
> > the use-after-free.
> > 
> > Fixes: 3cbcb2892809 ("usb: Fix usb_kbd_deregister when console-muxing is used")
> > Fixes: 8a8348703081 ("dm: usb: Add a remove() method for USB keyboards")
> > Reported-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> > v2: Nicolas, can you test this one instead of yours?
> 
> Sadly this doesn't seem to work, and breaks a bunch of other tests in the
> process. You can try it yourself by running: './test/py/test.py --bd sandbox
> --build'

Thanks for trying.

Unfortunately I have unrelated bug somewhere:
Traceback (most recent call last):
  File "/home/andy/prj/u-boot/./test/py/test.py", line 20, in <module>
    sys.exit(load_entry_point('pytest', 'console_scripts', 'pytest')(args))
TypeError: console_main() takes 0 positional arguments but 1 was given

-- 
With Best Regards,
Andy Shevchenko

[PATCH v1] usb: kbd: destroy device after console is stopped

[Andy Shevchenko]

On Thu, Jan 28, 2021 at 07:46:49PM +0200, Andy Shevchenko wrote:
> On Thu, Jan 28, 2021 at 06:19:56PM +0100, Nicolas Saenz Julienne wrote:
> > Hi Andy,
> > 
> > On Thu, 2021-01-28 at 18:55 +0200, Andy Shevchenko wrote:
> > > In case of IOMUX enabled it assumes that console devices in the list
> > > are available to get them stopped properly via ->stop() callback.
> > > However, the USB keyboard driver violates this assumption and tries
> > > to play tricks so the device get destroyed while being listed as
> > > an active console.
> > > 
> > > Swap the order of device deregistration and IOMUX update to avoid
> > > the use-after-free.
> > > 
> > > Fixes: 3cbcb2892809 ("usb: Fix usb_kbd_deregister when console-muxing is used")
> > > Fixes: 8a8348703081 ("dm: usb: Add a remove() method for USB keyboards")
> > > Reported-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > ---
> > > v2: Nicolas, can you test this one instead of yours?
> > 
> > Sadly this doesn't seem to work, and breaks a bunch of other tests in the
> > process. You can try it yourself by running: './test/py/test.py --bd sandbox
> > --build'
> 
> Thanks for trying.
> 
> Unfortunately I have unrelated bug somewhere:
> Traceback (most recent call last):
>   File "/home/andy/prj/u-boot/./test/py/test.py", line 20, in <module>
>     sys.exit(load_entry_point('pytest', 'console_scripts', 'pytest')(args))
> TypeError: console_main() takes 0 positional arguments but 1 was given

Seems test cases are broken in U-Boot.
I'm not sure how you were able to run them.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v1] usb: kbd: destroy device after console is stopped

[Tom Rini]

On Thu, Jan 28, 2021 at 07:52:36PM +0200, Andy Shevchenko wrote:
> On Thu, Jan 28, 2021 at 07:46:49PM +0200, Andy Shevchenko wrote:
> > On Thu, Jan 28, 2021 at 06:19:56PM +0100, Nicolas Saenz Julienne wrote:
> > > Hi Andy,
> > > 
> > > On Thu, 2021-01-28 at 18:55 +0200, Andy Shevchenko wrote:
> > > > In case of IOMUX enabled it assumes that console devices in the list
> > > > are available to get them stopped properly via ->stop() callback.
> > > > However, the USB keyboard driver violates this assumption and tries
> > > > to play tricks so the device get destroyed while being listed as
> > > > an active console.
> > > > 
> > > > Swap the order of device deregistration and IOMUX update to avoid
> > > > the use-after-free.
> > > > 
> > > > Fixes: 3cbcb2892809 ("usb: Fix usb_kbd_deregister when console-muxing is used")
> > > > Fixes: 8a8348703081 ("dm: usb: Add a remove() method for USB keyboards")
> > > > Reported-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > > ---
> > > > v2: Nicolas, can you test this one instead of yours?
> > > 
> > > Sadly this doesn't seem to work, and breaks a bunch of other tests in the
> > > process. You can try it yourself by running: './test/py/test.py --bd sandbox
> > > --build'
> > 
> > Thanks for trying.
> > 
> > Unfortunately I have unrelated bug somewhere:
> > Traceback (most recent call last):
> >   File "/home/andy/prj/u-boot/./test/py/test.py", line 20, in <module>
> >     sys.exit(load_entry_point('pytest', 'console_scripts', 'pytest')(args))
> > TypeError: console_main() takes 0 positional arguments but 1 was given
> 
> Seems test cases are broken in U-Boot.
> I'm not sure how you were able to run them.

This is I guess what Heinrich was posting a patch for earlier today.
The supported way to run the tests (so that they're the same for
everyone) is to use "pip" and "pip install -r test/py/requirements.txt".

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210128/a72b26a6/attachment.sig>

[PATCH v1] usb: kbd: destroy device after console is stopped

[Andy Shevchenko]

On Thu, Jan 28, 2021 at 07:52:36PM +0200, Andy Shevchenko wrote:
> On Thu, Jan 28, 2021 at 07:46:49PM +0200, Andy Shevchenko wrote:
> > On Thu, Jan 28, 2021 at 06:19:56PM +0100, Nicolas Saenz Julienne wrote:
> > > Hi Andy,
> > > 
> > > On Thu, 2021-01-28 at 18:55 +0200, Andy Shevchenko wrote:
> > > > In case of IOMUX enabled it assumes that console devices in the list
> > > > are available to get them stopped properly via ->stop() callback.
> > > > However, the USB keyboard driver violates this assumption and tries
> > > > to play tricks so the device get destroyed while being listed as
> > > > an active console.
> > > > 
> > > > Swap the order of device deregistration and IOMUX update to avoid
> > > > the use-after-free.
> > > > 
> > > > Fixes: 3cbcb2892809 ("usb: Fix usb_kbd_deregister when console-muxing is used")
> > > > Fixes: 8a8348703081 ("dm: usb: Add a remove() method for USB keyboards")
> > > > Reported-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > > ---
> > > > v2: Nicolas, can you test this one instead of yours?
> > > 
> > > Sadly this doesn't seem to work, and breaks a bunch of other tests in the
> > > process. You can try it yourself by running: './test/py/test.py --bd sandbox
> > > --build'
> > 
> > Thanks for trying.
> > 
> > Unfortunately I have unrelated bug somewhere:
> > Traceback (most recent call last):
> >   File "/home/andy/prj/u-boot/./test/py/test.py", line 20, in <module>
> >     sys.exit(load_entry_point('pytest', 'console_scripts', 'pytest')(args))
> > TypeError: console_main() takes 0 positional arguments but 1 was given
> 
> Seems test cases are broken in U-Boot.
> I'm not sure how you were able to run them.

Tom, any insights? Seems your patch changed the way how test.py works and
something wrong happened meanwhile.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v1] usb: kbd: destroy device after console is stopped

[Andy Shevchenko]

On Thu, Jan 28, 2021 at 12:58:30PM -0500, Tom Rini wrote:
> On Thu, Jan 28, 2021 at 07:52:36PM +0200, Andy Shevchenko wrote:
> > On Thu, Jan 28, 2021 at 07:46:49PM +0200, Andy Shevchenko wrote:
> > > On Thu, Jan 28, 2021 at 06:19:56PM +0100, Nicolas Saenz Julienne wrote:
> > > > Hi Andy,
> > > > 
> > > > On Thu, 2021-01-28 at 18:55 +0200, Andy Shevchenko wrote:
> > > > > In case of IOMUX enabled it assumes that console devices in the list
> > > > > are available to get them stopped properly via ->stop() callback.
> > > > > However, the USB keyboard driver violates this assumption and tries
> > > > > to play tricks so the device get destroyed while being listed as
> > > > > an active console.
> > > > > 
> > > > > Swap the order of device deregistration and IOMUX update to avoid
> > > > > the use-after-free.
> > > > > 
> > > > > Fixes: 3cbcb2892809 ("usb: Fix usb_kbd_deregister when console-muxing is used")
> > > > > Fixes: 8a8348703081 ("dm: usb: Add a remove() method for USB keyboards")
> > > > > Reported-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > > > ---
> > > > > v2: Nicolas, can you test this one instead of yours?
> > > > 
> > > > Sadly this doesn't seem to work, and breaks a bunch of other tests in the
> > > > process. You can try it yourself by running: './test/py/test.py --bd sandbox
> > > > --build'
> > > 
> > > Thanks for trying.
> > > 
> > > Unfortunately I have unrelated bug somewhere:
> > > Traceback (most recent call last):
> > >   File "/home/andy/prj/u-boot/./test/py/test.py", line 20, in <module>
> > >     sys.exit(load_entry_point('pytest', 'console_scripts', 'pytest')(args))
> > > TypeError: console_main() takes 0 positional arguments but 1 was given
> > 
> > Seems test cases are broken in U-Boot.
> > I'm not sure how you were able to run them.
> 
> This is I guess what Heinrich was posting a patch for earlier today.
> The supported way to run the tests (so that they're the same for
> everyone) is to use "pip" and "pip install -r test/py/requirements.txt".

My Gosh! It is full of package == version, which is simply awful. Can it be
more flexible?

I hate this Python hell.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v1] usb: kbd: destroy device after console is stopped

[Tom Rini]

On Thu, Jan 28, 2021 at 08:18:47PM +0200, Andy Shevchenko wrote:
> On Thu, Jan 28, 2021 at 12:58:30PM -0500, Tom Rini wrote:
> > On Thu, Jan 28, 2021 at 07:52:36PM +0200, Andy Shevchenko wrote:
> > > On Thu, Jan 28, 2021 at 07:46:49PM +0200, Andy Shevchenko wrote:
> > > > On Thu, Jan 28, 2021 at 06:19:56PM +0100, Nicolas Saenz Julienne wrote:
> > > > > Hi Andy,
> > > > > 
> > > > > On Thu, 2021-01-28 at 18:55 +0200, Andy Shevchenko wrote:
> > > > > > In case of IOMUX enabled it assumes that console devices in the list
> > > > > > are available to get them stopped properly via ->stop() callback.
> > > > > > However, the USB keyboard driver violates this assumption and tries
> > > > > > to play tricks so the device get destroyed while being listed as
> > > > > > an active console.
> > > > > > 
> > > > > > Swap the order of device deregistration and IOMUX update to avoid
> > > > > > the use-after-free.
> > > > > > 
> > > > > > Fixes: 3cbcb2892809 ("usb: Fix usb_kbd_deregister when console-muxing is used")
> > > > > > Fixes: 8a8348703081 ("dm: usb: Add a remove() method for USB keyboards")
> > > > > > Reported-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> > > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > > > > ---
> > > > > > v2: Nicolas, can you test this one instead of yours?
> > > > > 
> > > > > Sadly this doesn't seem to work, and breaks a bunch of other tests in the
> > > > > process. You can try it yourself by running: './test/py/test.py --bd sandbox
> > > > > --build'
> > > > 
> > > > Thanks for trying.
> > > > 
> > > > Unfortunately I have unrelated bug somewhere:
> > > > Traceback (most recent call last):
> > > >   File "/home/andy/prj/u-boot/./test/py/test.py", line 20, in <module>
> > > >     sys.exit(load_entry_point('pytest', 'console_scripts', 'pytest')(args))
> > > > TypeError: console_main() takes 0 positional arguments but 1 was given
> > > 
> > > Seems test cases are broken in U-Boot.
> > > I'm not sure how you were able to run them.
> > 
> > This is I guess what Heinrich was posting a patch for earlier today.
> > The supported way to run the tests (so that they're the same for
> > everyone) is to use "pip" and "pip install -r test/py/requirements.txt".
> 
> My Gosh! It is full of package == version, which is simply awful. Can it be
> more flexible?
> 
> I hate this Python hell.

It's intentional to follow the best practices of using python packages
as best I can tell.  CI doesn't mean so much if it's not repeatable and
non-versioned packages mean that you can't be sure that running a test 6
months from now gives you what you get today on the same code base.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210128/68c65697/attachment.sig>

[PATCH v1] usb: kbd: destroy device after console is stopped

[Andy Shevchenko]

On Thu, Jan 28, 2021 at 01:24:04PM -0500, Tom Rini wrote:
> On Thu, Jan 28, 2021 at 08:18:47PM +0200, Andy Shevchenko wrote:
> > On Thu, Jan 28, 2021 at 12:58:30PM -0500, Tom Rini wrote:
> > > On Thu, Jan 28, 2021 at 07:52:36PM +0200, Andy Shevchenko wrote:
> > > > On Thu, Jan 28, 2021 at 07:46:49PM +0200, Andy Shevchenko wrote:
> > > > > On Thu, Jan 28, 2021 at 06:19:56PM +0100, Nicolas Saenz Julienne wrote:

...

> > > > > Unfortunately I have unrelated bug somewhere:
> > > > > Traceback (most recent call last):
> > > > >   File "/home/andy/prj/u-boot/./test/py/test.py", line 20, in <module>
> > > > >     sys.exit(load_entry_point('pytest', 'console_scripts', 'pytest')(args))
> > > > > TypeError: console_main() takes 0 positional arguments but 1 was given
> > > > 
> > > > Seems test cases are broken in U-Boot.
> > > > I'm not sure how you were able to run them.
> > > 
> > > This is I guess what Heinrich was posting a patch for earlier today.
> > > The supported way to run the tests (so that they're the same for
> > > everyone) is to use "pip" and "pip install -r test/py/requirements.txt".
> > 
> > My Gosh! It is full of package == version, which is simply awful. Can it be
> > more flexible?
> > 
> > I hate this Python hell.
> 
> It's intentional to follow the best practices of using python packages
> as best I can tell.  CI doesn't mean so much if it's not repeatable and
> non-versioned packages mean that you can't be sure that running a test 6
> months from now gives you what you get today on the same code base.

I agree on CI point, I disagree on end user's point of view.
I have a distribution that has provided all required packages. Moreover, I have
another (locally installed) packages that may require something else. The Py
virtual environment maybe a way to solve this, but it's like an exploitation of
the hi-tech laboratory to chop a firewood.

And as I told, it's not your fault, it's "best practices of using Py packages".

-- 
With Best Regards,
Andy Shevchenko

[PATCH v1] usb: kbd: destroy device after console is stopped

[Andy Shevchenko]

On Thu, Jan 28, 2021 at 06:19:56PM +0100, Nicolas Saenz Julienne wrote:
> Hi Andy,
> 
> On Thu, 2021-01-28 at 18:55 +0200, Andy Shevchenko wrote:
> > In case of IOMUX enabled it assumes that console devices in the list
> > are available to get them stopped properly via ->stop() callback.
> > However, the USB keyboard driver violates this assumption and tries
> > to play tricks so the device get destroyed while being listed as
> > an active console.
> > 
> > Swap the order of device deregistration and IOMUX update to avoid
> > the use-after-free.
> > 
> > Fixes: 3cbcb2892809 ("usb: Fix usb_kbd_deregister when console-muxing is used")
> > Fixes: 8a8348703081 ("dm: usb: Add a remove() method for USB keyboards")
> > Reported-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> > v2: Nicolas, can you test this one instead of yours?
> 
> Sadly this doesn't seem to work, and breaks a bunch of other tests in the
> process. You can try it yourself by running: './test/py/test.py --bd sandbox
> --build'

Now I'm able to run test cases. I see some of them failing even without my
patch, but few definitely related. Can you give a list of failed ones on your
side?  I can compare that we are on the same page here.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v1] usb: kbd: destroy device after console is stopped

[Tom Rini]

On Thu, Jan 28, 2021 at 10:35:37PM +0200, Andy Shevchenko wrote:
> On Thu, Jan 28, 2021 at 06:19:56PM +0100, Nicolas Saenz Julienne wrote:
> > Hi Andy,
> > 
> > On Thu, 2021-01-28 at 18:55 +0200, Andy Shevchenko wrote:
> > > In case of IOMUX enabled it assumes that console devices in the list
> > > are available to get them stopped properly via ->stop() callback.
> > > However, the USB keyboard driver violates this assumption and tries
> > > to play tricks so the device get destroyed while being listed as
> > > an active console.
> > > 
> > > Swap the order of device deregistration and IOMUX update to avoid
> > > the use-after-free.
> > > 
> > > Fixes: 3cbcb2892809 ("usb: Fix usb_kbd_deregister when console-muxing is used")
> > > Fixes: 8a8348703081 ("dm: usb: Add a remove() method for USB keyboards")
> > > Reported-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > ---
> > > v2: Nicolas, can you test this one instead of yours?
> > 
> > Sadly this doesn't seem to work, and breaks a bunch of other tests in the
> > process. You can try it yourself by running: './test/py/test.py --bd sandbox
> > --build'
> 
> Now I'm able to run test cases. I see some of them failing even without my
> patch, but few definitely related. Can you give a list of failed ones on your
> side?  I can compare that we are on the same page here.

Running this here on sandbox I get:
FAILED test/py/tests/test_ut.py::test_ut[ut_dm_bootcount] - OSError: [Errno 5] Input/output ...
FAILED test/py/tests/test_ut.py::test_ut[ut_dm_usb_flash] - OSError: [Errno 5] Input/output ...
FAILED test/py/tests/test_ut.py::test_ut[ut_dm_usb_multi] - OSError: [Errno 5] Input/output ...
FAILED test/py/tests/test_ut.py::test_ut[ut_dm_video_ansi] - OSError: [Errno 5] Input/output...

and those 4 are segfaults of sandbox.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210128/e08d487d/attachment.sig>

[PATCH v1] usb: kbd: destroy device after console is stopped

[Andy Shevchenko]

On Thu, Jan 28, 2021 at 10:47 PM Tom Rini <trini@konsulko.com> wrote:
> On Thu, Jan 28, 2021 at 10:35:37PM +0200, Andy Shevchenko wrote:
> > On Thu, Jan 28, 2021 at 06:19:56PM +0100, Nicolas Saenz Julienne wrote:
> > > On Thu, 2021-01-28 at 18:55 +0200, Andy Shevchenko wrote:

...

> > > Sadly this doesn't seem to work, and breaks a bunch of other tests in the
> > > process. You can try it yourself by running: './test/py/test.py --bd sandbox
> > > --build'
> >
> > Now I'm able to run test cases. I see some of them failing even without my
> > patch, but few definitely related. Can you give a list of failed ones on your
> > side?  I can compare that we are on the same page here.
>
> Running this here on sandbox I get:
> FAILED test/py/tests/test_ut.py::test_ut[ut_dm_bootcount] - OSError: [Errno 5] Input/output ...
> FAILED test/py/tests/test_ut.py::test_ut[ut_dm_usb_flash] - OSError: [Errno 5] Input/output ...
> FAILED test/py/tests/test_ut.py::test_ut[ut_dm_usb_multi] - OSError: [Errno 5] Input/output ...
> FAILED test/py/tests/test_ut.py::test_ut[ut_dm_video_ansi] - OSError: [Errno 5] Input/output...
>
> and those 4 are segfaults of sandbox.

Right, I have got the same. Thanks.

Actually it's a can of worms I have opened. I already see a mess with
the console assignment between devices.
I will check later how possibly to fix this (to some extent), today is
almost midnight here...

-- 
With Best Regards,
Andy Shevchenko

[PATCH v1] usb: kbd: destroy device after console is stopped

[Andy Shevchenko]

On Thu, Jan 28, 2021 at 10:47 PM Tom Rini <trini@konsulko.com> wrote:
> On Thu, Jan 28, 2021 at 10:35:37PM +0200, Andy Shevchenko wrote:

> Running this here on sandbox I get:
> FAILED test/py/tests/test_ut.py::test_ut[ut_dm_bootcount] - OSError: [Errno 5] Input/output ...

Btw this is the wrong test case name printed. The real one which
crashed is blk_usb (previous one to above mentioned). And it's failing
only when you run either blk_usb or acpi_basic before (I haven't
checked other permutations). Consider this as a bug report against
tests.

> FAILED test/py/tests/test_ut.py::test_ut[ut_dm_usb_flash] - OSError: [Errno 5] Input/output ...
> FAILED test/py/tests/test_ut.py::test_ut[ut_dm_usb_multi] - OSError: [Errno 5] Input/output ...
> FAILED test/py/tests/test_ut.py::test_ut[ut_dm_video_ansi] - OSError: [Errno 5] Input/output...
>
> and those 4 are segfaults of sandbox.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v1] usb: kbd: destroy device after console is stopped

[Nicolas Saenz Julienne]

Andy, Tony,
Sorry for my late reply, but I got a bad cold.

On Thu, 2021-01-28 at 15:46 -0500, Tom Rini wrote:
> On Thu, Jan 28, 2021 at 10:35:37PM +0200, Andy Shevchenko wrote:
> > On Thu, Jan 28, 2021 at 06:19:56PM +0100, Nicolas Saenz Julienne wrote:
> > > Hi Andy,
> > > 
> > > On Thu, 2021-01-28 at 18:55 +0200, Andy Shevchenko wrote:
> > > > In case of IOMUX enabled it assumes that console devices in the list
> > > > are available to get them stopped properly via ->stop() callback.
> > > > However, the USB keyboard driver violates this assumption and tries
> > > > to play tricks so the device get destroyed while being listed as
> > > > an active console.
> > > > 
> > > > Swap the order of device deregistration and IOMUX update to avoid
> > > > the use-after-free.
> > > > 
> > > > Fixes: 3cbcb2892809 ("usb: Fix usb_kbd_deregister when console-muxing is used")
> > > > Fixes: 8a8348703081 ("dm: usb: Add a remove() method for USB keyboards")
> > > > Reported-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > > ---
> > > > v2: Nicolas, can you test this one instead of yours?
> > > 
> > > Sadly this doesn't seem to work, and breaks a bunch of other tests in the
> > > process. You can try it yourself by running: './test/py/test.py --bd sandbox
> > > --build'
> > 
> > Now I'm able to run test cases. I see some of them failing even without my
> > patch, but few definitely related. Can you give a list of failed ones on your
> > side?  I can compare that we are on the same page here.
> 
> Running this here on sandbox I get:
> FAILED test/py/tests/test_ut.py::test_ut[ut_dm_bootcount] - OSError: [Errno 5] Input/output ...
> FAILED test/py/tests/test_ut.py::test_ut[ut_dm_usb_flash] - OSError: [Errno 5] Input/output ...
> FAILED test/py/tests/test_ut.py::test_ut[ut_dm_usb_multi] - OSError: [Errno 5] Input/output ...
> FAILED test/py/tests/test_ut.py::test_ut[ut_dm_video_ansi] - OSError: [Errno 5] Input/output...

This is what I'm seeing too.

Regards,
Nicolas

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: This is a digitally signed message part
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210203/eb8c59b3/attachment.sig>

[PATCH v1] usb: kbd: destroy device after console is stopped

[Andy Shevchenko]

On Wed, Feb 03, 2021 at 01:25:38PM +0100, Nicolas Saenz Julienne wrote:
> Andy, Tony,
> Sorry for my late reply, but I got a bad cold.

I hope you are doing well.

Unfortunately I haven't heard from you lately, so I have decided to send out
whatever I have as a patch series. You are Cc'ed on the last patch (it has
prerequisites, so better to apply entire series for easy going).

That said, I have dropped the 'iomux' branch from my public tree.

> On Thu, 2021-01-28 at 15:46 -0500, Tom Rini wrote:
> > On Thu, Jan 28, 2021 at 10:35:37PM +0200, Andy Shevchenko wrote:
> > > On Thu, Jan 28, 2021 at 06:19:56PM +0100, Nicolas Saenz Julienne wrote:
> > > > Hi Andy,
> > > > 
> > > > On Thu, 2021-01-28 at 18:55 +0200, Andy Shevchenko wrote:
> > > > > In case of IOMUX enabled it assumes that console devices in the list
> > > > > are available to get them stopped properly via ->stop() callback.
> > > > > However, the USB keyboard driver violates this assumption and tries
> > > > > to play tricks so the device get destroyed while being listed as
> > > > > an active console.
> > > > > 
> > > > > Swap the order of device deregistration and IOMUX update to avoid
> > > > > the use-after-free.
> > > > > 
> > > > > Fixes: 3cbcb2892809 ("usb: Fix usb_kbd_deregister when console-muxing is used")
> > > > > Fixes: 8a8348703081 ("dm: usb: Add a remove() method for USB keyboards")
> > > > > Reported-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > > > ---
> > > > > v2: Nicolas, can you test this one instead of yours?
> > > > 
> > > > Sadly this doesn't seem to work, and breaks a bunch of other tests in the
> > > > process. You can try it yourself by running: './test/py/test.py --bd sandbox
> > > > --build'
> > > 
> > > Now I'm able to run test cases. I see some of them failing even without my
> > > patch, but few definitely related. Can you give a list of failed ones on your
> > > side?  I can compare that we are on the same page here.
> > 
> > Running this here on sandbox I get:
> > FAILED test/py/tests/test_ut.py::test_ut[ut_dm_bootcount] - OSError: [Errno 5] Input/output ...
> > FAILED test/py/tests/test_ut.py::test_ut[ut_dm_usb_flash] - OSError: [Errno 5] Input/output ...
> > FAILED test/py/tests/test_ut.py::test_ut[ut_dm_usb_multi] - OSError: [Errno 5] Input/output ...
> > FAILED test/py/tests/test_ut.py::test_ut[ut_dm_video_ansi] - OSError: [Errno 5] Input/output...
> 
> This is what I'm seeing too.

-- 
With Best Regards,
Andy Shevchenko