linux-kernel - Re: [PATCH net-next v6 09/15] ethtool: generic handlers for GET requests

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: <20190703142510.GA2250@nanopsycho>
Date:   Wed, 3 Jul 2019 16:25:10 +0200
From:   Jiri Pirko <jiri@...nulli.us>
To:     Michal Kubecek <mkubecek@...e.cz>
Cc:     David Miller <davem@...emloft.net>, netdev@...r.kernel.org,
        Jakub Kicinski <jakub.kicinski@...ronome.com>,
        Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        John Linville <linville@...driver.com>,
        Stephen Hemminger <stephen@...workplumber.org>,
        Johannes Berg <johannes@...solutions.net>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v6 09/15] ethtool: generic handlers for GET
 requests

Tue, Jul 02, 2019 at 01:50:24PM CEST, mkubecek@...e.cz wrote:

[...]	
	
>+/* generic ->doit() handler for GET type requests */
>+static int ethnl_get_doit(struct sk_buff *skb, struct genl_info *info)

It is very unfortunate for review to introduce function in a patch and
don't use it. In general, this approach is frowned upon. You should use
whatever you introduce in the same patch. I understand it is sometimes
hard.

IIUC, you have one ethnl_get_doit for all possible commands, and you
have this ops to do cmd-specific tasks. That is quite unusual. Plus if
you consider the complicated datastructures connected with this, 
I'm lost from the beginning :( Any particular reason form this indirection?
I don't think any other generic netlink code does that (correct me if
I'm wrong). The nice thing about generic netlink is the fact that
you have separate handlers per cmd.

I don't think you need these ops and indirections. For the common parts,
just have a set of common helpers, as the other generic netlink users
are doing. The code would be much easier to read and follow then.


>+{
>+	const u8 cmd = info->genlhdr->cmd;
>+	const struct get_request_ops *ops;
>+	struct ethnl_req_info *req_info;
>+	struct sk_buff *rskb;
>+	void *reply_payload;
>+	int reply_len;
>+	int ret;
>+
>+	ops = get_requests[cmd];
>+	if (WARN_ONCE(!ops, "cmd %u has no get_request_ops\n", cmd))
>+		return -EOPNOTSUPP;
>+	req_info = ethnl_alloc_get_data(ops);
>+	if (!req_info)
>+		return -ENOMEM;
>+	ret = ethnl_std_parse(req_info, info->nlhdr, genl_info_net(info), ops,
>+			      info->extack, !ops->allow_nodev_do);
>+	if (ret < 0)
>+		goto err_dev;
>+	req_info->privileged = ethnl_is_privileged(skb);
>+	ethnl_init_reply_data(req_info, ops, req_info->dev);
>+
>+	rtnl_lock();
>+	ret = ops->prepare_data(req_info, info);
>+	if (ret < 0)
>+		goto err_rtnl;
>+	reply_len = ops->reply_size(req_info);
>+	if (ret < 0)
>+		goto err_cleanup;
>+	ret = -ENOMEM;
>+	rskb = ethnl_reply_init(reply_len, req_info->dev, ops->reply_cmd,
>+				ops->hdr_attr, info, &reply_payload);
>+	if (!rskb)
>+		goto err_cleanup;
>+	ret = ops->fill_reply(rskb, req_info);
>+	if (ret < 0)
>+		goto err_msg;
>+	rtnl_unlock();
>+
>+	genlmsg_end(rskb, reply_payload);
>+	if (req_info->dev)
>+		dev_put(req_info->dev);
>+	ethnl_free_get_data(ops, req_info);
>+	return genlmsg_reply(rskb, info);
>+
>+err_msg:
>+	WARN_ONCE(ret == -EMSGSIZE,
>+		  "calculated message payload length (%d) not sufficient\n",
>+		  reply_len);
>+	nlmsg_free(rskb);
>+err_cleanup:
>+	ethnl_free_get_data(ops, req_info);
>+err_rtnl:
>+	rtnl_unlock();
>+err_dev:
>+	if (req_info->dev)
>+		dev_put(req_info->dev);
>+	return ret;
>+}

[...]