| Message ID | 20180911152052.15032-1-vaibhav@linux.ibm.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | opal: use for_each_safe to iterate over opal_syncers | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | master/apply_patch Successfully applied |
| snowpatch_ozlabs/make_check | success | Test make_check on branch master |
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@linux.vnet.ibm.com> -Vasant > > Signed-off-by: Vaibhav Jain <vaibhav@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) >
Thanks for reviewing this patch Vasant, Vasant Hegde <hegdevasant@linux.vnet.ibm.com> writes: > 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. Thats correct, but I am working on a patch for enabling fast reset for CAPI2 and need this function for some cleanup code. > Reviewed-by: Vasant Hegde <hegdevasant@linux.vnet.ibm.com> Thanks
Vaibhav Jain <vaibhav@linux.ibm.com> writes: > 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. > > Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com> > --- > core/opal.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) Fair enough, merged to master as of 5f728c53d42c664234b45a33218ba522a6e8f216
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)
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. Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com> --- core/opal.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)