linux-kernel - Re: [PATCH] cpuhp: make target_store() a nop when target == state

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <Yo0z56Fqgj3gqYlG@lorien.usersys.redhat.com>
Date:   Tue, 24 May 2022 15:37:11 -0400
From:   Phil Auld <pauld@...hat.com>
To:     Valentin Schneider <vschneid@...hat.com>
Cc:     linux-kernel@...r.kernel.org, Thomas Gleixner <tglx@...utronix.de>,
        Peter Zijlstra <peterz@...radead.org>
Subject: Re: [PATCH] cpuhp: make target_store() a nop when target == state

Hi Valentin,

On Tue, May 24, 2022 at 04:11:51PM +0100 Valentin Schneider wrote:
> On 23/05/22 10:47, Phil Auld wrote:
> > writing the current state back into hotplug/target calls cpu_down()
> > which will set cpu dying even when it isn't and then nothing will
> > ever clear it. A stress test that reads values and writes them back
> > for all cpu device files in sysfs will trigger the BUG() in
> > select_fallback_rq once all cpus are marked as dying.
> >
> > kernel/cpu.c::target_store()
> > 	...
> >         if (st->state < target)
> >                 ret = cpu_up(dev->id, target);
> >         else
> >                 ret = cpu_down(dev->id, target);
> >
> > cpu_down() -> cpu_set_state()
> > 	 bool bringup = st->state < target;
> > 	 ...
> > 	 if (cpu_dying(cpu) != !bringup)
> > 		set_cpu_dying(cpu, !bringup);
> >
> > Make this safe by catching the case where target == state
> > and bailing early.
> >
> > Signed-off-by: Phil Auld <pauld@...hat.com>
> > ---
> >
> > Yeah, I know... don't do that. But it's still messy.
> >
> > !< != > 
> >
> >  kernel/cpu.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/kernel/cpu.c b/kernel/cpu.c
> > index d0a9aa0b42e8..8a71b1149c60 100644
> > --- a/kernel/cpu.c
> > +++ b/kernel/cpu.c
> > @@ -2302,6 +2302,9 @@ static ssize_t target_store(struct device *dev, struct device_attribute *attr,
> >  		return -EINVAL;
> >  #endif
> >  
> > +	if (target == st->state)
> > +		return count;
> > +
> 
> The current checks are against static boundaries, this has to compare
> against st->state - AFAICT this could race with another hotplug operation
> to the same CPU, e.g.
> 
>   CPU42.cpuhp_state
>     ->state  == CPUHP_AP_SCHED_STARTING
>     ->target == CPUHP_ONLINE
> 
>   <write CPUHP_ONLINE via sysfs, OK because current state != CPUHP_ONLINE>
> 
>   CPU42.cpuhp_state == CPUHP_ONLINE
>   
>   <issues ensue>
> 
> 
> _cpu_up() has:
> 
> 	/*
> 	 * The caller of cpu_up() might have raced with another
> 	 * caller. Nothing to do.
> 	 */
> 	if (st->state >= target)
> 		goto out;
> 
> Looks like we want an equivalent in _cpu_down(), what do you think?
>

I did it like this (shown below) and from my test it also works for
this case.

I could move it below the lock and goto out;  instead if you think
that is better. It still seems better to me to stop this higher up
because there's work being done in the out path too.  We're not
actually doing any hot(un)plug so doing post unplug cleanup seems
iffy.

_cpu_down()
...
out:
        cpus_write_unlock();
        /*
         * Do post unplug cleanup. This is still protected against
         * concurrent CPU hotplug via cpu_add_remove_lock.
         */
        lockup_detector_cleanup();
        arch_smt_update();
        cpu_up_down_serialize_trainwrecks(tasks_frozen);
	return ret;
}

----------

diff --git a/kernel/cpu.c b/kernel/cpu.c
index 8a71b1149c60..e36788742d18 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -1130,6 +1130,13 @@ static int __ref _cpu_down(unsigned int cpu, int tasks_frozen,
 	if (!cpu_present(cpu))
 		return -EINVAL;
 
+	/*
+	 * The caller of cpu_down() might have raced with another
+	 * caller. Nothing to do.
+	 */
+	if (st->state <= target)
+		return 0;
+
 	cpus_write_lock();
 
 	cpuhp_tasks_frozen = tasks_frozen;




Cheers,
Phil

--