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