linux-kernel - Re: [PATCH 1/9] platform/surface: Add Surface Aggregator subsystem

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: <596dd647-b874-29f5-5bbf-a02f9d6ac587@gmail.com>
Date:   Tue, 17 Nov 2020 07:05:13 +0100
From:   Maximilian Luz <luzmaximilian@...il.com>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Hans de Goede <hdegoede@...hat.com>,
        Mark Gross <mgross@...ux.intel.com>,
        Arnd Bergmann <arnd@...db.de>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Rob Herring <robh@...nel.org>,
        Jiri Slaby <jirislaby@...nel.org>,
        "Rafael J . Wysocki" <rjw@...ysocki.net>,
        Len Brown <lenb@...nel.org>,
        Blaž Hrastnik <blaz@...n.io>,
        Dorian Stoll <dorian.stoll@...p.io>,
        Platform Driver <platform-driver-x86@...r.kernel.org>,
        "open list:SERIAL DRIVERS" <linux-serial@...r.kernel.org>,
        ACPI Devel Maling List <linux-acpi@...r.kernel.org>
Subject: Re: [PATCH 1/9] platform/surface: Add Surface Aggregator subsystem

On 11/16/20 6:03 PM, Maximilian Luz wrote:
> On 11/16/20 2:33 PM, Andy Shevchenko wrote:
>> On Sun, Nov 15, 2020 at 9:25 PM Maximilian Luz <luzmaximilian@...il.com> wrote:

[...]

> READ_ONCE and WRITE_ONCE are used to ensure proper access to state that
> can be changed outside of the queue/pending locks (or under any one of
> them). In general, I have tried to document all critical access of such
> state with an explanation of why it is safe to do so.

I've looked at this again and noticed that I can guard the packet
timestamp by the pending lock and the packet priority by the queue lock
(after first submission). This makes reasoning about access to them
significantly easier and removes the need for WRITE_ONCE / READ_ONCE.

After that, READ_ONCE is used

- to access the controller state for smoke-testing to (hopefully) detect
   invalid request function usage (note that all other access to this
   state happens under the controller state lock)

- for the "safe counters", where access to the shared value is, after
   initialization, restricted to the increment function

- to update the timeout reaper, where access to the shared value
   (rtx_timeout.expires) is, after initialization, restricted to its
   modification function (ssh_ptl_timeout_reaper_mod() /
   ssh_rtl_timeout_reaper_mod()) and the timer function

- to access the request timestamp, which is, after initialization, only
   set once in the lifetime of a request (all other access is read-only)

- to access the 'ptl' reference of the packet, which, after
   initialization, is only set once, either at packet or request
   submission (all other access is read-only). Note due to this,
   READ_ONCE is only required for functions that can run concurrently
   with ssh_ptl_submit() and ssh_rtl_submit(), i.e. ssh_ptl_cancel() and
   ssh_rtl_cancel().

- to access request state outside of bit-ops when canceling

I'd argue that all of these cases can be checked and verified with a
reasonable amount of effort. Cancellation (last two points) is probably
the most complex one. Unfortunately, I don't see any way to simplify
that part without disallowing cancellation to run concurrently to
submission, which is something I'd like to support as this makes
implementing asynchronous requests in the future easier.

Regards,
Max