From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755987AbdLOSEW (ORCPT ); Fri, 15 Dec 2017 13:04:22 -0500 Received: from mx0b-00190b01.pphosted.com ([67.231.157.127]:35954 "EHLO mx0b-00190b01.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755409AbdLOSES (ORCPT ); Fri, 15 Dec 2017 13:04:18 -0500 Subject: Re: [PATCH v4.1 2/2] livepatch: force transition to finish To: Miroslav Benes , jpoimboe@redhat.com, jeyu@kernel.org, jikos@kernel.org Cc: pmladek@suse.com, lpechacek@suse.cz, pavel@ucw.cz, live-patching@vger.kernel.org, linux-kernel@vger.kernel.org References: <20171122102921.12289-1-mbenes@suse.cz> From: Jason Baron Message-ID: <70454f58-3d33-f305-e1df-5f9dc43222c7@akamai.com> Date: Fri, 15 Dec 2017 13:04:05 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: <20171122102921.12289-1-mbenes@suse.cz> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-12-15_10:,, signatures=0 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=0 malwarescore=0 phishscore=0 bulkscore=0 spamscore=0 mlxscore=0 mlxlogscore=715 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1711220000 definitions=main-1712150251 X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-12-15_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=1011 lowpriorityscore=0 mlxscore=0 impostorscore=0 mlxlogscore=685 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1711220000 definitions=main-1712150251 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 11/22/2017 05:29 AM, Miroslav Benes wrote: > If a task sleeps in a set of patched functions uninterruptedly, it could > block the whole transition indefinitely. Thus it may be useful to clear > its TIF_PATCH_PENDING to allow the process to finish. > > Admin can do that now by writing to force sysfs attribute in livepatch > sysfs directory. TIF_PATCH_PENDING is then cleared for all tasks and the > transition can finish successfully. > > Important note! Administrator should not use this feature without a > clearance from a patch distributor. It must be checked that by doing so > the consistency model guarantees are not violated. Removal (rmmod) of > patch modules is permanently disabled when the feature is used. It > cannot be guaranteed there is no task sleeping in such module. > > Signed-off-by: Miroslav Benes > Acked-by: Josh Poimboeuf > Reviewed-by: Petr Mladek > --- > Documentation/ABI/testing/sysfs-kernel-livepatch | 14 ++++++++++ > Documentation/livepatch/livepatch.txt | 18 ++++++++++-- > kernel/livepatch/core.c | 30 ++++++++++++++++++++ > kernel/livepatch/transition.c | 35 +++++++++++++++++++++++- > kernel/livepatch/transition.h | 1 + > 5 files changed, 95 insertions(+), 3 deletions(-) .... > + > +/* > + * Drop TIF_PATCH_PENDING of all tasks on admin's request. This forces an > + * existing transition to finish. > + * > + * NOTE: klp_update_patch_state(task) requires the task to be inactive or > + * 'current'. This is not the case here and the consistency model could be > + * broken. Administrator, who is the only one to execute the > + * klp_force_transitions(), has to be aware of this. > + */ > +void klp_force_transition(void) > +{ > + struct task_struct *g, *task; > + unsigned int cpu; > + > + pr_warn("forcing remaining tasks to the patched state\n"); > + > + read_lock(&tasklist_lock); > + for_each_process_thread(g, task) > + klp_update_patch_state(task); > + read_unlock(&tasklist_lock); > + > + for_each_possible_cpu(cpu) > + klp_update_patch_state(idle_task(cpu)); > + > + klp_forced = true; > +} I had a question on this bit. If say cpu 0 executes klp_force_transition(void), right up until klp_forced is set to true, and then cpu 1 does klp_complete_transition() (since all threads have the correct state), wouldn't it be possible then for klp_complete_transition() to not see klp_forced set to true, and thus the module could be potentially removed even though it was forced? If so, I think that the force path just needs to be set before the threads are updated (as below). I don't think that the klp_complete_transition() needs the corresponding rmb, b/c there is sufficient ordering there already (although it would deserve a comment). Thanks, -Jason diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c index be5bfa5..cca6a3a 100644 --- a/kernel/livepatch/transition.c +++ b/kernel/livepatch/transition.c @@ -671,6 +671,15 @@ void klp_force_transition(void) pr_warn("forcing remaining tasks to the patched state\n"); + klp_forced = true; + + /* + * ensure that if klp_complete_transition() sees that all + * the threads have been updated to desired task->patch_state + * that we also see klp_forced = true; + */ + smp_wmb(); + read_lock(&tasklist_lock); for_each_process_thread(g, task) klp_update_patch_state(task); @@ -678,6 +687,4 @@ void klp_force_transition(void) for_each_possible_cpu(cpu) klp_update_patch_state(idle_task(cpu)); - - klp_forced = true; }