netdev - Re: [PATCH v2 net-next 6/9] net: netdevsim: create a mock-up PTP Hardware Clock 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]

Date: Thu, 15 Jun 2023 01:17:18 +0300
From: Vladimir Oltean <vladimir.oltean@....com>
To: Simon Horman <simon.horman@...igine.com>
Cc: netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Jamal Hadi Salim <jhs@...atatu.com>,
	Cong Wang <xiyou.wangcong@...il.com>, Jiri Pirko <jiri@...nulli.us>,
	Vinicius Costa Gomes <vinicius.gomes@...el.com>,
	linux-kernel@...r.kernel.org, intel-wired-lan@...ts.osuosl.org,
	Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@...el.com>,
	Peilin Ye <yepeilin.cs@...il.com>,
	Pedro Tammela <pctammela@...atatu.com>,
	Richard Cochran <richardcochran@...il.com>,
	Zhengchao Shao <shaozhengchao@...wei.com>,
	Maxim Georgiev <glipus@...il.com>
Subject: Re: [PATCH v2 net-next 6/9] net: netdevsim: create a mock-up PTP
 Hardware Clock driver

Hi Simon,

On Wed, Jun 14, 2023 at 03:11:44PM +0200, Simon Horman wrote:
> > +#define MOCK_PHC_CC_SHIFT		31
> > +#define MOCK_PHC_CC_MULT		(1 << MOCK_PHC_CC_SHIFT)
> 
> Maybe BIT()?

Sorry, not everything that is 1 << something has BIT() semantics.
This in particular is quite clearly just a multiplier factor
expressed as a power of 2.

> > +struct mock_phc *mock_phc_create(struct device *dev)
> > +{
> > +	struct mock_phc *phc;
> > +	int err;
> > +
> > +	phc = kzalloc(sizeof(*phc), GFP_KERNEL);
> > +	if (!phc) {
> > +		err = -ENOMEM;
> > +		goto out;
> > +	}
> > +
> > +	phc->info = (struct ptp_clock_info) {
> > +		.owner		= THIS_MODULE,
> > +		.name		= "Mock-up PTP clock",
> > +		.max_adj	= MOCK_PHC_MAX_ADJ_PPB,
> > +		.adjfine	= mock_phc_adjfine,
> > +		.adjtime	= mock_phc_adjtime,
> > +		.gettime64	= mock_phc_gettime64,
> > +		.settime64	= mock_phc_settime64,
> > +		.do_aux_work	= mock_phc_refresh,
> > +	};
> > +
> > +	phc->cc = (struct cyclecounter) {
> > +		.read	= mock_phc_cc_read,
> > +		.mask	= CYCLECOUNTER_MASK(64),
> > +		.mult	= MOCK_PHC_CC_MULT,
> > +		.shift	= MOCK_PHC_CC_SHIFT,
> > +	};
> > +
> > +	spin_lock_init(&phc->lock);
> > +	timecounter_init(&phc->tc, &phc->cc, 0);
> > +
> > +	phc->clock = ptp_clock_register(&phc->info, dev);
> > +	if (IS_ERR_OR_NULL(phc->clock)) {
> > +		err = PTR_ERR_OR_ZERO(phc->clock);
> > +		goto out_free_phc;
> > +	}
> > +
> > +	ptp_schedule_worker(phc->clock, MOCK_PHC_REFRESH_INTERVAL);
> > +
> > +	return phc;
> > +
> > +out_free_phc:
> > +	kfree(phc);
> > +out:
> > +	return ERR_PTR(err);
> > +}
> 
> Smatch complains that ERR_PTR may be passed zero.
> Looking at the IS_ERR_OR_NULL block above, this does indeed seem to be the
> case.

The intention here had something to do with PTP being optional for the
caller (netdevsim). Not sure whether the implementation is the best -
and in particular whether ERR_PTR(0) is NULL or not. I guess this is
what the smatch warning (which I haven't looked at) is saying.

> Keeping Smatch happy is one thing - your call - but I do wonder if the
> caller of mock_phc_create() handles the NULL case correctly.

mock_phc_create() returns a pointer to an opaque data structure -
struct mock_phc - and the caller just carries that pointer around to the
other API calls exported by the mock_phc module. It doesn't need to care
whether the pointer is NULL or not, just the mock_phc module does (and
it does handle that part well, at least assuming that the pointer is NULL).