linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Petr Mladek <pmladek@suse.com>
To: Miroslav Benes <mbenes@suse.cz>
Cc: Jiri Kosina <jikos@kernel.org>,
	Josh Poimboeuf <jpoimboe@redhat.com>,
	Jason Baron <jbaron@akamai.com>,
	Joe Lawrence <joe.lawrence@redhat.com>,
	Evgenii Shatokhin <eshatokhin@virtuozzo.com>,
	live-patching@vger.kernel.org, linux-kernel@vger.kernel.org,
	Jessica Yu <jeyu@kernel.org>
Subject: Re: [PATCH v13 04/12] livepatch: Consolidate klp_free functions
Date: Wed, 21 Nov 2018 15:40:59 +0100	[thread overview]
Message-ID: <20181121144059.gv7sbzca55jvztgi@pathway.suse.cz> (raw)
In-Reply-To: <alpine.LSU.2.21.1811211450280.7578@pobox.suse.cz>

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

  reply	other threads:[~2018-11-21 14:41 UTC|newest]

Thread overview: 35+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-10-15 12:37 [PATCH v13 00/12] livepatch: Atomic replace feature Petr Mladek
2018-10-15 12:37 ` [PATCH v13 01/12] livepatch: Change void *new_func -> unsigned long new_addr in struct klp_func Petr Mladek
2018-10-15 12:37 ` [PATCH v13 02/12] livepatch: Helper macros to define livepatch structures Petr Mladek
2018-10-17 18:17   ` Josh Poimboeuf
2018-10-18 11:11     ` Petr Mladek
2018-10-18 12:58       ` Josh Poimboeuf
2018-10-24 11:28         ` Petr Mladek
2018-11-21 12:17           ` Miroslav Benes
2018-10-15 12:37 ` [PATCH v13 03/12] livepatch: Shuffle klp_enable_patch()/klp_disable_patch() code Petr Mladek
2018-10-15 12:37 ` [PATCH v13 04/12] livepatch: Consolidate klp_free functions Petr Mladek
2018-10-17 18:22   ` Josh Poimboeuf
2018-10-18 11:40     ` Petr Mladek
2018-11-21 13:59   ` Miroslav Benes
2018-11-21 14:40     ` Petr Mladek [this message]
2018-10-15 12:37 ` [PATCH v13 05/12] livepatch: Refuse to unload only livepatches available during a forced transition Petr Mladek
2018-10-17 18:35   ` Josh Poimboeuf
2018-10-18 12:09     ` Petr Mladek
2018-10-18 13:00       ` Josh Poimboeuf
2018-10-15 12:37 ` [PATCH v13 06/12] livepatch: Simplify API by removing registration step Petr Mladek
2018-10-17 19:06   ` Josh Poimboeuf
2018-10-18 12:33     ` Petr Mladek
2018-10-15 12:37 ` [PATCH v13 07/12] livepatch: Use lists to manage patches, objects and functions Petr Mladek
2018-10-17 20:31   ` Josh Poimboeuf
2018-10-18 12:34     ` Petr Mladek
2018-10-15 12:37 ` [PATCH v13 08/12] livepatch: Add atomic replace Petr Mladek
2018-11-23 12:00   ` Miroslav Benes
2018-10-15 12:37 ` [PATCH v13 09/12] livepatch: Remove Nop structures when unused Petr Mladek
2018-10-17 20:48   ` Josh Poimboeuf
2018-10-18 12:40     ` Petr Mladek
2018-10-15 12:37 ` [PATCH v13 10/12] livepatch: Atomic replace and cumulative patches documentation Petr Mladek
2018-10-15 12:37 ` [PATCH v13 11/12] livepatch: Remove ordering and refuse loading conflicting patches Petr Mladek
2018-10-15 12:37 ` [PATCH v13 12/12] selftests/livepatch: introduce tests Petr Mladek
2018-10-15 19:46   ` Joe Lawrence
2018-10-17 20:48   ` Josh Poimboeuf
2018-10-18 13:34 ` [PATCH v13 00/12] livepatch: Atomic replace feature Josh Poimboeuf

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20181121144059.gv7sbzca55jvztgi@pathway.suse.cz \
    --to=pmladek@suse.com \
    --cc=eshatokhin@virtuozzo.com \
    --cc=jbaron@akamai.com \
    --cc=jeyu@kernel.org \
    --cc=jikos@kernel.org \
    --cc=joe.lawrence@redhat.com \
    --cc=jpoimboe@redhat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=live-patching@vger.kernel.org \
    --cc=mbenes@suse.cz \
    --subject='Re: [PATCH v13 04/12] livepatch: Consolidate klp_free functions' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).