mail archive of the barebox mailing list
 help / color / mirror / Atom feed
* [Barebox] Polling infrastructure causes reentrancy problem
@ 2021-11-05 12:40 Christian Eggers
  2021-11-08 10:34 ` Sascha Hauer
  0 siblings, 1 reply; 3+ messages in thread
From: Christian Eggers @ 2021-11-05 12:40 UTC (permalink / raw)
  To: Ahmad Fatoum, Sascha Hauer, Marc Kleine-Budde; +Cc: barebox

Disclaimer: I stumbled over this on barebox 2020.01.
I haven't tried to reproduce this on the current version.

Summary:
----------
Use case: Linux kernel is started via Android fastboot (uses polling infrastructure) while probing of other device drivers is ongoing.
Behavior: usb_stor_disconnect() is called before usb_stor_probe() returned
Result: NULL pointer dereference in usb_stor_disconnect()


Details:
----------
I use fastboot over USB gadget for loading a FIT image (I have
stripped down/modified the Android fastboot command for this).
The fastboot command (on the PC) is executed via udev, so the
transfer of the FIT image starts as soon as fastboot has been
activated on the barebox side.

"Parallel" to the fastboot transfer, my (custom) barebox_main
scans for USB devices (e.g. for alternative recovery option via
USB thumb driver). During detection/registration of the first
USB drive, there are delay() statements which causes progress
on the fastboot file transfer. As soon as the FIT transfer has
finished, do_bootm_linux/shutdown_barebox is called. Here the
USB storage device is disconnected without ever having been
fully probed.

Call stack (start reading from bottom):
----------
-000|usb_stor_disconnect(usbdev = 0x9010D0B0)
--> usb_stor_disconnect is called with a partially probed device
--> NULL pointer dereference 

/* Handle a USB mass-storage disconnect */
static void usb_stor_disconnect(struct usb_device *usbdev)
{
	struct us_data *us = (struct us_data *)usbdev->drv_data;
--> us = NULL!
	struct us_blk_dev *bdev, *bdev_tmp;

...

-001|devices_shutdown()
-002|shutdown_barebox()
-003|start_linux(adr = 0x82000000, swap = 0, initrd_address = ?, initrd_size = 5739640, oftree = 0x828A20
-004|__do_bootm_linux(:data = 0x915FF560, free_mem = 2190090240, swap = 0, fdt = ?)
-005|do_bootm_linux(:data = 0x915FF560)
-006|bootm_boot(:bootm_data = 0x9FFEFBF8)
-007|cb_boot(:f_fb = 0x8FE644B0, opt = ?)
-008|fb_run_command(inline)
--> fastboot transfer is finished
-008|rx_handler_command(:ep = 0x8FE53050, :req = 0x8FE645F8)
-009|list_del_init(inline)
-009|done(:ep = 0x8FE53050, :req = 0x8FE645F8, status = ?)
-010|dtd_complete_irq(inline)
-010|fsl_udc_gadget_poll(inline)
-010|fsl_udc_gadget_poll(gadget = 0x8FE52EA0)
-011|poller_call()
-012|is_timeout(:start_ns = 34359738368, :time_offset_ns = 0)
-013|udelay(:usecs = 100)
--> delay caues progress on fastboot transfer
-013|mdelay(tailcall)
-014|usb_stor_transport(usb_blkdev = 0x9038DA48, cmd = 0x9FFEFD40 -> "", cmdlen = ?, data = 0x0, datalen
-015|usb_stor_test_unit_ready(:usb_blkdev = 0x9038DA48)
-016|usb_stor_init_blkdev(inline)
-016|usb_stor_add_blkdev(inline)
-016|usb_stor_scan(inline)
-016|usb_stor_probe(:usbdev = 0x9010DBC0, id = ?)
--> usb_store_probe() starts registration of USB device
-017|list_add(inline)
-017|device_probe(:dev = 0x9010DBC0)
-018|match(:drv = 0x9FC4B11C, :dev = 0x9010DBC0)
-019|list_add_tail(inline)
-019|register_device(:new_device = 0x9010DBC0)
-020|usb_new_device(:dev = 0x9010D0B0)
-021|usb_hub_port_connect_change(:dev = 0x8FE69A30, :port = 4)
-022|usb_scan_port(inline)
-022|usb_device_list_scan(inline)
-022|usb_hub_configure_ports(inline)
-022|usb_hub_detect(dev = 0x8FE692A0)
-023|usb_host_detect(:host = 0x8FE558A0)
-024|usb_rescan()
-025|orbiter_usb_recovery(inline)
-025|orbiter_recovery(inline)
--> Fastboot is activated somewhere here.
-025|orbiter_recovery()
-026|start_barebox()
-027|barebox_non_pbl_start(membase = 2147483648, memsize = ?, boarddata = 0x9FC3A6BF)
-028|start(membase = ?, memsize = ?, boarddata = ?)
 ---|end of frame


Probably I shouldn't use fastboot this way (causing linux boot from polling
infrastructure). On the other hand, using fastboot allows taking control from
a PC without the need for manually entering commands in barebox (which is
quite useful in my application).

As a first step, I will try to check for NULL pointer in usb_stor_disconnect().
But I guess there should be a more generic solution (e.g. not calling
disconnect/remove for partially probed devices).

regards
Christian




_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [Barebox] Polling infrastructure causes reentrancy problem
  2021-11-05 12:40 [Barebox] Polling infrastructure causes reentrancy problem Christian Eggers
@ 2021-11-08 10:34 ` Sascha Hauer
  2021-11-10 13:50   ` Christian Eggers
  0 siblings, 1 reply; 3+ messages in thread
From: Sascha Hauer @ 2021-11-08 10:34 UTC (permalink / raw)
  To: Christian Eggers; +Cc: Ahmad Fatoum, barebox

Hi Christian,

On Fri, Nov 05, 2021 at 01:40:52PM +0100, Christian Eggers wrote:
> Disclaimer: I stumbled over this on barebox 2020.01.
> I haven't tried to reproduce this on the current version.

Please update to a recent barebox. The fastboot commands now run in a
workqueue which itself only runs when we are outside any critical pathes
in barebox.
Particularly fastboot commands will only execute when you called
command_slice_release() beforehand. You'll have to call
command_slice_acquire() before entering any critical pathes like probing
for USB devices.

Regards,
  Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [Barebox] Polling infrastructure causes reentrancy problem
  2021-11-08 10:34 ` Sascha Hauer
@ 2021-11-10 13:50   ` Christian Eggers
  0 siblings, 0 replies; 3+ messages in thread
From: Christian Eggers @ 2021-11-10 13:50 UTC (permalink / raw)
  To: Sascha Hauer; +Cc: Ahmad Fatoum, barebox

Hi Sascha,

On Monday, 8 November 2021, 11:34:26 CET, Sascha Hauer wrote:
> Hi Christian,
> 
> On Fri, Nov 05, 2021 at 01:40:52PM +0100, Christian Eggers wrote:
> > Disclaimer: I stumbled over this on barebox 2020.01.
> > I haven't tried to reproduce this on the current version.
> 
> Please update to a recent barebox. The fastboot commands now run in a
> workqueue which itself only runs when we are outside any critical pathes
> in barebox.
thanks for the hint. I just upgraded to 2021.10.

> Particularly fastboot commands will only execute when you called
> command_slice_release() beforehand. You'll have to call
> command_slice_acquire() before entering any critical pathes like probing
> for USB devices.
I guess that more or less all commands can be critical because
is_timeout() is called from many places. As parallelism for fastboot
is not mandatory, I implemented it the following way: 

	struct usbgadget_funcs const fastboot_funcs= {
		.flags = USBGADGET_FASTBOOT,
		.fastboot_opts = "/dev/null(null)",
	};

	/*
	 *  Register fastboot gadget (will run in background)
	 */
	ret = usbgadget_register(&fastboot_funcs);
	if (ret)
		goto error_usb_gadget;

	/* fastboot runs within a work queue. Allow fastboot execution within
	 * the next 5 seconds...
	 */
	command_slice_release();
	mdelay(5000);
	command_slice_acquire();


BTW: I use a modified/stripped down version of the Android fastboot for booting
Barebox FIT images. Is there any interest integrating this into barebox/scripts/?
- more or less stand alone (about 15 files at all)
- currently USB-only
- Apache-2.0 / BSD-2-clause
- C++
- CMake (only 60 lines, so it can be ported to Kbuild)

regards
Christian




_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2021-11-10 13:52 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-05 12:40 [Barebox] Polling infrastructure causes reentrancy problem Christian Eggers
2021-11-08 10:34 ` Sascha Hauer
2021-11-10 13:50   ` Christian Eggers

mail archive of the barebox mailing list

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://lore.barebox.org/barebox/0 barebox/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 barebox barebox/ https://lore.barebox.org/barebox \
		barebox@lists.infradead.org
	public-inbox-index barebox

Example config snippet for mirrors.


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git