linux-kernel - Re: [RFC 08/10] platform/x86/intel/ifs: Add IFS sysfs interface

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: <7c620f8a-189e-5ac4-30fe-1fa14ba799ea@intel.com>
Date:   Mon, 7 Mar 2022 11:55:26 -0800
From:   "Joseph, Jithu" <jithu.joseph@...el.com>
To:     Dan Williams <dan.j.williams@...el.com>
CC:     "hdegoede@...hat.com" <hdegoede@...hat.com>,
        "markgross@...nel.org" <markgross@...nel.org>,
        "corbet@....net" <corbet@....net>,
        "Raj, Ashok" <ashok.raj@...el.com>,
        "Luck, Tony" <tony.luck@...el.com>,
        "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
        "patches@...ts.linux.dev" <patches@...ts.linux.dev>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "mingo@...hat.com" <mingo@...hat.com>,
        "rostedt@...dmis.org" <rostedt@...dmis.org>,
        "Shankar, Ravi V" <ravi.v.shankar@...el.com>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "platform-driver-x86@...r.kernel.org" 
        <platform-driver-x86@...r.kernel.org>,
        "linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
        "hpa@...or.com" <hpa@...or.com>, "bp@...en8.de" <bp@...en8.de>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "andriy.shevchenko@...ux.intel.com" 
        <andriy.shevchenko@...ux.intel.com>,
        "x86@...nel.org" <x86@...nel.org>
Subject: Re: [RFC 08/10] platform/x86/intel/ifs: Add IFS sysfs interface



On 3/7/2022 11:15 AM, Dan Williams wrote:
> On Mon, Mar 7, 2022 at 11:09 AM Joseph, Jithu <jithu.joseph@...el.com> wrote:
>>
>>
>>
>> On 3/7/2022 9:38 AM, Dan Williams wrote:
>>> On Fri, Mar 4, 2022 at 12:42 PM Joseph, Jithu <jithu.joseph@...el.com> wrote:
>>>>
>>>>
>>>>
>>
>>>>>> + */
>>>>>> +static ssize_t details_show(struct device *dev,
>>>>>> +                           struct device_attribute *attr,
>>>>>> +                           char *buf)
>>>>>> +{
>>>>>> +       unsigned int cpu = dev->id;
>>>>>> +       int ret;
>>>>>> +
>>>>>> +       if (down_trylock(&ifs_sem))
>>>>>> +               return -EBUSY;
>>>>>
>>>>> What is the ifs_sem protecting? This result is immediately invalid
>>>>> after the lock is dropped anyway, so why hold it over reading the
>>>>> value? You can't prevent 2 threads racing each other here.
>>>>
>>>> percpu thread running scan_test_worker() will update per_cpu(ifs_state, cpu).scan_details. (before signalling this thread to run, this lock would be acquired)
>>>> This is to protect against the scenario where if the percpu thread is running a test and if at the same time a user is querying its status, they would see busy.
>>>
>>> That begs the question why would userspace be polling this file? Is it
>>> because it does not know when a test completes otherwise? How does it
>>> know that the result it is seeing is from the test it ran and not some
>>> other invocation to start a new test?
>>
>> I think I should have explained the need for locking at a higher level .
>> It is to make sure that only one of the below action happens at any time
>>
>> 1. single percpu test
>> 2. all-cpu test (tests all cores sequentially)
>> 3. scan binary reload
>> 4. querying the status
>>
>> (There are h/w reasons for why we restrict to a  single IFS test at any time)
>> If 4 happens while 1 or 2 is in progress , we return busy.  My earlier explanation was trying to explain why we are preventing 4 when 1 or 2   is in progress.
>>
>> As to the question of why would a user do 4 while 1 or 2 is in progress, there is no reason for him to do that, both the run_test (percpu and global) are blocking.
>> But if he issues a test from one shell and uses another shell to query the status  (while it is still in progress) he will see busy.
> 
> ...and what about the race that the semaphore cannot solve of test run
> launch racing collection of previous run results?


pardon me if I am missing something obvious here … but everybody (the 4 scenarios listed above) tries to acquire the same semaphore, or returns -EBUSY if try_lock() fails.
So in case of triggering "run_test" and viewing "status" racing scenario you mention, the below are the 2 interleaving I see

if "echo 1 >  /sys/devices/sysem/cpu/cpu10/ifs/run_test" gets the lock , the parallel  "cat  /sys/devices/sysem/cpu/cpu10/ifs/status" will return busy (and not the previous status)
if  "cat  /sys/devices/sysem/cpu/cpu10/ifs/status", gets  the lock it will display the status of the last test result and  the parallel  "echo 1 >  /sys/devices/sysem/cpu/cpu10/ifs/run_test" will fail with busy

This I believe is consistent behavior.