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;
Subject: Re: [PATCH v8 0/8] livepatch: Atomic replace feature
To:     Petr Mladek <pmladek@suse.com>, Jason Baron <jbaron@akamai.com>
Cc:     Jiri Kosina <jikos@kernel.org>,
        Josh Poimboeuf <jpoimboe@redhat.com>,
        Miroslav Benes <mbenes@suse.cz>,
        Joe Lawrence <joe.lawrence@redhat.com>,
        Jessica Yu <jeyu@kernel.org>, live-patching@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20180221132914.4809-1-pmladek@suse.com>
From:   Evgenii Shatokhin <eshatokhin@virtuozzo.com>
Message-ID: <5c956014-b717-aed8-de1b-58967ef835c6@virtuozzo.com>
Date:   Mon, 5 Mar 2018 13:56:40 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.6.0
MIME-Version: 1.0
In-Reply-To: <20180221132914.4809-1-pmladek@suse.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Received-SPF: None (protection.outlook.com: virtuozzo.com does not designate
 permitted sender hosts)
SpamDiagnosticOutput: 1:99
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 05 Mar 2018 10:56:44.1324 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 1a1a5248-e8b5-4fe3-41f2-08d58287c916
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 0bc7f26d-0264-416e-a6fc-8352af79c58f
X-MS-Exchange-Transport-CrossTenantHeadersStamped: VI1PR0802MB2446
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi,

Petr, Jason - thanks a lot for working on this series, first of all! And 
especially for your patience.

On 21.02.2018 16:29, Petr Mladek wrote:
> The atomic replace allows to create cumulative patches. They
> are useful when you maintain many livepatches and want to remove
> one that is lower on the stack. In addition it is very useful when
> more patches touch the same function and there are dependencies
> between them.

I have experimented with this updated patchset a bit.
It looks like replace operation fails if there is a loaded but disabled 
patch.

Suppose there are 2 binary patches changing the same kernel functions. 
Load patch1, then disable it (echo 0 > 
/sys/kernel/livepatch/patch1/enabled), then try load patch2 with atomic 
replace. I get "Device or resource busy" error at this point.

It seems, __klp_enable_patch() returns -EBUSY for some reason. I haven't 
dug deeper into that code yet.

A workaround is simple: just unload all disabled patches before trying 
to load patch2 with replace. Still, the behavior is quite strange.

> 
> I have found one bug in v7. We were not able to initialize NOP
> struct klp_func when the patches module is not loaded. It was
> because func->new_func was NULL. I have fixed it in separate patch
> for an easier review.
> 
> Note that the original Jason's patch did not have this problem
> because func->new_func was always NULL there. But this required
> NOP-specific handling also on other locations, namely
> klp_ftrace_handler() and klp_check_stack_func().
> 
> Changes against v7:
> 
>    + Fixed handling of NOPs for not-yet-loaded modules
>    + Made klp_replaced_patches list static [Mirek]
>    + Made klp_free_object() public later [Mirek]
>    + Fixed several reported typos [Mirek, Joe]
>    + Updated documentation according to the feedback [Joe]
>    + Added some Acks [Mirek]
> 
> Changes against v6:
> 
>    + used list_move when disabling replaced patches [Jason]
>    + renamed KLP_FUNC_ORIGINAL -> KLP_FUNC_STATIC [Mirek]
>    + used klp_is_func_type() in klp_unpatch_object() [Mirek]
>    + moved static definition of klp_get_or_add_object() [Mirek]
>    + updated comment about synchronization in forced mode [Mirek]
>    + added user documentation
>    + fixed several typos
> 
> Jason Baron (5):
>    livepatch: Use lists to manage patches, objects and functions
>    livepatch: Initial support for dynamic structures
>    livepatch: Allow to unpatch only functions of the given type
>    livepatch: Support separate list for replaced patches.
>    livepatch: Add atomic replace
> 
> Petr Mladek (3):
>    livepatch: Free only structures with initialized kobject
>    livepatch: Correctly handle atomic replace for not yet loaded modules
>    livepatch: Atomic replace and cumulative patches documentation
> 
>   Documentation/livepatch/cumulative-patches.txt |  83 ++++++
>   include/linux/livepatch.h                      |  59 +++-
>   kernel/livepatch/core.c                        | 394 ++++++++++++++++++++++---
>   kernel/livepatch/core.h                        |   4 +
>   kernel/livepatch/patch.c                       |  31 +-
>   kernel/livepatch/patch.h                       |   4 +-
>   kernel/livepatch/transition.c                  |  41 ++-
>   7 files changed, 566 insertions(+), 50 deletions(-)
>   create mode 100644 Documentation/livepatch/cumulative-patches.txt
> 

Regards,
Evgenii