netdev - Re: [PATCH v8 10/12] pps: generators: Add PPS Generator TIO Driver

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: <ZlSZ63ST-Pj9CwCh@surfacebook.localdomain>
Date: Mon, 27 May 2024 17:34:19 +0300
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: "D, Lakshmi Sowjanya" <lakshmi.sowjanya.d@...el.com>
Cc: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
	"tglx@...utronix.de" <tglx@...utronix.de>,
	"jstultz@...gle.com" <jstultz@...gle.com>,
	"giometti@...eenne.com" <giometti@...eenne.com>,
	"corbet@....net" <corbet@....net>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"x86@...nel.org" <x86@...nel.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
	"intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>,
	"Dong, Eddie" <eddie.dong@...el.com>,
	"Hall, Christopher S" <christopher.s.hall@...el.com>,
	"Brandeburg, Jesse" <jesse.brandeburg@...el.com>,
	"davem@...emloft.net" <davem@...emloft.net>,
	"alexandre.torgue@...s.st.com" <alexandre.torgue@...s.st.com>,
	"joabreu@...opsys.com" <joabreu@...opsys.com>,
	"mcoquelin.stm32@...il.com" <mcoquelin.stm32@...il.com>,
	"perex@...ex.cz" <perex@...ex.cz>,
	"linux-sound@...r.kernel.org" <linux-sound@...r.kernel.org>,
	"Nguyen, Anthony L" <anthony.l.nguyen@...el.com>,
	"peter.hilber@...nsynergy.com" <peter.hilber@...nsynergy.com>,
	"N, Pandith" <pandith.n@...el.com>,
	"Mohan, Subramanian" <subramanian.mohan@...el.com>,
	"T R, Thejesh Reddy" <thejesh.reddy.t.r@...el.com>
Subject: Re: [PATCH v8 10/12] pps: generators: Add PPS Generator TIO Driver

Mon, May 27, 2024 at 11:48:54AM +0000, D, Lakshmi Sowjanya kirjoitti:
> > -----Original Message-----
> > From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
> > Sent: Monday, May 13, 2024 4:49 PM
> > On Mon, May 13, 2024 at 04:08:11PM +0530, lakshmi.sowjanya.d@...el.com
> > wrote:

...

> > > +static ssize_t enable_store(struct device *dev, struct device_attribute
> > *attr, const char *buf,
> > > +			    size_t count)
> > > +{
> > > +	struct pps_tio *tio = dev_get_drvdata(dev);
> > > +	bool enable;
> > > +	int err;
> > 
> > (1)
> > 
> > > +	err = kstrtobool(buf, &enable);
> > > +	if (err)
> > > +		return err;
> > > +
> > > +	guard(spinlock_irqsave)(&tio->lock);
> > > +	if (enable && !tio->enabled) {
> > 
> > > +		if (!timekeeping_clocksource_has_base(CSID_X86_ART)) {
> > > +			dev_err(tio->dev, "PPS cannot be started as clock is
> > not related
> > > +to ART");
> > 
> > Why not simply dev_err(dev, ...)?
> > 
> > > +			return -EPERM;
> > > +		}
> > 
> > I'm wondering if we can move this check to (1) above.
> > Because currently it's a good question if we are able to stop PPS which was
> > run by somebody else without this check done.
> 
> Do you mean can someone stop the signal without this check? 
> Yes, this check is not required to stop.  So, I feel it need not be moved to (1).
> 
> Please, correct me if my understanding is wrong.

So, there is a possibility to have a PPS being run (by somebody else) even if
there is no ART provided?

If "yes", your check is wrong to begin with. If "no", my suggestion is correct,
i.e. there is no need to stop something that can't be started at all.

> > I.o.w. this sounds too weird to me and reading the code doesn't give any hint
> > if it's even possible. And if it is, are we supposed to touch that since it was
> > definitely *not* us who ran it.
> 
> Yes, we are not restricting on who can stop/start the signal. 

See above. It's not about this kind of restriction.

> > > +		pps_tio_direction_output(tio);
> > > +		hrtimer_start(&tio->timer, first_event(tio),
> > HRTIMER_MODE_ABS);
> > > +		tio->enabled = true;
> > > +	} else if (!enable && tio->enabled) {
> > > +		hrtimer_cancel(&tio->timer);
> > > +		pps_tio_disable(tio);
> > > +		tio->enabled = false;
> > > +	}
> > > +	return count;
> > > +}

-- 
With Best Regards,
Andy Shevchenko