netdev - Re: [PATCH net-next v3] net/smc: avoid atomic_set and smp

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 17:49:52 +0800
From: Tony Lu <tonylu@...ux.alibaba.com>
To: Li RongQing <lirongqing@...du.com>
Cc: dust.li@...ux.alibaba.com, Alexandra Winter <wintera@...ux.ibm.com>,
	kgraul@...ux.ibm.com, wenjia@...ux.ibm.com, jaka@...ux.ibm.com,
	alibuda@...ux.alibaba.com, 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 Mon, Nov 20, 2023 at 10:17:15AM +0100, Alexandra Winter wrote:
> 
> 
> On 20.11.23 10:11, Alexandra Winter wrote:
> > 
> > 
> > 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.
> > 
> 
> 
> Tony Lu <tonylu@...ux.alibaba.com> ' mail address has been corrupted in this whole thread.
> Please reply to this message (corrected address) or take care, if replying to
> other messages in this thread.

Yes, that's true. Thanks Alexandra.

Please use my correct address, RongQing: Tony Lu <tonylu@...ux.alibaba.com>.