netdev - Re: [PATCH net-next WIP] ethtool: generic netlink policy

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] [day] [month] [year] [list]

Message-ID: <1460455274.6369.24.camel@sipsolutions.net>
Date:	Tue, 12 Apr 2016 12:01:14 +0200
From:	Johannes Berg <johannes@...solutions.net>
To:	Roopa Prabhu <roopa@...ulusnetworks.com>, netdev@...r.kernel.org
Cc:	davem@...emloft.net, jiri@...nulli.us, eladr@...lanox.com,
	idosch@...lanox.com
Subject: Re: [PATCH net-next WIP] ethtool: generic netlink policy

Hi,

> +	[ETHTOOL_ATTR_FLAGS]		= { .type = NLA_U32 },

I suppose this comes from the current API, but perhaps it'd be
worthwhile to make provision for more flags? Perhaps even using
NLA_BINARY and have "infinitely extensible" flags.

> +	[ETHTOOL_ATTR_SSET_COUNT]		= { .type = NLA_U32

What do you need that for? Wouldn't it be sufficient to count the SSET
values returned? I can see how this would be useful for ioctl, but not
really for netlink messages?

> +static struct genl_ops ethtool_ops[] = {
> +	{
> +		.cmd = ETHTOOL_CMD_GET_SETTINGS,
> +		.policy = ethtool_policy,
> +		.doit = ethtool_get_settings,
> +	},
[...]
> +	{
> +		.cmd = ETHTOOL_CMD_SET_MODULE_INFO,
> +		.policy = ethtool_policy,
> +		.doit = ethtool_set_module_info,
> +	},
> +};

Shouldn't the ops have GENL_ADMIN_PERM as flags?

> +int ethtool_get_settings(struct net_device *dev, struct ethtool_cmd
> *cmd)
> +{
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(ethtool_get_settings);

I don't understand what kind of placeholder this was meant to be - but
why would it be exported? This part is called by the genl ops, so
doesn't really make sense?

It seems that instead these functions should be static, declared above
the ops, and call into the existing ethtool driver ops, based on
IFINDEX demultiplexing.

> +void ethtool_get_ethtool_stats(struct net_device *dev,
> +			       struct ethtool_stats *stats,
> +			       u64 *data)
> +{
> +
> +	/* example the driver handler would do the below
> +	 *
> +	err = nla_put_u32(msg, PORT_ATTR_IFINDEX, ifindex);
> +	if (err < 0)
> +		goto err_out;
> +
> +	err = nla_put_u32(msg, PORT_ATTR_FLAGS, flags);
> +	if (err < 0)
> +		goto err_out;
> +
> +	err = nla_put_u32(msg, PORT_ATTR_SSET_COUNT,
> +			  count);
> +	if (err < 0)
> +		goto err_out;
> +
> +	nest = nla_nest_start(msg, PORT_ATTR_STATS);
> +	for (i = 0; i < count; i++)
> +		nla_put_u64(msg, PORT_ATTR_STAT, data[i]);
> +			    nla_nest_end(msg, nest);
> +
> +	 */
> +}
> +EXPORT_SYMBOL_GPL(ethtool_get_ethtool_stats);

It seems possible that you could have a lot of ports, or a lot of
strings, or similar, so I think this should be a dumpit instead of a
doit handler.

Similar, perhaps, for the EEPROM thing, unless you provide API to query
the size first so the application can size it's recvmsg() buffer
appropriately - however doing so also requires a big message allocation
and more code in userspace, so I think having an "offset/length" type
of API combined with a dumpit rather than doit would be good for all of
the things that could get bigger or that might be extended in the
future.

johannes