Re: [PATCH v13 04/12] livepatch: Consolidate klp_free functions

[Petr Mladek]

On Wed 2018-11-21 14:59:29, Miroslav Benes wrote:
> > -/*
> > - * Free all functions' kobjects in the array up to some limit. When limit is
> > - * NULL, all kobjects are freed.
> > - */
> > -static void klp_free_funcs_limited(struct klp_object *obj,
> > -				   struct klp_func *limit)
> > +static void klp_free_funcs(struct klp_object *obj)
> >  {
> >  	struct klp_func *func;
> >  
> > -	for (func = obj->funcs; func->old_name && func != limit; func++)
> > -		kobject_put(&func->kobj);
> > +	klp_for_each_func(obj, func) {
> > +		/* Might be called from klp_init_patch() error path. */
> > +		if (func->kobj.state_initialized)
> > +			kobject_put(&func->kobj);
> > +	}
> >  }
> 
> I have not noticed till today, but state_initialized is probably not the 
> best idea. kobject_init_and_add() sets it to 1 in kobject_init() part but 
> then _add() is called which could result in error. So we would end up with 
> state_initialized equal to 1 and kobject reference equal to 0. Later call 
> to kobject_put() in klp_free_funcs() (or elsewhere) would not call 
> ->release method, because refcount would be 0 by then.
> 
> I think that all would end up well, but that does not mean we should not 
> fix it.
> 
> We could use state_in_sysfs, but I do not think it guarantees anything. 
> Both are internal states and maybe we should not rely on them.
> 
> So kref_read() and check the reference?

I have discussed it a bit more with Miroslav in person. It seems that
the best solution is to add our own .initialized flag into
the affected structures.

The thing is that kobject should be used for dynamically allocated
structures. We use them for static structures just to get the sysfs
interface. Let's still use the sysfs functionality but let's
handle the freeing on our own (reduce hijacking kobject
free machinery).

Best Regards,
Petr