netdev - Re: [RFC net-next 9/9] genetlink: allow dumping command-specific 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] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 01 Oct 2020 09:59:47 +0200
From:   Johannes Berg <johannes@...solutions.net>
To:     Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org
Cc:     andrew@...n.ch, jiri@...nulli.us, mkubecek@...e.cz,
        dsahern@...nel.org, pablo@...filter.org
Subject: Re: [RFC net-next 9/9] genetlink: allow dumping command-specific
 policy

On Wed, 2020-09-30 at 17:05 -0700, Jakub Kicinski wrote:
> Right now CTRL_CMD_GETPOLICY can only dump the family-wide
> policy. Support dumping policy of a specific op.

So, hmm.

Yeah, I guess this is fine, but you could end up having to do a lot of
dumps, and with e.g. ethtool you'd end up with a lot of duplicate data
too, since it's structured as


common_policy = { ... }

cmd1_policy = {
	[CMD1_COMMON] = NLA_NESTED_POLICY(common_policy),
	...
};

cmd2_policy = {
	[CMD2_COMMON] = NLA_NESTED_POLICY(common_policy),
	...
};

etc.


So you end up dumping per command (and in practice, since they can be
different, you now *have to* unless you know out-of-band that there are
no per-command policies).


Even if I don't have a good idea now on how to avoid the duplication, it
might be nicer to have a (flag) attribute here for "CTRL_ATTR_ALL_OPS"?

johannes

> Signed-off-by: Jakub Kicinski <kuba@...nel.org>
> ---
>  include/uapi/linux/genetlink.h |  1 +
>  net/netlink/genetlink.c        | 23 +++++++++++++++++++++--
>  2 files changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/include/uapi/linux/genetlink.h b/include/uapi/linux/genetlink.h
> index 9c0636ec2286..7dbe2d5d7d46 100644
> --- a/include/uapi/linux/genetlink.h
> +++ b/include/uapi/linux/genetlink.h
> @@ -64,6 +64,7 @@ enum {
>  	CTRL_ATTR_OPS,
>  	CTRL_ATTR_MCAST_GROUPS,
>  	CTRL_ATTR_POLICY,
> +	CTRL_ATTR_OP,
>  	__CTRL_ATTR_MAX,
>  };
>  
> diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
> index f2833e9165c7..12e9f323af35 100644
> --- a/net/netlink/genetlink.c
> +++ b/net/netlink/genetlink.c
> @@ -1113,12 +1113,14 @@ static int genl_ctrl_event(int event, const struct genl_family *family,
>  struct ctrl_dump_policy_ctx {
>  	unsigned long state;
>  	unsigned int fam_id;
> +	u8 cmd;
>  };
>  
>  static const struct nla_policy ctrl_policy_policy[] = {
>  	[CTRL_ATTR_FAMILY_ID]	= { .type = NLA_U16 },
>  	[CTRL_ATTR_FAMILY_NAME]	= { .type = NLA_NUL_STRING,
>  				    .len = GENL_NAMSIZ - 1 },
> +	[CTRL_ATTR_OP]		= { .type = NLA_U8 },
>  };
>  
>  static int ctrl_dumppolicy_start(struct netlink_callback *cb)
> @@ -1127,6 +1129,8 @@ static int ctrl_dumppolicy_start(struct netlink_callback *cb)
>  	struct ctrl_dump_policy_ctx *ctx = (void *)cb->args;
>  	struct nlattr **tb = info->attrs;
>  	const struct genl_family *rt;
> +	struct genl_ops op;
> +	int err;
>  
>  	if (!tb[CTRL_ATTR_FAMILY_ID] && !tb[CTRL_ATTR_FAMILY_NAME])
>  		return -EINVAL;
> @@ -1145,10 +1149,23 @@ static int ctrl_dumppolicy_start(struct netlink_callback *cb)
>  	if (!rt)
>  		return -ENOENT;
>  
> -	if (!rt->policy)
> +	if (tb[CTRL_ATTR_OP]) {
> +		ctx->cmd = nla_get_u8(tb[CTRL_ATTR_OP]);
> +
> +		err = genl_get_cmd(ctx->cmd, rt, &op);
> +		if (err) {
> +			NL_SET_BAD_ATTR(cb->extack, tb[CTRL_ATTR_OP]);
> +			return err;
> +		}
> +	} else {
> +		op.policy = rt->policy;
> +		op.maxattr = rt->maxattr;
> +	}
> +
> +	if (!op.policy)
>  		return -ENODATA;
>  
> -	return netlink_policy_dump_start(rt->policy, rt->maxattr, &ctx->state);
> +	return netlink_policy_dump_start(op.policy, op.maxattr, &ctx->state);
>  }
>  
>  static int ctrl_dumppolicy(struct sk_buff *skb, struct netlink_callback *cb)
> @@ -1167,6 +1184,8 @@ static int ctrl_dumppolicy(struct sk_buff *skb, struct netlink_callback *cb)
>  
>  		if (nla_put_u16(skb, CTRL_ATTR_FAMILY_ID, ctx->fam_id))
>  			goto nla_put_failure;
> +		if (ctx->cmd && nla_put_u8(skb, CTRL_ATTR_OP, ctx->cmd))
> +			goto nla_put_failure;
>  
>  		nest = nla_nest_start(skb, CTRL_ATTR_POLICY);
>  		if (!nest)