netdev - Re: [PATCH 1/1] s390/ism: fix concurrency management in ism

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: <20250721095621.GB2459@horms.kernel.org>
Date: Mon, 21 Jul 2025 10:56:21 +0100
From: Simon Horman <horms@...nel.org>
To: Alexandra Winter <wintera@...ux.ibm.com>
Cc: Alexander Gordeev <agordeev@...ux.ibm.com>,
	Halil Pasic <pasic@...ux.ibm.com>,
	Andrew Lunn <andrew+netdev@...n.ch>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Thorsten Winkler <twinkler@...ux.ibm.com>,
	Heiko Carstens <hca@...ux.ibm.com>,
	Vasily Gorbik <gor@...ux.ibm.com>,
	Christian Borntraeger <borntraeger@...ux.ibm.com>,
	Sven Schnelle <svens@...ux.ibm.com>,
	Sebastian Ott <sebott@...ux.ibm.com>,
	Ursula Braun <ubraun@...ux.ibm.com>, netdev@...r.kernel.org,
	linux-s390@...r.kernel.org, linux-kernel@...r.kernel.org,
	Aliaksei Makarau <Aliaksei.Makarau@....com>,
	Mahanta Jambigi <mjambigi@...ux.ibm.com>
Subject: Re: [PATCH 1/1] s390/ism: fix concurrency management in ism_cmd()

On Mon, Jul 21, 2025 at 10:17:30AM +0200, Alexandra Winter wrote:
> 
> 
> On 21.07.25 09:30, Alexander Gordeev wrote:
> > On Sun, Jul 20, 2025 at 11:11:09PM +0200, Halil Pasic wrote:
> > 
> > Hi Halil,
> > 
> > ...
> >> @@ -129,7 +129,9 @@ static int ism_cmd(struct ism_dev *ism, void *cmd)
> >>  {
> >>  	struct ism_req_hdr *req = cmd;
> >>  	struct ism_resp_hdr *resp = cmd;
> >> +	unsigned long flags;
> >>  
> >> +	spin_lock_irqsave(&ism->cmd_lock, flags);
> > 
> > I only found smcd_handle_irq() scheduling a tasklet, but no commands issued.
> > Do we really need disable interrupts?
> 
> You are right in current code, the interrupt and event handlers of ism and smcd
> never issue a control command that calls ism_cmd().
> OTOH, future ism clients could do that.
> The control commands are not part of the data path, but of connection establish.
> So I don't really expect a performance impact.
> I have it on my ToDo list, to change this to threaded interrupts in the future.
> So no strong opinion on my side.
> Simple spin_lock is fine with me.

I would suggest using spin_lock() if it is sufficient.

I think it is generally assumed that the minimal locking primitive is used
given the context code is executed in.  And we can it can always be updated
if the contexts in which this code executes subsequently changes.

IOW, I'm suggesting avoiding confusion if someone looks over this code.

...