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 for Android: free password hash cracker in your pocket

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

Message-ID: <xhsmhy1yqhrio.mognet@vschneid.remote.csb>
Date:   Wed, 25 May 2022 10:48:31 +0100
From:   Valentin Schneider <vschneid@...hat.com>
To:     Phil Auld <pauld@...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 24/05/22 12:39, Phil Auld wrote:
> On Tue, May 24, 2022 at 04:11:51PM +0100 Valentin Schneider wrote:
>
>>
>> _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?
>
> Maybe. I still think that
>
>> >         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.

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