Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 4 Dec 2018 15:00:07 +0100
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 v14 03/11] livepatch: Consolidate klp_free functions
Message-ID: <20181204140007.si7l2glbsgaj2dbf@pathway.suse.cz>
References: <20181129094431.7801-1-pmladek@suse.com>
 <20181129094431.7801-4-pmladek@suse.com>
 <alpine.LSU.2.21.1812031542540.22317@pobox.suse.cz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <alpine.LSU.2.21.1812031542540.22317@pobox.suse.cz>
User-Agent: NeoMutt/20170421 (1.8.2)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon 2018-12-03 15:59:57, Miroslav Benes wrote:
> On Thu, 29 Nov 2018, Petr Mladek wrote:
> 
> > -static int klp_init_patch(struct klp_patch *patch)
> > +/* Init operations that must succeed before klp_free_patch() can be called. */
> > +static int klp_init_patch_before_free(struct klp_patch *patch)
> 
> There is no klp_free_patch() now, so the comment is not correct. Also I 
> don't know if the function name is ideal, but I don't have a better one.
> 
> >  {
> >  	struct klp_object *obj;
> > -	int ret;
> > +	struct klp_func *func;
> >  
> >  	if (!patch->objs)
> >  		return -EINVAL;
> >  
> > -	mutex_lock(&klp_mutex);
> > -
> > +	INIT_LIST_HEAD(&patch->list);
> > +	patch->kobj_alive = false;
> >  	patch->enabled = false;
> >  	init_completion(&patch->finish);
> >  
> > -	ret = kobject_init_and_add(&patch->kobj, &klp_ktype_patch,
> > -				   klp_root_kobj, "%s", patch->mod->name);
> > +	klp_for_each_object(patch, obj) {
> > +		if (!obj->funcs)
> > +			return -EINVAL;
> > +
> > +		obj->kobj_alive = false;
> > +
> > +		klp_for_each_func(obj, func)
> > +			func->kobj_alive = false;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int klp_init_patch(struct klp_patch *patch)
> > +{
> > +	struct klp_object *obj;
> > +	int ret;
> > +
> > +	mutex_lock(&klp_mutex);
> > +
> > +	ret = klp_init_patch_before_free(patch);
> >  	if (ret) {
> >  		mutex_unlock(&klp_mutex);
> >  		return ret;
> >  	}
> >  
> > +	ret = kobject_init_and_add(&patch->kobj, &klp_ktype_patch,
> > +				   klp_root_kobj, "%s", patch->mod->name);
> > +	if (ret)
> > +		goto free;
> 
> Is this intentional? It should be sufficient (and it was before the patch) to
> unlock the mutex and return ret. We do not need to free anything here. Only
> klp_patch instance was initialized and that is all.

It will get needed later when try_module_get() is moved into
klp_init_patch_before_free(), see the 5th patch. I'll do it
in the right order in v15.

BTW: I remember that I did this by intention. I updated the patchset
step by step according to the feedback. The init_before_free() was
growing and growing. I did many rebases, shuffling changes between
patches, just wanted to be on the safe side, and maybe save one hunk.
But it seems that no shortcut can slip the careful review ;-)


> Otherwise it looks good. kobj_alive feels safer than state_initialized.

Yup.

Best Regards,
Petr