[Skiboot] [PATCH] opal: use for_each_safe to iterate over opal_syncers

[Vasant Hegde]

On 09/11/2018 08:50 PM, Vaibhav Jain wrote:
> Presently a fault will happen in opal_sync_host_reboot if a callback
> tries to remove itself from the opal_syncers list by calling
> opal_del_host_sync_notifier.
> 
> This happens as iteration over opal_syncers is done using the
> list_for_each() which doesn't preserve list_node->next. So when
> the current opal_syncers callback removes itself from the list, current
> node contents are lost and current_node->next pointer is rendered
> invalid.
> 
> To fix this we simply switch from list_for_each() to
> list_for_each_safe() which keeps the current_node->next cached hence
> even if the current node is freed, iteration over subsequent nodes can
> still continue.

Patch itself looks good to me.

No one is using opal_del_host_sync_notifier() today. Also I don't see why someone
will call this via callback path. May be we should revisit and see whether we really
need to keep this unused function or not.


Reviewed-by: Vasant Hegde <hegdevasant at linux.vnet.ibm.com>

-Vasant




> 
> Signed-off-by: Vaibhav Jain <vaibhav at linux.ibm.com>
> ---
>   core/opal.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/core/opal.c b/core/opal.c
> index 7ffca9c1..63a08510 100644
> --- a/core/opal.c
> +++ b/core/opal.c
> @@ -692,10 +692,10 @@ void opal_del_host_sync_notifier(bool (*notify)(void *data))
>    */
>   static int64_t opal_sync_host_reboot(void)
>   {
> -	struct opal_sync_entry *ent;
> +	struct opal_sync_entry *ent, *nxt;
>   	bool ret = true;
> 
> -	list_for_each(&opal_syncers, ent, link)
> +	list_for_each_safe(&opal_syncers, ent, nxt, link)
>   		ret &= ent->notify(ent->data);
> 
>   	if (ret)
>