diff mbox series

[net-next,9/9] genetlink: allow dumping command-specific policy

Message ID 20201001183016.1259870-10-kuba@kernel.org
State New
Headers show
Series genetlink: support per-command policy dump | expand

Commit Message

Jakub Kicinski Oct. 1, 2020, 6:30 p.m. UTC
Right now CTRL_CMD_GETPOLICY can only dump the family-wide
policy. Support dumping policy of a specific op.

v1:
 - don't echo op in the output in a naive way, this should
   make it cleaner to extend the output format for dumping
   policies for all the commands at once in the future.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 include/uapi/linux/genetlink.h |  1 +
 net/netlink/genetlink.c        | 18 ++++++++++++++++--
 2 files changed, 17 insertions(+), 2 deletions(-)

Comments

Johannes Berg Oct. 1, 2020, 9:03 p.m. UTC | #1
On Thu, 2020-10-01 at 11:30 -0700, Jakub Kicinski wrote:
> 
> +++ b/net/netlink/genetlink.c
> @@ -1119,6 +1119,7 @@ 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 },

I slightly wonder if we shouldn't make this wider. There's no real
*benefit* to using a u8 since, due to padding, the attribute actually
has the same size anyway (up to U32), and we also still need to validate
it actually exists.

However, if we ever run out of command numbers in some family (nl80211
is almost half way :-) ) then I believe we still have some reserved
space in the genl header that we could use for extensions, but if then
we have to also go around and change a bunch of other interfaces like
this, it'll just be so much harder, and ... we'd probably just be
tempted to multiplex onto an "extension command" or something? Or maybe
then we should have a separate genl family at that point?

(Internal interfaces are much easier to change)

Dunno. Just a thought. We probably won't ever get there, it just sort of
rubs me the wrong way that we'd be restricting this in an "unfixable"
API for no real reason :)

> -	if (!rt->policy)
> +	if (tb[CTRL_ATTR_OP]) {
> +		err = genl_get_cmd(nla_get_u8(tb[CTRL_ATTR_OP]), rt, &op);

OK, so maybe if we make that wider we also have to make the argument to
genl_get_cmd() wider so we don't erroneously return op 1 if you ask for
257, but that's in an at least 32-bit register anyway, presumably?

johannes
Jakub Kicinski Oct. 1, 2020, 9:11 p.m. UTC | #2
On Thu, 01 Oct 2020 23:03:01 +0200 Johannes Berg wrote:
> On Thu, 2020-10-01 at 11:30 -0700, Jakub Kicinski wrote:
> > 
> > +++ b/net/netlink/genetlink.c
> > @@ -1119,6 +1119,7 @@ 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 },  
> 
> I slightly wonder if we shouldn't make this wider. There's no real
> *benefit* to using a u8 since, due to padding, the attribute actually
> has the same size anyway (up to U32), and we also still need to validate
> it actually exists.
> 
> However, if we ever run out of command numbers in some family (nl80211
> is almost half way :-) ) then I believe we still have some reserved
> space in the genl header that we could use for extensions, but if then
> we have to also go around and change a bunch of other interfaces like
> this, it'll just be so much harder, and ... we'd probably just be
> tempted to multiplex onto an "extension command" or something? Or maybe
> then we should have a separate genl family at that point?
> 
> (Internal interfaces are much easier to change)
> 
> Dunno. Just a thought. We probably won't ever get there, it just sort of
> rubs me the wrong way that we'd be restricting this in an "unfixable"
> API for no real reason :)

Makes sense, I'll make it a u32. Gotta respin, anyway.

> > -	if (!rt->policy)
> > +	if (tb[CTRL_ATTR_OP]) {
> > +		err = genl_get_cmd(nla_get_u8(tb[CTRL_ATTR_OP]), rt, &op);  
> 
> OK, so maybe if we make that wider we also have to make the argument to
> genl_get_cmd() wider so we don't erroneously return op 1 if you ask for
> 257, but that's in an at least 32-bit register anyway, presumably?

Ack.
diff mbox series

Patch

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 81f0b960e9f8..bc5a6fd60abc 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -1119,6 +1119,7 @@  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 +1128,8 @@  static int ctrl_dumppolicy_start(struct netlink_callback *cb)
 	struct ctrl_dump_policy_ctx *ctx = (void *)cb->ctx;
 	struct nlattr **tb = info->attrs;
 	const struct genl_family *rt;
+	struct genl_ops op;
+	int err;
 
 	BUILD_BUG_ON(sizeof(*ctx) > sizeof(cb->ctx));
 
@@ -1147,10 +1150,21 @@  static int ctrl_dumppolicy_start(struct netlink_callback *cb)
 	if (!rt)
 		return -ENOENT;
 
-	if (!rt->policy)
+	if (tb[CTRL_ATTR_OP]) {
+		err = genl_get_cmd(nla_get_u8(tb[CTRL_ATTR_OP]), 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)