MIME-Version: 1.0
References: <20230125-hid-unregister-leds-v3-0-0a52ac225e00@diag.uniroma1.it>
 <20230125-hid-unregister-leds-v3-1-0a52ac225e00@diag.uniroma1.it> <20230210142634.2exh6mvbvysy3hqo@mail.corp.redhat.com>
In-Reply-To: <20230210142634.2exh6mvbvysy3hqo@mail.corp.redhat.com>
From:   Pietro Borrello <borrello@diag.uniroma1.it>
Date:   Sat, 11 Feb 2023 23:27:45 +0100
Message-ID: <CAEih1qXsWT2u_M45cuY0RbOts8RM2cM6iPDju07ZtT-gtgr=xQ@mail.gmail.com>
Subject: Re: [PATCH v3 1/2] HID: bigben: use spinlock to safely schedule workers
To:     Benjamin Tissoires <benjamin.tissoires@redhat.com>
Cc:     Jiri Kosina <jikos@kernel.org>, Hanno Zulla <kontakt@hanno.de>,
        Carlo Caione <carlo@endlessm.com>,
        Cristiano Giuffrida <c.giuffrida@vu.nl>,
        "Bos, H.J." <h.j.bos@vu.nl>, Jakob Koschel <jkl820.git@gmail.com>,
        Hillf Danton <hdanton@sina.com>,
        Roderick Colenbrander <roderick@gaikai.com>,
        linux-input@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, 10 Feb 2023 at 15:26, Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> [...]
> >
> > -     if (bigben->removed || !report_field)
>
> You are removing an important test here: if (!report_field), please keep
> it.

To my understanding, that check was added in commit
918aa1ef104d ("HID: bigbenff: prevent null pointer dereference")
to prevent the NULL pointer crash, only fixing the crash point.
However, the true root cause was a missing check for output
reports patched in commit
c7bf714f8755 ("HID: check empty report_list in bigben_probe()"),
where the type-confused report list_entry was overlapping with
a NULL pointer, which was then causing the crash.
Let me know if there is any other path that may result in having a
report with no fields. In that case, it would make sense to keep the
check.

>
> > -             return;
> > +     spin_lock_irqsave(&bigben->lock, flags);
> >
> >       if (bigben->work_led) {
> >               bigben->work_led = false;
> > @@ -219,6 +229,8 @@ static void bigben_worker(struct work_struct *work)
> >               report_field->value[7] = 0x00; /* padding */
> >               hid_hw_request(bigben->hid, bigben->report, HID_REQ_SET_REPORT);
> >       }
> > +
> > +     spin_unlock_irqrestore(&bigben->lock, flags);
>
> Ouch, having hid_hw_request() called whithin a spinlock is definitely not
> something that should be done.
>
> However, the spinlock should be protecting 2 kinds of things:
> - any access to any value of struct bigben_device, but in an atomic way
>   (i.e. copy everything you need locally in a spinlock, then release it
>   and never read that struct again in that function).
> - the access to bigben->removed, which should be checked only in
>   bigben_schedule_work() and in the .remove() function.
>
> Please note that this is what the playstation driver does: it prepares
> the report under the spinlock (which is really fast) before sending the
> report to the device which can be slow and be interrupted.
>
> With that being said, it is clear that we need 2 patches for this one:
> - the first one introduces the spinlock and protects the concurrent
>   accesses to struct bigben_device (which is roughly everything below
>   with the changes I just said)
> - the second one introduces bigben_schedule_work() and piggy backs on
>   top of that new lock.

Thanks for clarifying. I will work on a v4 patch.
Best regards,
Pietro