linux-kernel - Re: [PATCH v4.1 2/2] livepatch: force transition to finish

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20171219132722.v6lkmluvxdjgevrf@pathway.suse.cz>
Date:   Tue, 19 Dec 2017 14:27:22 +0100
From:   Petr Mladek <pmladek@...e.com>
To:     Miroslav Benes <mbenes@...e.cz>
Cc:     Jason Baron <jbaron@...mai.com>, jpoimboe@...hat.com,
        jeyu@...nel.org, jikos@...nel.org, lpechacek@...e.cz, pavel@....cz,
        live-patching@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4.1 2/2] livepatch: force transition to finish

On Mon 2017-12-18 14:23:40, Miroslav Benes wrote:
> On Fri, 15 Dec 2017, Jason Baron wrote:
> 
> > 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 <mbenes@...e.cz>
> > > Acked-by: Josh Poimboeuf <jpoimboe@...hat.com>
> > > Reviewed-by: Petr Mladek <pmladek@...e.com>
> > > ---
> > >  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?
> 
> Yes, you're right. That could happen.
>  
> > 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).
> 
> Or we can take klp_mutex in force_store() (kernel/livepatch/core.c) and be 
> done with it once and for all. The problem is exactly what Petr predicted 
> and I refused to have klp_mutex here just because it may have fixed 
> theoretical issue. 
> 
> Petr, Josh, what do you think?

Sounds good to me. I would feel better if the lock is there.

Best Regards,
Petr