netdev - Re: [PATCH net-next 08/14] ice: protect init and calibrating fields with spinlock

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

Message-ID: <05b266b6-0be2-3326-2877-d26e1bd60fdf@intel.com>
Date:   Mon, 5 Dec 2022 11:57:28 -0800
From:   Jacob Keller <jacob.e.keller@...el.com>
To:     Leon Romanovsky <leon@...nel.org>
CC:     Tony Nguyen <anthony.l.nguyen@...el.com>, <davem@...emloft.net>,
        <kuba@...nel.org>, <pabeni@...hat.com>, <edumazet@...gle.com>,
        <netdev@...r.kernel.org>, <richardcochran@...il.com>,
        Gurucharan G <gurucharanx.g@...el.com>
Subject: Re: [PATCH net-next 08/14] ice: protect init and calibrating fields
 with spinlock



On 12/4/2022 5:30 AM, Leon Romanovsky wrote:
> On Fri, Dec 02, 2022 at 03:07:16PM -0800, Jacob Keller wrote:
>>
>>
>> On 12/2/2022 10:36 AM, Jacob Keller wrote:
>>> I am not sure what the best way to fix c) is. Perhaps an additional flag
>>> of some sort which indicates that the timestamp thread function is
>>> processing so that tear down can wait until after the interrupt function
>>> completes. Once init is cleared, the function will stop re-executing,
>>> but we need a way to wait until it has stopped. That method in tear down
>>> can't hold the lock or else we'd potentially deadlock... Maybe an
>>> additional "processing" flag which is set under lock only if the init
>>> flag is set? and then cleared when the function exits. Then the tear
>>> down can check and wait for the processing to be complete? Hmm.
>>>
>>
>> For what its worth, I think this is an issue regardless of whether this
>> series is applied, since the change from kthread to threaded IRQ was done a
>> while ago.
> 
> Sorry, I can't say anything clear without deep dive into ice code. But this specific
> patch is not correct.
> 
> Thanks

I understand. Its rather complicated :( I appreciate the review. I've 
got a new version we're testing now that drops the changes around 
tx->init for the ice_ptp_tx_tstamp function and adds a call to 
synchronize_irq in the teardown path to ensure that we wait for any 
outstanding interrupt before continuing teardown. I believe that should 
address your concerns. Hopefully we can send a fixed version of the 
series soon once it completes some internal testing.

Thanks,
Jake