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. > 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
