| 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: <20240308211842.6d64b5bc@kernel.org>
Date: Fri, 8 Mar 2024 21:18:42 -0800
From: Jakub Kicinski <kuba@...nel.org>
To: Maxime Chevallier <maxime.chevallier@...tlin.com>
Cc: davem@...emloft.net, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org, thomas.petazzoni@...tlin.com, Andrew Lunn
<andrew@...n.ch>, Eric Dumazet <edumazet@...gle.com>, Paolo Abeni
<pabeni@...hat.com>, Russell King <linux@...linux.org.uk>,
linux-arm-kernel@...ts.infradead.org, Christophe Leroy
<christophe.leroy@...roup.eu>, Herve Codina <herve.codina@...tlin.com>,
Florian Fainelli <f.fainelli@...il.com>, Heiner Kallweit
<hkallweit1@...il.com>, Vladimir Oltean <vladimir.oltean@....com>,
Köry Maincent <kory.maincent@...tlin.com>, Jesse Brandeburg
<jesse.brandeburg@...el.com>, Jonathan Corbet <corbet@....net>, Marek
Behún <kabel@...nel.org>, Piergiorgio Beruto
<piergiorgio.beruto@...il.com>, Oleksij Rempel <o.rempel@...gutronix.de>,
Nicolò Veronese <nicveronese@...il.com>, Simon Horman
<horms@...nel.org>, mwojtas@...omium.org
Subject: Re: [PATCH net-next v10 07/13] net: ethtool: Introduce a command to
list PHYs on an interface
On Mon, 4 Mar 2024 16:10:03 +0100 Maxime Chevallier wrote:
> +PHY_GET
> +=======
> +
> +Retrieve information about a given Ethernet PHY sitting on the link. As there
> +can be more than one PHY, the DUMP operation can be used to list the PHYs
> +present on a given interface, by passing an interface index or name in
> +the dump request
Could be worth re-stating the default behavior of the DO request?
That the DO will return the dev->phydev by default and if PHY_INDEX
is specified in the header, the particular PHY with that index?
> + if (phydev->drv && nla_put_string(skb, ETHTOOL_A_PHY_DRVNAME, phydev->drv->name))
could you break the lines at 80 where it doesn't hurt readability?
This way:
if (phydev->drv &&
nla_put_string(skb, ETHTOOL_A_PHY_DRVNAME, phydev->drv->name))
> + return -EMSGSIZE;
> +
> + if (ptype == PHY_UPSTREAM_PHY) {
> + struct phy_device *upstream = pdn->upstream.phydev;
> + const char *sfp_upstream_name;
> +
> + /* Parent index */
> + if (nla_put_u32(skb, ETHTOOL_A_PHY_UPSTREAM_INDEX, upstream->phyindex))
> + return -EMSGSIZE;
> +
> + if (pdn->parent_sfp_bus) {
> + sfp_upstream_name = sfp_get_name(pdn->parent_sfp_bus);
> + if (sfp_upstream_name && nla_put_string(skb,
> + ETHTOOL_A_PHY_UPSTREAM_SFP_NAME,
> + sfp_upstream_name))
ditto
> + return -EMSGSIZE;
> + }
> + }
> +int ethnl_phy_doit(struct sk_buff *skb, struct genl_info *info)
> +{
> + struct phy_req_info req_info = {};
> + struct nlattr **tb = info->attrs;
> + struct sk_buff *rskb;
> + void *reply_payload;
> + int reply_len;
> + int ret;
> +
> + ret = ethnl_parse_header_dev_get(&req_info.base,
> + tb[ETHTOOL_A_PHY_HEADER],
> + genl_info_net(info), info->extack,
> + true);
> + if (ret < 0)
> + return ret;
> +
> + rtnl_lock();
> +
> + ret = ethnl_phy_parse_request(&req_info.base, tb);
> + if (ret < 0)
> + goto err_unlock_rtnl;
> +
> + /* No PHY, return early */
> + if (!req_info.pdn.phy)
> + goto err_unlock_rtnl;
> +
> + ret = ethnl_phy_reply_size(&req_info.base, info->extack);
> + if (ret < 0)
> + goto err_unlock_rtnl;
> + reply_len = ret + ethnl_reply_header_size();
> +
> + rskb = ethnl_reply_init(reply_len, req_info.base.dev,
> + ETHTOOL_MSG_PHY_GET_REPLY,
> + ETHTOOL_A_PHY_HEADER,
> + info, &reply_payload);
> + if (!rskb) {
> + ret = -ENOMEM;
> + goto err_unlock_rtnl;
> + }
> +
> + ret = ethnl_phy_fill_reply(&req_info.base, rskb);
> + if (ret)
> + goto err_free_msg;
> +
> + rtnl_unlock();
> + ethnl_parse_header_dev_put(&req_info.base);
> + genlmsg_end(rskb, reply_payload);
> +
> + return genlmsg_reply(rskb, info);
> +
> +err_free_msg:
> + nlmsg_free(rskb);
> +err_unlock_rtnl:
> + rtnl_unlock();
> + ethnl_parse_header_dev_put(&req_info.base);
> + return ret;
> +}
> +
> +struct ethnl_phy_dump_ctx {
> + struct phy_req_info *phy_req_info;
> + unsigned long ifindex;
> + unsigned long phy_index;
> +};
> +
> +int ethnl_phy_start(struct netlink_callback *cb)
> +{
> + const struct genl_dumpit_info *info = genl_dumpit_info(cb);
You can save some chars by using genl_info_dump() here
> + struct ethnl_phy_dump_ctx *ctx = (void *)cb->ctx;
> + struct nlattr **tb = info->info.attrs;
then you can ditch this
> + int ret;
> +
> + BUILD_BUG_ON(sizeof(*ctx) > sizeof(cb->ctx));
> +
> + ctx->phy_req_info = kzalloc(sizeof(*ctx->phy_req_info), GFP_KERNEL);
> + if (!ctx->phy_req_info)
> + return -ENOMEM;
> +
> + ret = ethnl_parse_header_dev_get(&ctx->phy_req_info->base,
> + tb[ETHTOOL_A_PHY_HEADER],
and:
info->attrs[ETHTOOL_A_PHY_HEADER],
> + sock_net(cb->skb->sk), cb->extack,
> + false);
leaking ctx->phy_req_info on error?
> + ctx->ifindex = 0;
> + ctx->phy_index = 0;
> + return ret;
> +}
> +static int ethnl_phy_dump_one_dev(struct sk_buff *skb, struct net_device *dev,
> + struct netlink_callback *cb)
> +{
> + struct ethnl_phy_dump_ctx *ctx = (void *)cb->ctx;
> + struct phy_req_info *pri = ctx->phy_req_info;
> + struct phy_device_node *pdn;
> + int ret = 0;
> + void *ehdr;
> +
> + pri->base.dev = dev;
> +
> + xa_for_each_start(&dev->link_topo->phys, ctx->phy_index, pdn, ctx->phy_index) {
> + ehdr = ethnl_dump_put(skb, cb,
> + ETHTOOL_MSG_PHY_GET_REPLY);
this one OTOH fits on a line :)
> + if (!ehdr) {
> + ret = -EMSGSIZE;
> + break;
> + }
> +
> + ret = ethnl_fill_reply_header(skb, dev,
> + ETHTOOL_A_PHY_HEADER);
ditto
> + if (ret < 0) {
> + genlmsg_cancel(skb, ehdr);
> + break;
> + }
> +
> + memcpy(&pri->pdn, pdn, sizeof(*pdn));
> + ret = ethnl_phy_fill_reply(&pri->base, skb);
On a DO fill() shouldn't fail, because we size the skb, but if we pack
many entries into the skb on a DUMP it can. So:
if (ret < 0) {
genlmsg_cancel(skb, ehdr);
break;
}
no?
> + genlmsg_end(skb, ehdr);
> + }
> +
> + return ret;
> +}
> +
> +int ethnl_phy_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
> +{
> + struct ethnl_phy_dump_ctx *ctx = (void *)cb->ctx;
> + struct net *net = sock_net(skb->sk);
> + struct net_device *dev;
> + int ret = 0;
> +
> + rtnl_lock();
> +
> + if (ctx->phy_req_info->base.dev) {
> + ret = ethnl_phy_dump_one_dev(skb, ctx->phy_req_info->base.dev, cb);
> + } else {
> + for_each_netdev_dump(net, dev, ctx->ifindex) {
> + ret = ethnl_phy_dump_one_dev(skb, dev, cb);
> + if (ret)
> + break;
> +
> + ctx->phy_index = 0;
> + }
> + }
> + rtnl_unlock();
> +
> + if (ret == -EMSGSIZE && skb->len)
> + return skb->len;
you can remove this if thanks to (very recent) commit b5a899154aa9
> + return ret;
> +}
Very sorry for the late review, BTW :(
Powered by blists - more mailing lists