netdev - Re: [PATCH net-next 1/6] ethtool: wire up get policies to ops

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: <631a2328a95d0dd06d901cdb411c3eb06f90bda7.camel@sipsolutions.net>
Date:   Mon, 05 Oct 2020 20:56:29 +0200
From:   Johannes Berg <johannes@...solutions.net>
To:     Jakub Kicinski <kuba@...nel.org>, davem@...emloft.net
Cc:     netdev@...r.kernel.org, kernel-team@...com, jiri@...nulli.us,
        andrew@...n.ch, mkubecek@...e.cz
Subject: Re: [PATCH net-next 1/6] ethtool: wire up get policies to ops

On Mon, 2020-10-05 at 08:57 -0700, Jakub Kicinski wrote:
> 
> @@ -783,6 +799,9 @@ static const struct genl_ops ethtool_genl_ops[] = {
>  		.start	= ethnl_default_start,
>  		.dumpit	= ethnl_default_dumpit,
>  		.done	= ethnl_default_done,
> +		.policy = ethnl_rings_get_policy,
> +		.maxattr = ARRAY_SIZE(ethnl_rings_get_policy) - 1,
> +
>  	},

If you find some other reason to respin, perhaps remove that blank line
:)

Unrelated to that, it bothers me a bit that you put here the maxattr as
the ARRAY_SIZE(), which is of course fine, but then still have

> @@ -127,7 +127,7 @@ const struct ethnl_request_ops ethnl_privflags_request_ops = {
>  	.max_attr		= ETHTOOL_A_PRIVFLAGS_MAX,

max_attr here, using the original define - yes, mostly the policies use
the define to size them, but they didn't really *need* to, and one might
make an argument that on the policy arrays the size might as well be
removed (and it be sized automatically based on the contents) since all
the unspecified attrs are rejected anyway.

But with the difference it seems to me that it'd be possible to get this
mixed up?

I do see that you still need this to size the attrs for parsing them
even after patch 2 where this:

>  	.req_info_size		= sizeof(struct privflags_req_info),
>  	.reply_data_size	= sizeof(struct privflags_reply_data),
> -	.request_policy		= privflags_get_policy,
> +	.request_policy		= ethnl_privflags_get_policy,

gets removed completely.

Perhaps we can look up the genl_ops pointer, or add the ops pointer to
struct genl_info (could point to the temporary full struct that gets
populated, size of genl_info itself doesn't matter much since it's on
the stack and temporary), and then use ops->maxattr instead of
request_ops->max_attr in ethnl_default_parse()?

johannes