netdev - Re: [PATCH net-next v2 10/10] ethtool: netlink: always pass genl_info to .prepare

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: Fri, 11 Aug 2023 08:42:45 +0200
From: Jiri Pirko <jiri@...nulli.us>
To: Jakub Kicinski <kuba@...nel.org>
Cc: davem@...emloft.net, netdev@...r.kernel.org, edumazet@...gle.com,
	pabeni@...hat.com, johannes@...solutions.net,
	Vladimir Oltean <vladimir.oltean@....com>, gal@...dia.com,
	tariqt@...dia.com, lucien.xin@...il.com, f.fainelli@...il.com,
	andrew@...n.ch, simon.horman@...igine.com, linux@...pel-privat.de,
	mkubecek@...e.cz
Subject: Re: [PATCH net-next v2 10/10] ethtool: netlink: always pass
 genl_info to .prepare_data

Fri, Aug 11, 2023 at 01:38:45AM CEST, kuba@...nel.org wrote:

[...]


>diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
>index f7b3171a0aad..3bbd5afb7b31 100644
>--- a/net/ethtool/netlink.c
>+++ b/net/ethtool/netlink.c
>@@ -444,12 +444,12 @@ static int ethnl_default_doit(struct sk_buff *skb, struct genl_info *info)
> 
> static int ethnl_default_dump_one(struct sk_buff *skb, struct net_device *dev,
> 				  const struct ethnl_dump_ctx *ctx,
>-				  struct netlink_callback *cb)
>+				  const struct genl_info *info)
> {
> 	void *ehdr;
> 	int ret;
> 
>-	ehdr = genlmsg_put(skb, NETLINK_CB(cb->skb).portid, cb->nlh->nlmsg_seq,
>+	ehdr = genlmsg_put(skb, info->snd_portid, info->snd_seq,
> 			   &ethtool_genl_family, NLM_F_MULTI,
> 			   ctx->ops->reply_cmd);
> 	if (!ehdr)
>@@ -457,7 +457,7 @@ static int ethnl_default_dump_one(struct sk_buff *skb, struct net_device *dev,
> 
> 	ethnl_init_reply_data(ctx->reply_data, ctx->ops, dev);
> 	rtnl_lock();
>-	ret = ctx->ops->prepare_data(ctx->req_info, ctx->reply_data, NULL);
>+	ret = ctx->ops->prepare_data(ctx->req_info, ctx->reply_data, info);
> 	rtnl_unlock();
> 	if (ret < 0)
> 		goto out;
>@@ -495,7 +495,7 @@ static int ethnl_default_dumpit(struct sk_buff *skb,
> 		dev_hold(dev);
> 		rtnl_unlock();
> 
>-		ret = ethnl_default_dump_one(skb, dev, ctx, cb);
>+		ret = ethnl_default_dump_one(skb, dev, ctx, genl_info_dump(cb));
> 
> 		rtnl_lock();
> 		dev_put(dev);
>@@ -647,11 +647,14 @@ static void ethnl_default_notify(struct net_device *dev, unsigned int cmd,
> 	struct ethnl_reply_data *reply_data;
> 	const struct ethnl_request_ops *ops;
> 	struct ethnl_req_info *req_info;
>+	struct genl_info info;
> 	struct sk_buff *skb;
> 	void *reply_payload;
> 	int reply_len;
> 	int ret;
> 
>+	genl_info_init_ntf(&info, &ethtool_genl_family, cmd);
>+
> 	if (WARN_ONCE(cmd > ETHTOOL_MSG_KERNEL_MAX ||
> 		      !ethnl_default_notify_ops[cmd],
> 		      "unexpected notification type %u\n", cmd))
>@@ -670,7 +673,7 @@ static void ethnl_default_notify(struct net_device *dev, unsigned int cmd,
> 	req_info->flags |= ETHTOOL_FLAG_COMPACT_BITSETS;
> 
> 	ethnl_init_reply_data(reply_data, ops, dev);
>-	ret = ops->prepare_data(req_info, reply_data, NULL);
>+	ret = ops->prepare_data(req_info, reply_data, &info);
> 	if (ret < 0)
> 		goto err_cleanup;
> 	ret = ops->reply_size(req_info, reply_data);


[...]


>@@ -24,7 +24,7 @@ const struct nla_policy ethnl_wol_get_policy[] = {
> 
> static int wol_prepare_data(const struct ethnl_req_info *req_base,
> 			    struct ethnl_reply_data *reply_base,
>-			    struct genl_info *info)
>+			    const struct genl_info *info)
> {
> 	struct wol_reply_data *data = WOL_REPDATA(reply_base);
> 	struct net_device *dev = reply_base->dev;
>@@ -39,7 +39,8 @@ static int wol_prepare_data(const struct ethnl_req_info *req_base,
> 	dev->ethtool_ops->get_wol(dev, &data->wol);
> 	ethnl_ops_complete(dev);
> 	/* do not include password in notifications */
>-	data->show_sopass = info && (data->wol.supported & WAKE_MAGICSECURE);
>+	data->show_sopass = genl_info_is_ntf(info) &&
>+		(data->wol.supported & WAKE_MAGICSECURE);

I believe that you are missing "!" here:
	data->show_sopass = !genl_info_is_ntf(info) &&
		(data->wol.supported & WAKE_MAGICSECURE);

But, you are changing the output for dumpit if I'm not mistaken.
ethnl_default_dump_one() currently calls this with info==NULL too, not
only ethnl_default_notify().

Anyway, the genl_info_is_ntf() itself seems a bit odd to me. The only
user is here and I doubt there ever going to be any other. This
conditional per-op attr fill seems a bit odd.

Can't you handle this in side ethtool somehow? IDK :/


> 
> 	return 0;
> }
>-- 
>2.41.0
>