From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753785AbdGUR5j (ORCPT ); Fri, 21 Jul 2017 13:57:39 -0400 Received: from mx0a-00190b01.pphosted.com ([67.231.149.131]:32818 "EHLO mx0b-00190b01.pphosted.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1750849AbdGUR5h (ORCPT ); Fri, 21 Jul 2017 13:57:37 -0400 Subject: Re: [PATCH 0/3] livepatch: introduce atomic replace To: Miroslav Benes References: Cc: linux-kernel@vger.kernel.org, live-patching@vger.kernel.org, jpoimboe@redhat.com, jeyu@kernel.org, jikos@kernel.org, pmladek@suse.com From: Jason Baron Message-ID: <2476c7a9-e962-8c20-2cf1-41befdd8ef95@akamai.com> Date: Fri, 21 Jul 2017 13:56:40 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-07-21_10:,, signatures=0 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 suspectscore=0 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1706020000 definitions=main-1707210278 X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-07-21_10:,, signatures=0 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 priorityscore=1501 malwarescore=0 suspectscore=0 phishscore=0 bulkscore=0 spamscore=0 clxscore=1015 lowpriorityscore=0 impostorscore=0 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1706020000 definitions=main-1707210277 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 07/21/2017 09:06 AM, Miroslav Benes wrote: > On Wed, 19 Jul 2017, Jason Baron wrote: > >> Hi, >> >> In testing livepatch, I found that when doing cumulative patches, if a patched >> function is completed reverted by a subsequent patch (back to its original state) >> livepatch does not revert the funtion to its original state. Specifically, if >> patch A introduces a change to function 1, and patch B reverts the change to >> function 1 and introduces changes to say function 2 and 3 as well, the change >> that patch A introducd to function 1 is still present. This could be addressed >> by first completely removing patch A (disable and then rmmod) and then inserting >> patch B (insmod and enable), but this leaves an unpatched window. In discussing >> this issue with Josh on the kpatch mailing list, he mentioned that we could get >> 'atomic replace working properly', and that is the direction of this patchset: >> https://www.redhat.com/archives/kpatch/2017-June/msg00005.html > > Hi Jason, > > this has been on my TODO list for a long time now, so thanks for working > on this. We have the same feature in kGraft and we use it heavily (in fact > we distribute our patches as cumulative and "replace_all" how we call it). > Hi Miroslav, Cool - we feel like this is an important feature as well and would like to have an upstream solution as well. > The forward port of the feature from kGraft is unfortunately not > straightforward. We do not have a concept of klp_target_state there, so we > can freely let functions to be patched or reverted in one go. We cannot do > the same upstream. At first glance, you used nop function exactly for this > case. Nice hack :). > >> Patches: >> >> 1) livepatch: Add klp_object and klp_func iterators >> Just a prep patch for the 'atomic revert' feature >> >> 2) livepatch: add atomic replace >> Core feature >> >> 3) livepatch: Add a sysctl livepatch_mode for atomic replace >> Introduces a knob for enabling atomic replace. I hate knobs and perhaps >> its possible to default to cumulative replace? Although I suspect there >> are workflows relying on the existing behavior - I'm not sure. It may >> be desirable to associate the knob with the patch itself as in the >> 'immediate' flag, such that we don't introduce a global sysctl that >> likely would also need to built-in, if there are patches in the initrd. > > Yes. I think it should be associated with the patch itself. This would > allow more flexible behaviour. You could stack more patches on top of > "atomic replace" patch. > Ok - associating the "atomic replace" with the patch itself makes sense to me. It would also basically work, I think with the patch I proposed except for the case where the the "atomic replace" was on top of several non-"atomic replace" patches. The reason is that the "atomic replace" I posted looks back 1 patch to see what it needs to replace (assuming all patches are in atomic replace mode). So instead of just looking back 1 patch, it could 'look back' and make sure it was replacing all previously loaded patches. > Anyway, I'm on holiday next week, so I'll take a proper look the week > after. > Ok - have a nice holiday! Thanks, -Jason > Thanks, > Miroslav >