DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 5A5E78553C
Date: Tue, 12 Sep 2017 18:05:44 -0400
From: Joe Lawrence <joe.lawrence@redhat.com>
To: Miroslav Benes <mbenes@suse.cz>
Cc: live-patching@vger.kernel.org, linux-kernel@vger.kernel.org,
        Josh Poimboeuf <jpoimboe@redhat.com>, Jessica Yu <jeyu@kernel.org>,
        Jiri Kosina <jikos@kernel.org>, Petr Mladek <pmladek@suse.com>,
        Chris J Arges <chris.j.arges@canonical.com>
Subject: Re: [PATCH v5 1/3] livepatch: add (un)patch callbacks
Message-ID: <20170912220544.zdgh65o4aqmd5ni4@redhat.com>
References: <1504191233-2642-1-git-send-email-joe.lawrence@redhat.com>
 <1504191233-2642-2-git-send-email-joe.lawrence@redhat.com>
 <alpine.LSU.2.21.1709111628210.13925@san.suse.cz>
 <c50de669-e630-dff2-7bc4-d0a8997d204e@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <c50de669-e630-dff2-7bc4-d0a8997d204e@redhat.com>
User-Agent: Mutt/1.6.2-neo (2016-08-08)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2731
Lines: 78

On Tue, Sep 12, 2017 at 11:48:48AM -0400, Joe Lawrence wrote:
> On 09/12/2017 04:53 AM, Miroslav Benes wrote:
> >> @@ -871,13 +882,27 @@ int klp_module_coming(struct module *mod)
> >>  			pr_notice("applying patch '%s' to loading module '%s'\n",
> >>  				  patch->mod->name, obj->mod->name);
> >>  
> >> +			ret = klp_pre_patch_callback(obj);
> >> +			if (ret) {
> >> +				pr_warn("pre-patch callback failed for object '%s'\n",
> >> +					obj->name);
> >> +				goto err;
> >> +			}
> > 
> > There is a problem here. We cycle through all enabled patches (or 
> > klp_transition_patch) and call klp_pre_patch_callback() everytime an 
> > enabled patch contains a patch for a coming module. Now, it can easily 
> > happen that klp_pre_patch_callback() fails. And not the first one from the 
> > first relevant patch, but the next one. In that case we need to call 
> > klp_post_unpatch_callback() for all already processed relevant patches in 
> > the error path.
> 
> Good test case, if I understand you correctly:
> 
>  - Load target modules mod1 and mod2
>  - Load a livepatch that targets mod1 and mod2
>    - pre-patch succeeds for mod1
>    - pre-patch fails for mod2
> 
> and then we should:
> 
>  - NOT run post-patch or pre/post-unpatch handlers for mod2
>  - NOT run post-patch or pre-unpatch handlers for mod1
>  - do run post-unpatch handler for mod1
>  - Refuse to load the livepatch
> 
> Does that sound right?

Erm, probably not...

> > Unfortunately, we need to do the same for klp_patch_object() below, 
> > because there is the same problem and we missed it.
> > 
> >> +
> >>  			ret = klp_patch_object(obj);
> >>  			if (ret) {
> >>  				pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",
> >>  					patch->mod->name, obj->mod->name, ret);
> >> +
> >> +				if (patch != klp_transition_patch)
> >> +					klp_post_unpatch_callback(obj);
> >> +
> >>  				goto err;
> > 
> > Here.
> > 
> > Could you do it as a part of the patch set (or send it separately), 
> > please?

I've re-read this a few times, and I think I might have been originally
off-base with what I thought you were concerned about.  But I think I
grok it now: the problem you pointed out arises because
klp_module_coming() iterates like so:

  for each klp_patch
    for each kobj in klp_patch

which means that we may have made pre-patch callbacks and patched a
given kobj for an earlier klp_patch that now fails for a later
klp_patch.

What should be the defined behavior in this case?  I would expect that
we need to unpatch all similar kobjs across klp_patches which have
already been successfully patched.  In turn, their post-unpatch
callbacks should be invoked.

If that's true, maybe this would make a better follow-on patch.

-- Joe