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: <20240305175253.764f041a@kmaincent-XPS-13-7390>
Date: Tue, 5 Mar 2024 17:52:53 +0100
From: Köry Maincent <kory.maincent@...tlin.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: Florian Fainelli <florian.fainelli@...adcom.com>, Broadcom internal
 kernel review list <bcm-kernel-feedback-list@...adcom.com>, Andrew Lunn
 <andrew@...n.ch>, Heiner Kallweit <hkallweit1@...il.com>, Russell King
 <linux@...linux.org.uk>, "David S. Miller" <davem@...emloft.net>, Eric
 Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>, Richard
 Cochran <richardcochran@...il.com>, Radu Pirea
 <radu-nicolae.pirea@....nxp.com>, Jay Vosburgh <j.vosburgh@...il.com>, Andy
 Gospodarek <andy@...yhouse.net>, Nicolas Ferre
 <nicolas.ferre@...rochip.com>, Claudiu Beznea <claudiu.beznea@...on.dev>,
 Willem de Bruijn <willemdebruijn.kernel@...il.com>, Jonathan Corbet
 <corbet@....net>, Horatiu Vultur <horatiu.vultur@...rochip.com>,
 UNGLinuxDriver@...rochip.com, Simon Horman <horms@...nel.org>, Vladimir
 Oltean <vladimir.oltean@....com>, Thomas Petazzoni
 <thomas.petazzoni@...tlin.com>, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org, Maxime Chevallier
 <maxime.chevallier@...tlin.com>, Rahul Rameshbabu <rrameshbabu@...dia.com>
Subject: Re: [PATCH net-next v9 12/13] net: ethtool: tsinfo: Add support for
 hwtstamp provider and get/set hwtstamp config

On Mon, 4 Mar 2024 19:27:33 -0800
Jakub Kicinski <kuba@...nel.org> wrote:

> On Mon, 26 Feb 2024 14:40:03 +0100 Kory Maincent wrote:
> > diff --git a/net/ethtool/common.c b/net/ethtool/common.c
> > index b3f45c307301..37071929128a 100644
> > --- a/net/ethtool/common.c
> > +++ b/net/ethtool/common.c
> > @@ -426,6 +426,7 @@ const char sof_timestamping_names[][ETH_GSTRING_LEN] = {
> >  	[const_ilog2(SOF_TIMESTAMPING_OPT_TX_SWHW)]  = "option-tx-swhw",
> >  	[const_ilog2(SOF_TIMESTAMPING_BIND_PHC)]     = "bind-phc",
> >  	[const_ilog2(SOF_TIMESTAMPING_OPT_ID_TCP)]   = "option-id-tcp",
> > +	[const_ilog2(SOF_TIMESTAMPING_GHWTSTAMP)]    = "get-hwtstamp",  
> 
> What is this new SOF_TIMESTAMPING_GHWTSTAMP? If there's 
> a good reason for it to exist it should be documented in
> Documentation/networking/timestamping.rst

/o\ Sorry I totally forgot about documentation here!

> > +const struct nla_policy ethnl_tsinfo_get_policy[ETHTOOL_A_TSINFO_MAX + 1]
> > = { [ETHTOOL_A_TSINFO_HEADER]		=
> >  		NLA_POLICY_NESTED(ethnl_header_policy),
> > +	[ETHTOOL_A_TSINFO_TIMESTAMPING] = { .type = NLA_NESTED },
> > +	[ETHTOOL_A_TSINFO_HWTSTAMP_PROVIDER_NEST] = { .type = NLA_NESTED
> > },  
> 
> link the policy by NLA_POLICY_NESTED() so that user space can inspect
> the sub-layers via the control family.

Ok thanks!

> > +
> > +	if (!hwtst_tb[ETHTOOL_A_TSINFO_HWTSTAMP_PROVIDER_INDEX] ||
> > +	    !hwtst_tb[ETHTOOL_A_TSINFO_HWTSTAMP_PROVIDER_QUALIFIER])
> > +		return -EINVAL;  
> 
> NL_REQ_ATTR_CHECK()

ok.

> 
> > +	ret =
> > nla_get_u32(hwtst_tb[ETHTOOL_A_TSINFO_HWTSTAMP_PROVIDER_INDEX]);
> > +	if (ret < 0)
> > +		return -EINVAL;  
> 
> How's the get_u32 going to return a negative value?
> That's the purpose of this check?
> The policy should contain the max expected value - NLA_POLICY_MAX().

Right I will use more NLA_POLICY_* to check the values in next version.

> >  		return ret;
> > -	ret = __ethtool_get_ts_info(dev, &data->ts_info);
> > +
> > +	if (!netif_device_present(dev)) {  
> 
>  ethnl_ops_begin() checks for presence

Ok thanks!

> 
> > +	if (req->hwtst.index != -1) {
> > +		struct hwtstamp_provider hwtstamp;
> > +
> > +		hwtstamp.ptp = ptp_clock_get_by_index(req->hwtst.index);
> > +		if (!hwtstamp.ptp) {
> > +			ret = -ENODEV;
> > +			goto out;
> > +		}
> > +		hwtstamp.qualifier = req->hwtst.qualifier;
> > +
> > +		ret = ethtool_get_ts_info_by_phc(dev, &data->ts_info,
> > +						 &hwtstamp);
> > +	} else {
> > +		ret = __ethtool_get_ts_info(dev, &data->ts_info);  
> 
> Not sure I grok why we need 3 forms of getting the tstamp config.
> 
> Please make sure to always update
> Documentation/networking/ethtool-netlink.rst
> when extending ethtool-nl.

Yes sorry I forgot!
The three cases are:
- get hwtstamp config like ioctl SIOCGHWTSTAMP
- get tsinfo of the current hwtstamp
- get tsinfo of a specific hwtstamp

> > +	if (ts_info->phc_index >= 0) {
> > +		/* _TSINFO_HWTSTAMP_PROVIDER_NEST */
> > +		len += nla_total_size(sizeof(u32) * 2);  
> 
> That translates to two raw u32s into a single attribute.
> Is that what you mean?

Oh right that's not what I want. Thanks you!
This is better:
len += 2 * nla_total_size(sizeof(u32));

> > +	if (ts_info->phc_index >= 0) {
> > +		ret = nla_put_u32(skb, ETHTOOL_A_TSINFO_PHC_INDEX,
> > +				  ts_info->phc_index);
> > +		if (ret)
> > +			return -EMSGSIZE;
> > +
> > +		nest = nla_nest_start(skb,
> > ETHTOOL_A_TSINFO_HWTSTAMP_PROVIDER_NEST);
> > +		if (!nest)
> > +			return -EMSGSIZE;
> > +
> > +		ret = nla_put_u32(skb,
> > +				  ETHTOOL_A_TSINFO_HWTSTAMP_PROVIDER_INDEX,
> > +				  ts_info->phc_index);  
> 
> You can assume nla_put_u32 only returns EMSGSIZE, so doing:
> 
> if (nla_put_u32(....) ||
>     nla_put_u32(....))
> 	return -EMSGSIZE;
> 
> is generally considered to be fine.

Ok.

> > +
> > +		/* Does the hwtstamp supported in the netdev topology */
> > +		if (mod) {
> > +			hwtstamp.ptp = ptp_clock_get_by_index(phc_index);  
> 
> This just returns a pointer without any refcounting, right?
> What guarantees the ptp object doesn't disappear?

Could the ptp object disappears within rtnlock?
Maybe I should add refcounting.

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ