mail archive of the barebox mailing list
 help / color / mirror / Atom feed
From: Christian Eggers <ceggers@arri.de>
To: Ahmad Fatoum <a.fatoum@pengutronix.de>,
	Sascha Hauer <s.hauer@pengutronix.de>,
	Marc Kleine-Budde <mkl@pengutronix.de>
Cc: <barebox@lists.infradead.org>
Subject: [Barebox] Polling infrastructure causes reentrancy problem
Date: Fri, 5 Nov 2021 13:40:52 +0100	[thread overview]
Message-ID: <3170930.44csPzL39Z@n95hx1g2> (raw)

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


             reply	other threads:[~2021-11-05 12:42 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-11-05 12:40 Christian Eggers [this message]
2021-11-08 10:34 ` Sascha Hauer
2021-11-10 13:50   ` Christian Eggers

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=3170930.44csPzL39Z@n95hx1g2 \
    --to=ceggers@arri.de \
    --cc=a.fatoum@pengutronix.de \
    --cc=barebox@lists.infradead.org \
    --cc=mkl@pengutronix.de \
    --cc=s.hauer@pengutronix.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox