| Message ID | 20201001183016.1259870-10-kuba@kernel.org |
|---|---|
| State | Superseded |
| Delegated to: | David Miller |
| Headers | show |
| Series | genetlink: support per-command policy dump | expand |
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
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 --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)
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(-)