Re: [PATCH] ACPI / APEI: Add missing synchronize_rcu() on NOTIFY_SCI removal.

["Huang\, Ying" <ying.huang@intel.com>]

James Morse <james.morse@arm.com> writes:

> Hi Huang, Ying
>
> On 17/03/17 01:23, Huang, Ying wrote:
>> James Morse <james.morse@arm.com> 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@intel.com>

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