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
