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] [thread-next>] [day] [month] [year] [list]

Message-ID:
 <AM0PR0402MB38910DB23A6DABF1C074EF1D88E52@AM0PR0402MB3891.eurprd04.prod.outlook.com>
Date: Wed, 8 May 2024 03:29:10 +0000
From: Wei Fang <wei.fang@....com>
To: Andrew Lunn <andrew@...n.ch>
CC: "davem@...emloft.net" <davem@...emloft.net>, "edumazet@...gle.com"
	<edumazet@...gle.com>, "kuba@...nel.org" <kuba@...nel.org>,
	"pabeni@...hat.com" <pabeni@...hat.com>, Shenwei Wang <shenwei.wang@....com>,
	Clark Wang <xiaoning.wang@....com>, "richardcochran@...il.com"
	<richardcochran@...il.com>, "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>
Subject: RE: [PATCH net-next] net: fec: Convert fec driver to use lock guards

> -----Original Message-----
> From: Andrew Lunn <andrew@...n.ch>
> Sent: 2024年5月7日 22:01
> To: Wei Fang <wei.fang@....com>
> Cc: davem@...emloft.net; edumazet@...gle.com; kuba@...nel.org;
> pabeni@...hat.com; Shenwei Wang <shenwei.wang@....com>; Clark Wang
> <xiaoning.wang@....com>; richardcochran@...il.com;
> netdev@...r.kernel.org; linux-kernel@...r.kernel.org; imx@...ts.linux.dev
> Subject: Re: [PATCH net-next] net: fec: Convert fec driver to use lock guards
> 
> On Tue, May 07, 2024 at 05:05:20PM +0800, Wei Fang wrote:
> > Use guard() and scoped_guard() defined in linux/cleanup.h to automate
> > lock lifetime control in fec driver.
> 
> You are probably the first to use these in netdev. Or one of the very
> early adopters. As such, you should explain in a bit more detail why
> these changes are safe.
>
Okay, I can add more in the commit message.

> > -	spin_lock_irqsave(&fep->tmreg_lock, flags);
> > -	ns = timecounter_cyc2time(&fep->tc, ts);
> > -	spin_unlock_irqrestore(&fep->tmreg_lock, flags);
> > +	scoped_guard(spinlock_irqsave, &fep->tmreg_lock) {
> > +		ns = timecounter_cyc2time(&fep->tc, ts);
> > +	}
> 
> This looks fine.
> 
> > -			mutex_lock(&fep->ptp_clk_mutex);
> > -			ret = clk_prepare_enable(fep->clk_ptp);
> > -			if (ret) {
> > -				mutex_unlock(&fep->ptp_clk_mutex);
> > -				goto failed_clk_ptp;
> > -			} else {
> > -				fep->ptp_clk_on = true;
> > +			scoped_guard(mutex, &fep->ptp_clk_mutex) {
> > +				ret = clk_prepare_enable(fep->clk_ptp);
> > +				if (ret)
> > +					goto failed_clk_ptp;
> > +				else
> > +					fep->ptp_clk_on = true;
> >  			}
> 
> As Eric pointed out, it is not obvious what the semantics are
> here. You are leaving the scope, so i hope it does not matter it is a
> goto you are using to leave the scope. But a quick search did not find
> anything to confirm this. So i would like to see some justification in
> the commit message this is safe.
>
According to the explanation of the cleanup attribute of gcc [1] and clang [2],
the cleanup attribute runs a function when the variable goes out of scope. So
the lock will be freed when leaving the scope.
In addition, I have seen cases of using goto statements in scope_guard() in
the gpiolib driver [3].

[1] https://gcc.gnu.org/onlinedocs/gcc/Common-Variable-Attributes.html#index-cleanup-variable-attribute
[2] https://clang.llvm.org/docs/AttributeReference.html#cleanup
[3] https://elixir.bootlin.com/linux/v6.9-rc7/source/drivers/gpio/gpiolib.c#L930

> > +++ 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. 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.

> I also personally don't like guard(). scoped_guard() {} is much easier
> to understand.
> 
If the scope of the lock is from the time it is acquired until the function returns,
I think guard() is simpler. Of course, you may think scope_guard() is more reasonable
based on other considerations.

> In order to get my Reviewed-by: you need to drop all the plain guard()
> calls. I'm also not sure as a community we want to see changes like
> this.
> 
Why I do this is because I see more and more drivers converting to using
Scope-based resource management mechanisms to manage resources,
not just locks, but memory and some other resources. I think the community
should actively embrace this new mechanism.