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: <20250318084737.e27y6x4mrky47ih4@skbuf>
Date: Tue, 18 Mar 2025 10:47:37 +0200
From: Vladimir Oltean <vladimir.oltean@....com>
To: Claudiu Manoil <claudiu.manoil@....com>
Cc: Wei Fang <wei.fang@....com>, Clark Wang <xiaoning.wang@....com>,
	"andrew+netdev@...n.ch" <andrew+netdev@...n.ch>,
	"davem@...emloft.net" <davem@...emloft.net>,
	"edumazet@...gle.com" <edumazet@...gle.com>,
	"kuba@...nel.org" <kuba@...nel.org>,
	"pabeni@...hat.com" <pabeni@...hat.com>,
	"christophe.leroy@...roup.eu" <christophe.leroy@...roup.eu>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"imx@...ts.linux.dev" <imx@...ts.linux.dev>,
	"linuxppc-dev@...ts.ozlabs.org" <linuxppc-dev@...ts.ozlabs.org>,
	"linux-arm-kernel@...ts.infradead.org" <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v4 net-next 04/14] net: enetc: add MAC filter for i.MX95
 ENETC PF

Hi Claudiu,

On Tue, Mar 18, 2025 at 10:08:24AM +0200, Claudiu Manoil wrote:
> 
> > -----Original Message-----
> > From: Vladimir Oltean <vladimir.oltean@....com>
> > Sent: Monday, March 17, 2025 4:18 PM
> [...]
> > Subject: Re: [PATCH v4 net-next 04/14] net: enetc: add MAC filter for i.MX95
> > ENETC PF
> > 
> > On Tue, Mar 11, 2025 at 01:38:20PM +0800, Wei Fang wrote:
> [...]
> > > +static void enetc4_pf_destroy_mac_list(struct enetc_pf *pf)
> > > +{
> > > +	struct enetc_mac_list_entry *entry;
> > > +	struct hlist_node *tmp;
> > > +
> > > +	mutex_lock(&pf->mac_list_lock);
> > 
> > The mutex_lock() usage here should raise serious questions. This is
> > running right before mutex_destroy(). So if there were any concurrent
> > attempt to acquire this lock, that concurrent code would have been broken
> > any time it would have lost arbitration, by the fact that it would
> > attempt to acquire a destroyed mutex.
> > 
> > But there's no such concurrent thread, because we run after destroy_workqueue()
> > which flushes those concurrent calls and prevents new ones. So the mutex
> > usage here is not necessary.
> > 
> > [ same thing with mutex_init() immediately followed by mutex_lock().
> >   It is an incorrect pattern most of the time. ]
> > 
> 
> This is not as bad as it seems. In the final version of the code, mutex 'mac_list_lock'
> serializes the access btw the thread programming the filter for the PF instance,
> and the threads programming the filter on behalf of underlying VFs (triggered by async
> request from VF). But since VF support is not included in this patch set (as Wei
> already mentioned) the lock can/should be added later, with the VF patches.

We all agree that the lock is unnecessary as far as the logic presented
in this patch set is concerned, and thus should not be added.

When Wei will present new code that will make explicit serialization
necessary, we can comment more. But, I don't see how any extra logic
can change the basic fact I was pointing out in the portion that you've
quoted. Before you destroy the mutex, you need to do something that
ensures concurrent threads can no longer execute. Then, there's no point
in locking in enetc4_pf_destroy_mac_list(). I expect not to see such
locking there in future patch sets.

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ