linux-kernel - Re: [PATCH v3 1/2] HID: bigben: use spinlock to safely schedule workers

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

Message-ID: <CAO-hwJ++sRYUXhu9URU_=12y1nwC+pgEfPwmtyWisQenkZkiiA@mail.gmail.com>
Date:   Mon, 13 Feb 2023 11:47:40 +0100
From:   Benjamin Tissoires <benjamin.tissoires@...hat.com>
To:     Hillf Danton <hdanton@...a.com>
Cc:     Pietro Borrello <borrello@...g.uniroma1.it>,
        Jiri Kosina <jikos@...nel.org>, Hanno Zulla <kontakt@...no.de>,
        linux-input@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 1/2] HID: bigben: use spinlock to safely schedule workers

On Mon, Feb 13, 2023 at 11:37 AM Hillf Danton <hdanton@...a.com> wrote:
>
> On Mon, 13 Feb 2023 09:25:37 +0100 Benjamin Tissoires <benjamin.tissoires@...hat.com>
> >
> > The remove flag is just a way to prevent any other workqueue from
> > starting. It doesn't mean that the struct bigben has been freed, so
> > acquiring a lock at that point is fine.
> > We then rely on 2 things:
> > - devm_class_led to be released before struct bigben, because it was
> > devm-allocated *after* the struct bigben was devm-allocated
>
> This is the current behavior and it is intact after this patch.
>
> > - we prevent any new workqueue to start and we guarantee that any
> > running workqueue is terminated before leaving the .remove function.
>
> If spinlock is added for not scheduling new workqueue then it is not
> needed, because the removed flag is set before running workqueue is
> terminated. Checking the flag is enough upon queuing new work.
>

I tend to disagree (based on Pietro's v4:
- no worker is running
- a led sysfs call is made
- the line "if (!bigben->removed)" is true
  - this gets interrupted/or another CPU kicks in for the next one
-> .remove gets called
- bigben->removed is set to false
- cancel_work_sync() called

the led call continues, and schedules the work

.removes terminates, and devm kicks in, killing led_class and struct
bigben while the workqueue is running.

So having a simple spinlocks ensures the atomicity between checking
for bigben->removed and scheduling a workqueue.

Cheers,
Benjamin