netdev - RE: [PATCH net-next] net: fec: Convert fec driver to use lock guards

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]

Date: Sun, 23 Jun 2024 10:22:38 +0200
From: Markus Elfring <Markus.Elfring@....de>
To: Wei Fang <wei.fang@....com>, netdev@...r.kernel.org,
 kernel-janitors@...r.kernel.org, imx@...ts.linux.dev,
 Andrew Lunn <andrew@...n.ch>
Cc: "David S. Miller" <davem@...emloft.net>,
 Clark Wang <xiaoning.wang@....com>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Julia Lawall <julia.lawall@...ia.fr>,
 Paolo Abeni <pabeni@...hat.com>, Peter Zijlstra <peterz@...radead.org>,
 Richard Cochran <richardcochran@...il.com>,
 Shenwei Wang <shenwei.wang@....com>, Waiman Long <longman@...hat.com>
Subject: RE: [PATCH net-next] net: fec: Convert fec driver to use lock guards

…
> > > +++ b/drivers/net/ethernet/freescale/fec_ptp.c
> > > @@ -99,18 +99,17 @@
> > >   */
> > >  static int fec_ptp_enable_pps(struct fec_enet_private *fep, uint enable)
> > >  {
> > > -	unsigned long flags;
> > >  	u32 val, tempval;
> > >  	struct timespec64 ts;
> > >  	u64 ns;
> > >
> > > -	if (fep->pps_enable == enable)
> > > -		return 0;
> > > -
> > >  	fep->pps_channel = DEFAULT_PPS_CHANNEL;
> > >  	fep->reload_period = PPS_OUPUT_RELOAD_PERIOD;
> > >
> > > -	spin_lock_irqsave(&fep->tmreg_lock, flags);
> > > +	guard(spinlock_irqsave)(&fep->tmreg_lock);
> > > +
> > > +	if (fep->pps_enable == enable)
> > > +		return 0;
> >
> > This is not obviously correct. Why has this condition moved?
> >
> As you see, the assignment of ' pps_enable ' is protected by the 'tmreg_lock',
> But the read operation of 'pps_enable' was not protected by the lock, so the
> Coverity tool will complain a LOCK EVASION warning which may cause data
> race to occur when running in a multithreaded environment.

Should such information trigger the addition of any corresponding tags
(like “Fixes” and “Cc”)?

>                                                            Of course, this
> data race issue is almost impossible, so I modified it by the way. Because I don't
> really want to fix it through another patch, unless you insist on doing so.

Would you like to take the known advice “Solve only one problem per patch”
better into account?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.10-rc4#n81

Please take another look at further approaches for the presentation of
similar “change combinations”.

Regards,
Markus