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]
Date: Mon, 20 Nov 2023 10:11:17 +0100
From: Alexandra Winter <wintera@...ux.ibm.com>
To: dust.li@...ux.alibaba.com, Li RongQing <lirongqing@...du.com>,
        kgraul@...ux.ibm.com, wenjia@...ux.ibm.com, jaka@...ux.ibm.com,
        alibuda@...ux.alibaba.com, tonylu@...ux.alibaba.co,
        guwen@...ux.alibaba.com, davem@...emloft.net, edumazet@...gle.com,
        kuba@...nel.org, pabeni@...hat.com, linux-s390@...r.kernel.org,
        netdev@...r.kernel.org
Subject: Re: [PATCH net-next v3] net/smc: avoid atomic_set and smp_wmb in the
 tx path when possible



On 20.11.23 04:20, Dust Li wrote:
>> It seems to me that the purpose of conn->tx_pushing is
>> a) Serve as a mutex, so only one thread per conn will call __smc_tx_sndbuf_nonempty().
>> b) Repeat, in case some other thread has added data to sndbuf concurrently.
>>
>> I agree that this patch does not change the behaviour of this function and removes an
>> atomic_set() in the likely path.
>>
>> I wonder however: All callers of smc_tx_sndbuf_nonempty() must hold the socket lock.
>> So how can we ever run in a concurrency situation?
>> Is this handling of conn->tx_pushing necessary at all?
> Hi Sandy,
> 
> Overall, I think you are right. But there is something we need to take care.
> 
> Before commit 6b88af839d20 ("net/smc: don't send in the BH context if
> sock_owned_by_user"), we used to call smc_tx_pending() in the soft IRQ,
> without checking sock_owned_by_user(), which would caused a race condition
> because bh_lock_sock() did not honor sock_lock(). To address this issue,
> I have added the tx_pushing mechanism. However, with commit 6b88af839d20,
> we now defer the transmission if sock_lock() is held by the user.
> Therefore, there should no longer be a race condition. Nevertheless, if
> we remove the tx_pending mechanism, we must always remember not to call
> smc_tx_sndbuf_nonempty() in the soft IRQ when the user holds the sock lock.
> 
> Thanks
> Dust


ok, I understand.
So whoever is willing to give it a try and simplify smc_tx_sndbuf_nonempty(),
should remember to document that requirement/precondition.
Maybe in a Function context section of a kernel-doc function decription?
(as described in https://docs.kernel.org/doc-guide/kernel-doc.html)
Although smc_tx_sndbuf_nonempty() is not exported, this format is helpful.

Powered by blists - more mailing lists