Messages in this thread | | | Date | Mon, 17 Dec 2018 16:27:41 +0100 | From | Petr Mladek <> | Subject | Re: [PATCH v14 07/11] livepatch: Add atomic replace |
| |
On Thu 2018-12-13 16:55:28, Josh Poimboeuf wrote: > On Thu, Nov 29, 2018 at 10:44:27AM +0100, Petr Mladek wrote: > > @@ -415,6 +449,124 @@ static struct attribute *klp_patch_attrs[] = { > > NULL > > }; > > > > +/* > > + * Dynamically allocated objects and functions. > > + */ > > I don't think this comment is needed. > > > +static void klp_free_object_dynamic(struct klp_object *obj) > > +{ > > + kfree(obj->name); > > + kfree(obj); > > +} > > @@ -456,6 +620,8 @@ static void klp_free_funcs(struct klp_object *obj) > > if (func->kobj_alive) { > > func->kobj_alive = false; > > kobject_put(&func->kobj); > > + } else if (func->nop) { > > + klp_free_func_nop(func); > > This removes 'func' from the list, so it needs to do a 'safe' list > iteration.
Good catch!
Well, note that the 'safe' list iterators were added by the very next patch anyway. This is why I never triggered a problem with this.
> > +void klp_discard_replaced_patches(struct klp_patch *new_patch) > > +{ > > + struct klp_patch *old_patch, *tmp_patch; > > + > > + list_for_each_entry_safe(old_patch, tmp_patch, &klp_patches, list) { > > + if (old_patch == new_patch) > > + return; > > + > > + old_patch->enabled = false; > > + klp_unpatch_objects(old_patch); > > + klp_free_patch_start(old_patch); > > + schedule_work(&old_patch->free_work); > > + } > > This doesn't need the "safe" list iteration because it doesn't remove > the patch from the list.
It does need the 'safe' list. klp_free_patch_start() removes the patch from the list.
> Side note, it would probably be useful to have a klp_for_each_patch() > helper.
Will do.
Best Regards, Petr
| |