diff mbox series

[net-next,8/9] genetlink: use per-op policy for CTRL_CMD_GETPOLICY

Message ID 20201001183016.1259870-9-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 
Wire up per-op policy for CTRL_CMD_GETPOLICY.
This saves us a call to genlmsg_parse() and will soon allow
dumping this policy.

Create a new policy definition, since we don't want to pollute
ctrl_policy with attributes which CTRL_CMD_GETFAMILY does not
support.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Reviewed-by: Johannes Berg <johannes@sipsolutions.net>
---
 net/netlink/genetlink.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

Comments

Johannes Berg Oct. 1, 2020, 8:55 p.m. UTC | #1 
On Thu, 2020-10-01 at 11:30 -0700, Jakub Kicinski wrote:
> Wire up per-op policy for CTRL_CMD_GETPOLICY.
> This saves us a call to genlmsg_parse() and will soon allow
> dumping this policy.

Hmm. Probably should've asked this before - I think the code makes
perfect sense, but I'm not sure how "this" follows?

I mean, we could've saved the genlmsg_parse() call before, with much the
same patch, having the per-op policy doesn't really have any bearing for
that? It was just using a different policy - the family one - instead of
the per-op one, but ...

Am I missing something?

johannes
Jakub Kicinski Oct. 1, 2020, 9:09 p.m. UTC | #2 
On Thu, 01 Oct 2020 22:55:09 +0200 Johannes Berg wrote:
> On Thu, 2020-10-01 at 11:30 -0700, Jakub Kicinski wrote:
> > Wire up per-op policy for CTRL_CMD_GETPOLICY.
> > This saves us a call to genlmsg_parse() and will soon allow
> > dumping this policy.  
> 
> Hmm. Probably should've asked this before - I think the code makes
> perfect sense, but I'm not sure how "this" follows?
> 
> I mean, we could've saved the genlmsg_parse() call before, with much the
> same patch, having the per-op policy doesn't really have any bearing for
> that? It was just using a different policy - the family one - instead of
> the per-op one, but ...
> 
> Am I missing something?

Hm, not as far as I can tell, I was probably typing out the message
fast cause the commit is kinda obivious.

Looking at the code again now I can't tell why it was calling
genlmsg_parse() in the first place. LMK if you remember if there 
was a reason.

Otherwise I'll just reword.
Johannes Berg Oct. 1, 2020, 9:10 p.m. UTC | #3 
On Thu, 2020-10-01 at 14:09 -0700, Jakub Kicinski wrote:
> On Thu, 01 Oct 2020 22:55:09 +0200 Johannes Berg wrote:
> > On Thu, 2020-10-01 at 11:30 -0700, Jakub Kicinski wrote:
> > > Wire up per-op policy for CTRL_CMD_GETPOLICY.
> > > This saves us a call to genlmsg_parse() and will soon allow
> > > dumping this policy.  
> > 
> > Hmm. Probably should've asked this before - I think the code makes
> > perfect sense, but I'm not sure how "this" follows?
> > 
> > I mean, we could've saved the genlmsg_parse() call before, with much the
> > same patch, having the per-op policy doesn't really have any bearing for
> > that? It was just using a different policy - the family one - instead of
> > the per-op one, but ...
> > 
> > Am I missing something?
> 
> Hm, not as far as I can tell, I was probably typing out the message
> fast cause the commit is kinda obivious.
> 
> Looking at the code again now I can't tell why it was calling
> genlmsg_parse() in the first place. LMK if you remember if there 
> was a reason.

Quite possibly it was just old code? I _think_, but didn't check now,
that the parsing for dumpit was added later. Hence the "don't parse"
validate flag, because it wasn't always done and thus would've accepted
any kind of junk as input ...

johannes
diff mbox series

Patch

diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index 2a3608cfb179..81f0b960e9f8 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -1115,20 +1115,21 @@  struct ctrl_dump_policy_ctx {
 	u16 fam_id;
 };
 
+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 },
+};
+
 static int ctrl_dumppolicy_start(struct netlink_callback *cb)
 {
+	const struct genl_dumpit_info *info = genl_dumpit_info(cb);
 	struct ctrl_dump_policy_ctx *ctx = (void *)cb->ctx;
-	struct nlattr *tb[CTRL_ATTR_MAX + 1];
+	struct nlattr **tb = info->attrs;
 	const struct genl_family *rt;
-	int err;
 
 	BUILD_BUG_ON(sizeof(*ctx) > sizeof(cb->ctx));
 
-	err = genlmsg_parse(cb->nlh, &genl_ctrl, tb, genl_ctrl.maxattr,
-			    genl_ctrl.policy, cb->extack);
-	if (err)
-		return err;
-
 	if (!tb[CTRL_ATTR_FAMILY_ID] && !tb[CTRL_ATTR_FAMILY_NAME])
 		return -EINVAL;
 
@@ -1198,6 +1199,8 @@  static const struct genl_ops genl_ctrl_ops[] = {
 	},
 	{
 		.cmd		= CTRL_CMD_GETPOLICY,
+		.policy		= ctrl_policy_policy,
+		.maxattr	= ARRAY_SIZE(ctrl_policy_policy) - 1,
 		.start		= ctrl_dumppolicy_start,
 		.dumpit		= ctrl_dumppolicy,
 	},