James Morse <james.morse@xxxxxxx> writes: > Hi Huang, Ying > > On 17/03/17 01:23, Huang, Ying wrote: >> James Morse <james.morse@xxxxxxx> writes: >> >>> When removing a GHES device notified by SCI, list_del_rcu() is used, >>> ghes_remove() should call synchronize_rcu() before it goes on to call >>> kfree(ghes), otherwise concurrent RCU readers may still hold this list >>> entry after it has been freed. > >>> --- >>> It looks like 81e88fdc432a lifted this into ACPI_HEST_NOTIFY_NMI, missing >>> that ACPI_HEST_NOTIFY_SCI needed it too. >>> >>> If there is only ever one SCI GHES entry this is safe today as >>> unregister_acpi_hed_notifier() takes a write lock on its semaphore, meaning >>> any RCU readers will have finished. > > >> In remove path >> >> unregister_acpi_hed_notifier() >> blocking_notifier_chain_unregister() >> down_write(&nh->rwsem) >> >> While in notifier call path >> >> acpi_hed_notify() >> blocking_notifier_call_chain() >> __blocking_notifier_call_chain() >> down_read(&nh->rwsem) >> >> So when unregister succeeds, the notifier call should have >> finished. > > You are only protected like this if the unregister call is made for every > list_del_rcu(), which would only be the case if there is only ever one SCI GHES > entry. > > Is this how NOTIFY_SCI is expected to be used? > > If so I agree its safe, (but confusing!) today, and we need to take account of > this behaviour in Shiju Jose's patch. > > > If there can be multiple SCI entries, you only get this unregister protection > for the last one to be freed, as the list_empty() check skips the unregister for > all but the last entry. Yes. You are right. Feel free to add Reviewed-by: "Huang, Ying" <ying.huang@xxxxxxxxx> In your patch. Best Regards, Huang, Ying >> case ACPI_HEST_NOTIFY_SCI: >> mutex_lock(&ghes_list_mutex); >> list_del_rcu(&ghes->list); >> if (list_empty(&ghes_sci)) >> unregister_acpi_hed_notifier(&ghes_notifier_sci); >> mutex_unlock(&ghes_list_mutex); >> break; > > > > Thanks, > > James -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
