Hi Rafael, what's your opinion on this race condition in ACPI hotplug?
On Fri, Feb 28, 2025 at 11:16:01AM -0600, Bjorn Helgaas wrote:
> On Mon, Feb 24, 2025 at 03:00:34PM +0800, Weiwen Hu wrote:
> > Ignore the eject notification when the previous ejection is still in
> > progress to prevent multiple _EJ0 invocations when the ejection completes.
> >
> > The first _EJ0 informs the platform to actually eject the device and frees
> > the slot for other devices. So the subsequent _EJ0 may accidentally eject
> > the new device that is just plugged into this slot. We need to avoid this.
> >
> > For each acpi_device, this patch introduces a new field `ejecting`, which
> > is set before enqueuing the `kacpi_hotplug_wq` and reset before invoking
> > _EJ0. Every notifications we received before invoking _EJ0 is targeted to
> > the old device. This ensures we don't miss any notifications for newly
> > plugged device. And a new flag `should_dedup_eject` allows each driver to
> > implement this feature gradually. A driver should set this flag on
> > initialization if it will reset `ejecting`.
>
> Which drivers do you have in mind when you say "each driver can
> implement this feature gradually"? You set should_dedup_eject in
> acpiphp, so I guess you mean other ACPI hotplug drivers? I see
> acpi_hotplug_context mentioned in libata-acpi.c, surface3-wmi.c; maybe
> those are the only other current ones?
Yes, I mean other ACPI hotplug drivers. I've searched for acpi_evaluate_ej0(),
because `ejecting` should be reset just before this. I got dock.c and scan.c.
All of them are called from acpi_device_hotplug(). So may be I can change all
of them at once. Should we also consider out-of-tree drivers?
As for surface3-wmi.c, its s3_wmi_hp_notify function seems only probing lid
state, and does not support hot unplug actually.
But ata_acpi_detach_device() seems not calling acpi_evaluate_ej0(), From
f730ae183863 ("libata: remove functions now handed by ACPI dock driver"), It
seems that ata relies on dock to call ej0 for it.
> > This fix is not perfect. If we receive an eject notification just after
> > resetting `ejecting` but before _EJ0, we will still invoke _EJ0 twice.
> > However this seems to be the best solution available, and it strictly
> > improves the current situation.
> >
> > Another potential fix is to add an `ejected` flag to each device and not
> > invoke _EJ0 for already ejected devices. However, this approach risks
> > losing synchronization with the platform if something else goes wrong,
> > potentially preventing the device from being ejected permanently. And we
> > need to check with bus driver to make sure the device is really ejected
> > successfully. But this check is still racy. So we cannot ensure no extra
> > _EJ0 invocations either.
>
> I see the problem. Thanks for the detailed explanation and details
> about reproducing it and the trace.
>
> I'm not sure whether the platform should reissue the Bus Check
> notification based on the fact that the OS hasn't invoked _EJ0 in some
> arbitrary time. That seems a little bit presumptuous because, for
> example, the platform can't know how long it will take to write out
> the dirty page cache. The racyness of the workaround seems
> troublesome to me.
>
> But this is all really an ACPI issue, not a PCI issue, so I'd like to
> defer to the ACPI experts here.
>
> > Signed-off-by: Weiwen Hu <huweiwen@xxxxxxxxxxxxxxxxx>
> > ---
> > drivers/acpi/osl.c | 6 ++++++
> > drivers/pci/hotplug/acpiphp_glue.c | 15 +++++++++++----
> > include/acpi/acpi_bus.h | 4 +++-
> > 3 files changed, 20 insertions(+), 5 deletions(-)
> >
> > We observed that umount can take extremely long time if there is a lot of
> > dirty page cache. umount will take the s_umount semaphore, which will
> > block the ejecting process:
> >
> > __schedule+0x1e0/0x630
> > ? kernfs_put.part.0+0xd4/0x1a0
> > schedule+0x46/0xb0
> > rwsem_down_read_slowpath+0x16b/0x490
> > __get_super.part.0+0xc1/0xe0
> > fsync_bdev+0x11/0x60
> > invalidate_partition+0x5c/0xa0
> > del_gendisk+0x103/0x2e0
> > virtblk_remove+0x27/0xa0
> > virtio_dev_remove+0x36/0x90
> > __device_release_driver+0x172/0x260
> > device_release_driver+0x24/0x30
> > bus_remove_device+0xf6/0x170
> > device_del+0x19b/0x450
> > device_unregister+0x16/0x60
> > unregister_virtio_device+0x11/0x20
> > virtio_pci_remove+0x31/0x60
> > pci_device_remove+0x38/0xa0
> > __device_release_driver+0x172/0x260
> > device_release_driver+0x24/0x30
> > pci_stop_bus_device+0x6c/0x90
> > pci_stop_and_remove_bus_device+0xe/0x20
> > disable_slot+0x49/0x90
> > acpiphp_disable_and_eject_slot+0x15/0x90
> >
> > While OS is not invoking _EJ0 timely, the user (or hypervisor) may retry by
> > issuing another notification, which will be queued in kacpi_hotplug_wq.
> > After the umount is finally done, the _EJ0 will be invoked. Then, if there
> > are devices pending attach, the hypervisor may choose to attach it
> > immediately to the same slot. The new device can be ejected by the queued
> > ejecting process unintentionally.
> >
> > On Alibaba Cloud, we re-issue the notification around every 10s if the OS
> > does not respond. (BTW, do you think platform is allowed to re-issue
> > the notification on timeout?)
> > We can easily reproduce this issue on Alibaba Cloud ECS:
> >
> > WRITE_SIZE=2300M # tune this value so that the umount takes 20s
> > # replace these disk serial numbers
> > DISK_DETACH=bp142xxxxxxxxxxxxxxx # pre-formatted
> > DISK_ATTACH=bp109xxxxxxxxxxxxxxx # any
> > # start from these two disks detached
> >
> > INSTANCE_ID=$(curl -sS http://100.100.100.200/latest/meta-data/instance-id)
> > echo "instance id: $INSTANCE_ID"
> > DISK_PATH=/dev/disk/by-id/nvme-Alibaba_Cloud_Elastic_Block_Storage_
> >
> > echo "attaching disk d-$DISK_DETACH"
> > aliyun ecs AttachDisk --DiskId "d-$DISK_DETACH" --InstanceId "$INSTANCE_ID"
> >
> > sleep 2
> > mkdir -p /mnt/slow
> > mount "$DISK_PATH$DISK_DETACH" /mnt/slow
> > echo "mounted d-$DISK_DETACH to /mnt/slow"
> >
> > rm -f /mnt/slow/zero
> > echo "populating dirty cache"
> > head -c $WRITE_SIZE /dev/zero > /mnt/slow/zero;
> >
> > echo umounting
> > (
> > umount /mnt/slow
> > echo umounted
> > )&
> >
> > sleep 2
> > echo "detaching disk d-$DISK_DETACH"
> > aliyun ecs DetachDisk --DiskId "d-$DISK_DETACH" --InstanceId "$INSTANCE_ID"
> >
> > sleep 10
> > echo "attaching disk d-$DISK_ATTACH"
> > aliyun ecs AttachDisk --DiskId "d-$DISK_ATTACH" --InstanceId "$INSTANCE_ID"
> >
> > sleep 7
> > wait
> > for _ in {1..10}; do
> > sleep 1
> > if [ -e "$DISK_PATH$DISK_ATTACH" ]; then
> > echo "disk d-$DISK_ATTACH attached, issue not reproduced"
> > exit 0
> > fi
> > echo "disk d-$DISK_ATTACH not found yet"
> > done
> >
> > echo "issue reproduced"
> > exit 1
> >
> > And here is the trace we got from `perf trace` while running the above script on an unpatched kernel:
> >
> > [starting detach]
> > 48202.244 kworker/0:0-ev/5 probe:acpi_ev_queue_notify_request(__probe_ip: -1452149680, notify_value: 3)
> > 48202.314 kworker/0:0-ev/5 probe:acpi_hotplug_schedule(__probe_ip: -1452297040, src: 3)
> > 48203.690 kworker/u8:0-e/1946 probe:acpi_device_hotplug(__probe_ip: -1452251424, src: 3)
> > [blocked, retrying detach]
> > 58023.813 kworker/0:0-ev/5 probe:acpi_ev_queue_notify_request(__probe_ip: -1452149680, notify_value: 3)
> > 58023.881 kworker/0:0-ev/5 probe:acpi_hotplug_schedule(__probe_ip: -1452297040, src: 3)
> > [detach done]
> > 62834.048 kworker/u8:0-e/1946 probe:acpi_evaluate_ej0(__probe_ip: -1452291632)
> > [another device attaching]
> > 62954.686 kworker/0:0-ev/5 probe:acpi_ev_queue_notify_request(__probe_ip: -1452149680, notify_value: 1)
> > 62954.828 kworker/0:0-ev/5 probe:acpi_hotplug_schedule(__probe_ip: -1452297040, src: 1)
> > 63042.506 kworker/u8:0-e/1946 probe:acpi_device_hotplug(__probe_ip: -1452251424, src: 3)
> > [the new device is ejected unintentionally]
> > 63042.520 kworker/u8:0-e/1946 probe:acpi_evaluate_ej0(__probe_ip: -1452291632)
> > [the actual attach task, scanned the bus but got nothing]
> > 63266.555 kworker/u8:0-e/1946 probe:acpi_device_hotplug(__probe_ip: -1452251424, src: 1)
> >
> > With this patch, the acpi_hotplug_schedule at 58023.881 will be skipped to
> > suppress the acpi_evaluate_ej0 at 63042.520.
> >
> > diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
> > index 5ff343096ece..f041c4db10f7 100644
> > --- a/drivers/acpi/osl.c
> > +++ b/drivers/acpi/osl.c
> > @@ -1193,6 +1193,12 @@ acpi_status acpi_hotplug_schedule(struct acpi_device *adev, u32 src)
> > {
> > struct acpi_hp_work *hpw;
> >
> > + if (src == ACPI_NOTIFY_EJECT_REQUEST && adev->flags.should_dedup_eject
> > + && atomic_xchg(&adev->hp->ejecting, 1)) {
> > + put_device(&adev->dev);
> > + return AE_OK;
> > + }
> > +
> > acpi_handle_debug(adev->handle,
> > "Scheduling hotplug event %u for deferred handling\n",
> > src);
> > diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
> > index 5b1f271c6034..3c50f2af1584 100644
> > --- a/drivers/pci/hotplug/acpiphp_glue.c
> > +++ b/drivers/pci/hotplug/acpiphp_glue.c
> > @@ -68,6 +68,7 @@ static struct acpiphp_context *acpiphp_init_context(struct acpi_device *adev)
> > context->hp.notify = acpiphp_hotplug_notify;
> > context->hp.fixup = acpiphp_post_dock_fixup;
> > acpi_set_hp_context(adev, &context->hp);
> > + adev->flags.should_dedup_eject = true;
> > return context;
> > }
> >
> > @@ -778,7 +779,8 @@ void acpiphp_check_host_bridge(struct acpi_device *adev)
> > }
> > }
> >
> > -static int acpiphp_disable_and_eject_slot(struct acpiphp_slot *slot);
> > +static int
> > +acpiphp_disable_and_eject_slot(struct acpi_hotplug_context *hp, struct acpiphp_slot *slot);
> >
> > static void hotplug_event(u32 type, struct acpiphp_context *context)
> > {
> > @@ -825,7 +827,7 @@ static void hotplug_event(u32 type, struct acpiphp_context *context)
> > case ACPI_NOTIFY_EJECT_REQUEST:
> > /* request device eject */
> > acpi_handle_debug(handle, "Eject request in %s()\n", __func__);
> > - acpiphp_disable_and_eject_slot(slot);
> > + acpiphp_disable_and_eject_slot(&context->hp, slot);
> > break;
> > }
> >
> > @@ -999,9 +1001,11 @@ int acpiphp_enable_slot(struct acpiphp_slot *slot)
> >
> > /**
> > * acpiphp_disable_and_eject_slot - power off and eject slot
> > + * @hp: the context that received eject notification, can be NULL
> > * @slot: ACPI PHP slot
> > */
> > -static int acpiphp_disable_and_eject_slot(struct acpiphp_slot *slot)
> > +static int
> > +acpiphp_disable_and_eject_slot(struct acpi_hotplug_context *hp, struct acpiphp_slot *slot)
> > {
> > struct acpiphp_func *func;
> >
> > @@ -1011,6 +1015,9 @@ static int acpiphp_disable_and_eject_slot(struct acpiphp_slot *slot)
> > /* unconfigure all functions */
> > disable_slot(slot);
> >
> > + if (hp)
> > + atomic_set(&hp->ejecting, 0);
> > +
> > list_for_each_entry(func, &slot->funcs, sibling)
> > if (func->flags & FUNC_HAS_EJ0) {
> > acpi_handle handle = func_to_handle(func);
> > @@ -1034,7 +1041,7 @@ int acpiphp_disable_slot(struct acpiphp_slot *slot)
> > */
> > acpi_scan_lock_acquire();
> > pci_lock_rescan_remove();
> > - ret = acpiphp_disable_and_eject_slot(slot);
> > + ret = acpiphp_disable_and_eject_slot(NULL, slot);
> > pci_unlock_rescan_remove();
> > acpi_scan_lock_release();
> > return ret;
> > diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> > index aad1a95e6863..870c1ffe47c9 100644
> > --- a/include/acpi/acpi_bus.h
> > +++ b/include/acpi/acpi_bus.h
> > @@ -151,6 +151,7 @@ typedef void (*acpi_hp_fixup) (struct acpi_device *);
> >
> > struct acpi_hotplug_context {
> > struct acpi_device *self;
> > + atomic_t ejecting;
> > acpi_hp_notify notify;
> > acpi_hp_uevent uevent;
> > acpi_hp_fixup fixup;
> > @@ -215,7 +216,8 @@ struct acpi_device_flags {
> > u32 cca_seen:1;
> > u32 enumeration_by_parent:1;
> > u32 honor_deps:1;
> > - u32 reserved:18;
> > + u32 should_dedup_eject:1;
> > + u32 reserved:17;
> > };
> >
> > /* File System */
> > --
> > 2.48.1
> >
