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]

Date:   Wed, 25 May 2022 09:31:33 -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

On Wed, May 25, 2022 at 10:48:31AM +0100 Valentin Schneider wrote:
> On 24/05/22 12:39, Phil Auld wrote:

> >
> >> >         if (st->state < target)
> >> >                 ret = cpu_up(dev->id, target);
> >> >         else
> >> >                 ret = cpu_down(dev->id, target);
> >
> > is not correct. If we catch the == case earlier then this makes
> > sense as is.
> >
> > I suppose "if (st->state <= target)" would work too since __cpu_up()
> > already checks. Catching this sooner seems better to me though.
> >
> 
> Yeah it would be neater to not even enter cpu_{up, down}(), but my paranoia
> makes me think we need the comparison to happen with at least the
> cpu_add_remove_lock held to make sure st->state isn't moving under our
> feet, otherwise we may still end up with target == state in _cpu_down() and
> hit the bug you're describing.
> 

This is what I was originally doing before I tried to "optimize" it:

         if (st->state < target)
                 ret = cpu_up(dev->id, target);
         else if (st->state > target)
                 ret = cpu_down(dev->id, target);

This does the check under the lock and just falls through if state==target.
I think I'll go back to that version.

I also noticed while testing that the boot cpu does not get its target set.
It's got state 233 but target 0.  So reading that out and writing it back
on offlines cpu0.   I'll try to find where that is not getting set.  


Thanks,

Phil

> >>
> >> >    ret = lock_device_hotplug_sysfs();
> >> >    if (ret)
> >> >            return ret;
> >> > --
> >> > 2.18.0
> >>
> >
> >
> > Cheers,
> > Phil
> >
> > --
> 

--