[PATCH net-next v2 00/13] genetlink: support per op type policies

[PATCH net-next v2 00/13] genetlink: support per op type policies

[Jakub Kicinski]

While writing new genetlink families I was increasingly annoyed by the fact
that we don't support different policies for do and dump callbacks.
This makes it hard to do proper input validation for dumps which usually
have a lot more narrow range of accepted attributes.

There is also a minor inconvenience of not supporting different per_doit
and post_doit callbacks per op.

This series addresses those problems by introducing another op format.

v2:
 - wait for net changes to propagate
 - restore the missing comment in patch 1
 - drop extra space in patch 3
 - improve commit message in patch 4
v1: https://lore.kernel.org/all/20221018230728.1039524-1-kuba@kernel.org/

Jakub Kicinski (13):
  genetlink: refactor the cmd <> policy mapping dump
  genetlink: move the private fields in struct genl_family
  genetlink: introduce split op representation
  genetlink: load policy based on validation flags
  genetlink: check for callback type at op load time
  genetlink: add policies for both doit and dumpit in
    ctrl_dumppolicy_start()
  genetlink: support split policies in ctrl_dumppolicy_put_op()
  genetlink: inline genl_get_cmd()
  genetlink: add iterator for walking family ops
  genetlink: use iterator in the op to policy map dumping
  genetlink: inline old iteration helpers
  genetlink: allow families to use split ops directly
  genetlink: convert control family to split ops

 include/net/genetlink.h   |  76 +++++-
 net/batman-adv/netlink.c  |   6 +-
 net/core/devlink.c        |   4 +-
 net/core/drop_monitor.c   |   4 +-
 net/ieee802154/nl802154.c |   6 +-
 net/netlink/genetlink.c   | 473 ++++++++++++++++++++++++++++----------
 net/wireless/nl80211.c    |   6 +-
 7 files changed, 433 insertions(+), 142 deletions(-)

-- 
2.38.1

[PATCH net-next v2 01/13] genetlink: refactor the cmd <> policy mapping dump

[Jakub Kicinski]

The code at the top of ctrl_dumppolicy() dumps mappings between
ops and policies. It supports dumping both the entire family and
single op if dump is filtered. But both of those cases are handled
inside a loop, which makes the logic harder to follow and change.
Refactor to split the two cases more clearly.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
v2: bring the comment back
---
 net/netlink/genetlink.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index 3e16527beb91..0a7a856e9ce0 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -1319,21 +1319,24 @@ static int ctrl_dumppolicy(struct sk_buff *skb, struct netlink_callback *cb)
 	void *hdr;
 
 	if (!ctx->policies) {
-		while (ctx->opidx < genl_get_cmd_cnt(ctx->rt)) {
-			struct genl_ops op;
+		struct genl_ops op;
 
-			if (ctx->single_op) {
-				int err;
+		if (ctx->single_op) {
+			int err;
 
-				err = genl_get_cmd(ctx->op, ctx->rt, &op);
-				if (WARN_ON(err))
-					return skb->len;
+			err = genl_get_cmd(ctx->op, ctx->rt, &op);
+			if (WARN_ON(err))
+				return err;
 
-				/* break out of the loop after this one */
-				ctx->opidx = genl_get_cmd_cnt(ctx->rt);
-			} else {
-				genl_get_cmd_by_index(ctx->opidx, ctx->rt, &op);
-			}
+			if (ctrl_dumppolicy_put_op(skb, cb, &op))
+				return skb->len;
+
+			/* don't enter the loop below */
+			ctx->opidx = genl_get_cmd_cnt(ctx->rt);
+		}
+
+		while (ctx->opidx < genl_get_cmd_cnt(ctx->rt)) {
+			genl_get_cmd_by_index(ctx->opidx, ctx->rt, &op);
 
 			if (ctrl_dumppolicy_put_op(skb, cb, &op))
 				return skb->len;
-- 
2.38.1

[PATCH net-next v2 02/13] genetlink: move the private fields in struct genl_family

[Jakub Kicinski]

Move the private fields down to form a "private section".
Use the kdoc "private:" label comment thing to hide them
from the main kdoc comment.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Reviewed-by: Johannes Berg <johannes@sipsolutions.net>
Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
---
I did this cleanup to add more private fields but ended up
not needing them. Still I think the commit makes sense?
---
 include/net/genetlink.h | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/include/net/genetlink.h b/include/net/genetlink.h
index 9f97f73615b6..81180fc6526a 100644
--- a/include/net/genetlink.h
+++ b/include/net/genetlink.h
@@ -23,7 +23,6 @@ struct genl_info;
 
 /**
  * struct genl_family - generic netlink family
- * @id: protocol family identifier (private)
  * @hdrsize: length of user specific header in bytes
  * @name: name of family
  * @version: protocol version
@@ -43,8 +42,6 @@ struct genl_info;
  * @resv_start_op: first operation for which reserved fields of the header
  *	can be validated and policies are required (see below);
  *	new families should leave this field at zero
- * @mcgrp_offset: starting number of multicast group IDs in this family
- *	(private)
  * @ops: the operations supported by this family
  * @n_ops: number of operations supported by this family
  * @small_ops: the small-struct operations supported by this family
@@ -58,12 +55,10 @@ struct genl_info;
  * if policy is not provided core will reject all TLV attributes.
  */
 struct genl_family {
-	int			id;		/* private */
 	unsigned int		hdrsize;
 	char			name[GENL_NAMSIZ];
 	unsigned int		version;
 	unsigned int		maxattr;
-	unsigned int		mcgrp_offset;	/* private */
 	u8			netnsok:1;
 	u8			parallel_ops:1;
 	u8			n_ops;
@@ -81,6 +76,12 @@ struct genl_family {
 	const struct genl_small_ops *small_ops;
 	const struct genl_multicast_group *mcgrps;
 	struct module		*module;
+
+/* private: internal use only */
+	/* protocol family identifier */
+	int			id;
+	/* starting number of multicast group IDs in this family */
+	unsigned int		mcgrp_offset;
 };
 
 /**
-- 
2.38.1

[PATCH net-next v2 03/13] genetlink: introduce split op representation

[Jakub Kicinski]

We currently have two forms of operations - small ops and "full" ops
(or just ops). The former does not have pointers for some of the less
commonly used features (namely dump start/done and policy).

The "full" ops, however, still don't contain all the necessary
information. In particular the policy is per command ID, while
do and dump often accept different attributes. It's also not
possible to define different pre_doit and post_doit callbacks
for different commands within the family.

At the same time a lot of commands do not support dumping and
therefore all the dump-related information is wasted space.

Create a new command representation which can hold info about
a do implementation or a dump implementation, but not both at
the same time.

Use this new representation on the command execution path
(genl_family_rcv_msg) as we either run a do or a dump and
don't have to create a "full" op there.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
v2: - actually use the pre/post from the op
    - remove stray space
    - move a comment from patch 9 here

CC: mareklindner@neomailbox.ch
CC: sw@simonwunderlich.de
CC: a@unstable.cc
CC: sven@narfation.org
CC: jiri@nvidia.com
CC: nhorman@tuxdriver.com
CC: alex.aring@gmail.com
CC: stefan@datenfreihafen.org
CC: johannes@sipsolutions.net
---
 include/net/genetlink.h   | 60 +++++++++++++++++++++++++++--
 net/batman-adv/netlink.c  |  6 ++-
 net/core/devlink.c        |  4 +-
 net/core/drop_monitor.c   |  4 +-
 net/ieee802154/nl802154.c |  6 ++-
 net/netlink/genetlink.c   | 79 +++++++++++++++++++++++++++++++--------
 net/wireless/nl80211.c    |  6 ++-
 7 files changed, 136 insertions(+), 29 deletions(-)

diff --git a/include/net/genetlink.h b/include/net/genetlink.h
index 81180fc6526a..4be7989c451b 100644
--- a/include/net/genetlink.h
+++ b/include/net/genetlink.h
@@ -18,7 +18,7 @@ struct genl_multicast_group {
 	u8			flags;
 };
 
-struct genl_ops;
+struct genl_split_ops;
 struct genl_info;
 
 /**
@@ -66,10 +66,10 @@ struct genl_family {
 	u8			n_mcgrps;
 	u8			resv_start_op;
 	const struct nla_policy *policy;
-	int			(*pre_doit)(const struct genl_ops *ops,
+	int			(*pre_doit)(const struct genl_split_ops *ops,
 					    struct sk_buff *skb,
 					    struct genl_info *info);
-	void			(*post_doit)(const struct genl_ops *ops,
+	void			(*post_doit)(const struct genl_split_ops *ops,
 					     struct sk_buff *skb,
 					     struct genl_info *info);
 	const struct genl_ops *	ops;
@@ -182,6 +182,58 @@ struct genl_ops {
 	u8			validate;
 };
 
+/**
+ * struct genl_split_ops - generic netlink operations (do/dump split version)
+ * @cmd: command identifier
+ * @internal_flags: flags used by the family
+ * @flags: GENL_* flags (%GENL_ADMIN_PERM or %GENL_UNS_ADMIN_PERM)
+ * @validate: validation flags from enum genl_validate_flags
+ * @policy: netlink policy (takes precedence over family policy)
+ * @maxattr: maximum number of attributes supported
+ *
+ * Do callbacks:
+ * @pre_doit: called before an operation's @doit callback, it may
+ *	do additional, common, filtering and return an error
+ * @doit: standard command callback
+ * @post_doit: called after an operation's @doit callback, it may
+ *	undo operations done by pre_doit, for example release locks
+ *
+ * Dump callbacks:
+ * @start: start callback for dumps
+ * @dumpit: callback for dumpers
+ * @done: completion callback for dumps
+ *
+ * Do callbacks can be used if %GENL_CMD_CAP_DO is set in @flags.
+ * Dump callbacks can be used if %GENL_CMD_CAP_DUMP is set in @flags.
+ * Exactly one of those flags must be set.
+ */
+struct genl_split_ops {
+	union {
+		struct {
+			int (*pre_doit)(const struct genl_split_ops *ops,
+					struct sk_buff *skb,
+					struct genl_info *info);
+			int (*doit)(struct sk_buff *skb,
+				    struct genl_info *info);
+			void (*post_doit)(const struct genl_split_ops *ops,
+					  struct sk_buff *skb,
+					  struct genl_info *info);
+		};
+		struct {
+			int (*start)(struct netlink_callback *cb);
+			int (*dumpit)(struct sk_buff *skb,
+				      struct netlink_callback *cb);
+			int (*done)(struct netlink_callback *cb);
+		};
+	};
+	const struct nla_policy *policy;
+	unsigned int		maxattr;
+	u8			cmd;
+	u8			internal_flags;
+	u8			flags;
+	u8			validate;
+};
+
 /**
  * struct genl_dumpit_info - info that is available during dumpit op call
  * @family: generic netlink family - for internal genl code usage
@@ -190,7 +242,7 @@ struct genl_ops {
  */
 struct genl_dumpit_info {
 	const struct genl_family *family;
-	struct genl_ops op;
+	struct genl_split_ops op;
 	struct nlattr **attrs;
 };
 
diff --git a/net/batman-adv/netlink.c b/net/batman-adv/netlink.c
index a5e4a4e976cf..ad5714f737be 100644
--- a/net/batman-adv/netlink.c
+++ b/net/batman-adv/netlink.c
@@ -1267,7 +1267,8 @@ batadv_get_vlan_from_info(struct batadv_priv *bat_priv, struct net *net,
  *
  * Return: 0 on success or negative error number in case of failure
  */
-static int batadv_pre_doit(const struct genl_ops *ops, struct sk_buff *skb,
+static int batadv_pre_doit(const struct genl_split_ops *ops,
+			   struct sk_buff *skb,
 			   struct genl_info *info)
 {
 	struct net *net = genl_info_net(info);
@@ -1332,7 +1333,8 @@ static int batadv_pre_doit(const struct genl_ops *ops, struct sk_buff *skb,
  * @skb: Netlink message with request data
  * @info: receiver information
  */
-static void batadv_post_doit(const struct genl_ops *ops, struct sk_buff *skb,
+static void batadv_post_doit(const struct genl_split_ops *ops,
+			     struct sk_buff *skb,
 			     struct genl_info *info)
 {
 	struct batadv_hard_iface *hard_iface;
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 0a16ad45520e..3f26fc3ff3bd 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -769,7 +769,7 @@ devlink_region_snapshot_get_by_id(struct devlink_region *region, u32 id)
 #define DEVLINK_NL_FLAG_NEED_RATE_NODE		BIT(3)
 #define DEVLINK_NL_FLAG_NEED_LINECARD		BIT(4)
 
-static int devlink_nl_pre_doit(const struct genl_ops *ops,
+static int devlink_nl_pre_doit(const struct genl_split_ops *ops,
 			       struct sk_buff *skb, struct genl_info *info)
 {
 	struct devlink_linecard *linecard;
@@ -827,7 +827,7 @@ static int devlink_nl_pre_doit(const struct genl_ops *ops,
 	return err;
 }
 
-static void devlink_nl_post_doit(const struct genl_ops *ops,
+static void devlink_nl_post_doit(const struct genl_split_ops *ops,
 				 struct sk_buff *skb, struct genl_info *info)
 {
 	struct devlink_linecard *linecard;
diff --git a/net/core/drop_monitor.c b/net/core/drop_monitor.c
index 11aa6e8a3098..5a782d1d8fd3 100644
--- a/net/core/drop_monitor.c
+++ b/net/core/drop_monitor.c
@@ -1620,7 +1620,7 @@ static const struct genl_small_ops dropmon_ops[] = {
 	},
 };
 
-static int net_dm_nl_pre_doit(const struct genl_ops *ops,
+static int net_dm_nl_pre_doit(const struct genl_split_ops *ops,
 			      struct sk_buff *skb, struct genl_info *info)
 {
 	mutex_lock(&net_dm_mutex);
@@ -1628,7 +1628,7 @@ static int net_dm_nl_pre_doit(const struct genl_ops *ops,
 	return 0;
 }
 
-static void net_dm_nl_post_doit(const struct genl_ops *ops,
+static void net_dm_nl_post_doit(const struct genl_split_ops *ops,
 				struct sk_buff *skb, struct genl_info *info)
 {
 	mutex_unlock(&net_dm_mutex);
diff --git a/net/ieee802154/nl802154.c b/net/ieee802154/nl802154.c
index 38c4f3cb010e..b33d1b5eda87 100644
--- a/net/ieee802154/nl802154.c
+++ b/net/ieee802154/nl802154.c
@@ -2157,7 +2157,8 @@ static int nl802154_del_llsec_seclevel(struct sk_buff *skb,
 #define NL802154_FLAG_CHECK_NETDEV_UP	0x08
 #define NL802154_FLAG_NEED_WPAN_DEV	0x10
 
-static int nl802154_pre_doit(const struct genl_ops *ops, struct sk_buff *skb,
+static int nl802154_pre_doit(const struct genl_split_ops *ops,
+			     struct sk_buff *skb,
 			     struct genl_info *info)
 {
 	struct cfg802154_registered_device *rdev;
@@ -2219,7 +2220,8 @@ static int nl802154_pre_doit(const struct genl_ops *ops, struct sk_buff *skb,
 	return 0;
 }
 
-static void nl802154_post_doit(const struct genl_ops *ops, struct sk_buff *skb,
+static void nl802154_post_doit(const struct genl_split_ops *ops,
+			       struct sk_buff *skb,
 			       struct genl_info *info)
 {
 	if (info->user_ptr[1]) {
diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index 0a7a856e9ce0..c66299740c05 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -189,6 +189,51 @@ static int genl_get_cmd(u32 cmd, const struct genl_family *family,
 	return genl_get_cmd_small(cmd, family, op);
 }
 
+static void
+genl_cmd_full_to_split(struct genl_split_ops *op,
+		       const struct genl_family *family,
+		       const struct genl_ops *full, u8 flags)
+{
+	if (flags & GENL_CMD_CAP_DUMP) {
+		op->start	= full->start;
+		op->dumpit	= full->dumpit;
+		op->done	= full->done;
+	} else {
+		op->pre_doit	= family->pre_doit;
+		op->doit	= full->doit;
+		op->post_doit	= family->post_doit;
+	}
+
+	op->policy		= full->policy;
+	op->maxattr		= full->maxattr;
+
+	op->cmd			= full->cmd;
+	op->internal_flags	= full->internal_flags;
+	op->flags		= full->flags;
+	op->validate		= full->validate;
+
+	/* Make sure flags include the GENL_CMD_CAP_DO / GENL_CMD_CAP_DUMP */
+	op->flags		|= flags;
+}
+
+static int
+genl_get_cmd_split(u32 cmd, u8 flags, const struct genl_family *family,
+		   struct genl_split_ops *op)
+{
+	struct genl_ops full;
+	int err;
+
+	err = genl_get_cmd(cmd, family, &full);
+	if (err) {
+		memset(op, 0, sizeof(*op));
+		return err;
+	}
+
+	genl_cmd_full_to_split(op, family, &full, flags);
+
+	return 0;
+}
+
 static void genl_get_cmd_by_index(unsigned int i,
 				  const struct genl_family *family,
 				  struct genl_ops *op)
@@ -544,7 +589,7 @@ static struct nlattr **
 genl_family_rcv_msg_attrs_parse(const struct genl_family *family,
 				struct nlmsghdr *nlh,
 				struct netlink_ext_ack *extack,
-				const struct genl_ops *ops,
+				const struct genl_split_ops *ops,
 				int hdrlen,
 				enum genl_validate_flags no_strict_flag)
 {
@@ -580,18 +625,19 @@ struct genl_start_context {
 	const struct genl_family *family;
 	struct nlmsghdr *nlh;
 	struct netlink_ext_ack *extack;
-	const struct genl_ops *ops;
+	const struct genl_split_ops *ops;
 	int hdrlen;
 };
 
 static int genl_start(struct netlink_callback *cb)
 {
 	struct genl_start_context *ctx = cb->data;
-	const struct genl_ops *ops = ctx->ops;
+	const struct genl_split_ops *ops;
 	struct genl_dumpit_info *info;
 	struct nlattr **attrs = NULL;
 	int rc = 0;
 
+	ops = ctx->ops;
 	if (ops->validate & GENL_DONT_VALIDATE_DUMP)
 		goto no_attrs;
 
@@ -633,7 +679,7 @@ static int genl_start(struct netlink_callback *cb)
 
 static int genl_lock_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
 {
-	const struct genl_ops *ops = &genl_dumpit_info(cb)->op;
+	const struct genl_split_ops *ops = &genl_dumpit_info(cb)->op;
 	int rc;
 
 	genl_lock();
@@ -645,7 +691,7 @@ static int genl_lock_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
 static int genl_lock_done(struct netlink_callback *cb)
 {
 	const struct genl_dumpit_info *info = genl_dumpit_info(cb);
-	const struct genl_ops *ops = &info->op;
+	const struct genl_split_ops *ops = &info->op;
 	int rc = 0;
 
 	if (ops->done) {
@@ -661,7 +707,7 @@ static int genl_lock_done(struct netlink_callback *cb)
 static int genl_parallel_done(struct netlink_callback *cb)
 {
 	const struct genl_dumpit_info *info = genl_dumpit_info(cb);
-	const struct genl_ops *ops = &info->op;
+	const struct genl_split_ops *ops = &info->op;
 	int rc = 0;
 
 	if (ops->done)
@@ -675,7 +721,7 @@ static int genl_family_rcv_msg_dumpit(const struct genl_family *family,
 				      struct sk_buff *skb,
 				      struct nlmsghdr *nlh,
 				      struct netlink_ext_ack *extack,
-				      const struct genl_ops *ops,
+				      const struct genl_split_ops *ops,
 				      int hdrlen, struct net *net)
 {
 	struct genl_start_context ctx;
@@ -721,7 +767,7 @@ static int genl_family_rcv_msg_doit(const struct genl_family *family,
 				    struct sk_buff *skb,
 				    struct nlmsghdr *nlh,
 				    struct netlink_ext_ack *extack,
-				    const struct genl_ops *ops,
+				    const struct genl_split_ops *ops,
 				    int hdrlen, struct net *net)
 {
 	struct nlattr **attrbuf;
@@ -747,16 +793,16 @@ static int genl_family_rcv_msg_doit(const struct genl_family *family,
 	genl_info_net_set(&info, net);
 	memset(&info.user_ptr, 0, sizeof(info.user_ptr));
 
-	if (family->pre_doit) {
-		err = family->pre_doit(ops, skb, &info);
+	if (ops->pre_doit) {
+		err = ops->pre_doit(ops, skb, &info);
 		if (err)
 			goto out;
 	}
 
 	err = ops->doit(skb, &info);
 
-	if (family->post_doit)
-		family->post_doit(ops, skb, &info);
+	if (ops->post_doit)
+		ops->post_doit(ops, skb, &info);
 
 out:
 	genl_family_rcv_msg_attrs_free(attrbuf);
@@ -801,8 +847,9 @@ static int genl_family_rcv_msg(const struct genl_family *family,
 {
 	struct net *net = sock_net(skb->sk);
 	struct genlmsghdr *hdr = nlmsg_data(nlh);
-	struct genl_ops op;
+	struct genl_split_ops op;
 	int hdrlen;
+	u8 flags;
 
 	/* this family doesn't exist in this netns */
 	if (!family->netnsok && !net_eq(net, &init_net))
@@ -815,7 +862,9 @@ static int genl_family_rcv_msg(const struct genl_family *family,
 	if (genl_header_check(family, nlh, hdr, extack))
 		return -EINVAL;
 
-	if (genl_get_cmd(hdr->cmd, family, &op))
+	flags = (nlh->nlmsg_flags & NLM_F_DUMP) == NLM_F_DUMP ?
+		GENL_CMD_CAP_DUMP : GENL_CMD_CAP_DO;
+	if (genl_get_cmd_split(hdr->cmd, flags, family, &op))
 		return -EOPNOTSUPP;
 
 	if ((op.flags & GENL_ADMIN_PERM) &&
@@ -826,7 +875,7 @@ static int genl_family_rcv_msg(const struct genl_family *family,
 	    !netlink_ns_capable(skb, net->user_ns, CAP_NET_ADMIN))
 		return -EPERM;
 
-	if ((nlh->nlmsg_flags & NLM_F_DUMP) == NLM_F_DUMP)
+	if (flags & GENL_CMD_CAP_DUMP)
 		return genl_family_rcv_msg_dumpit(family, skb, nlh, extack,
 						  &op, hdrlen, net);
 	else
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 148f66edb015..1ad0326ff4dc 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -16140,7 +16140,8 @@ static u32 nl80211_internal_flags[] = {
 #undef SELECTOR
 };
 
-static int nl80211_pre_doit(const struct genl_ops *ops, struct sk_buff *skb,
+static int nl80211_pre_doit(const struct genl_split_ops *ops,
+			    struct sk_buff *skb,
 			    struct genl_info *info)
 {
 	struct cfg80211_registered_device *rdev = NULL;
@@ -16241,7 +16242,8 @@ static int nl80211_pre_doit(const struct genl_ops *ops, struct sk_buff *skb,
 	return err;
 }
 
-static void nl80211_post_doit(const struct genl_ops *ops, struct sk_buff *skb,
+static void nl80211_post_doit(const struct genl_split_ops *ops,
+			      struct sk_buff *skb,
 			      struct genl_info *info)
 {
 	u32 internal_flags = nl80211_internal_flags[ops->internal_flags];
-- 
2.38.1

[PATCH net-next v2 04/13] genetlink: load policy based on validation flags

[Jakub Kicinski]

Set the policy and maxattr pointers based on validation flags.
genl_family_rcv_msg_attrs_parse() will do nothing and return NULL
if maxattrs is zero, so no behavior change is expected.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
v2: improve commit msg
---
 net/netlink/genetlink.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index c66299740c05..770726ac491e 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -204,8 +204,14 @@ genl_cmd_full_to_split(struct genl_split_ops *op,
 		op->post_doit	= family->post_doit;
 	}
 
-	op->policy		= full->policy;
-	op->maxattr		= full->maxattr;
+	if (flags & GENL_CMD_CAP_DUMP &&
+	    full->validate & GENL_DONT_VALIDATE_DUMP) {
+		op->policy	= NULL;
+		op->maxattr	= 0;
+	} else {
+		op->policy	= full->policy;
+		op->maxattr	= full->maxattr;
+	}
 
 	op->cmd			= full->cmd;
 	op->internal_flags	= full->internal_flags;
@@ -638,10 +644,8 @@ static int genl_start(struct netlink_callback *cb)
 	int rc = 0;
 
 	ops = ctx->ops;
-	if (ops->validate & GENL_DONT_VALIDATE_DUMP)
-		goto no_attrs;
-
-	if (ctx->nlh->nlmsg_len < nlmsg_msg_size(ctx->hdrlen))
+	if (!(ops->validate & GENL_DONT_VALIDATE_DUMP) &&
+	    ctx->nlh->nlmsg_len < nlmsg_msg_size(ctx->hdrlen))
 		return -EINVAL;
 
 	attrs = genl_family_rcv_msg_attrs_parse(ctx->family, ctx->nlh, ctx->extack,
@@ -650,7 +654,6 @@ static int genl_start(struct netlink_callback *cb)
 	if (IS_ERR(attrs))
 		return PTR_ERR(attrs);
 
-no_attrs:
 	info = genl_dumpit_info_alloc();
 	if (!info) {
 		genl_family_rcv_msg_attrs_free(attrs);
-- 
2.38.1

[PATCH net-next v2 05/13] genetlink: check for callback type at op load time

[Jakub Kicinski]

Now that genl_get_cmd_split() is informed what type of callback
user is trying to access (do or dump) we can check that this
callback is indeed available and return an error early.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/netlink/genetlink.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index 770726ac491e..7c04df1bee2b 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -189,11 +189,17 @@ static int genl_get_cmd(u32 cmd, const struct genl_family *family,
 	return genl_get_cmd_small(cmd, family, op);
 }
 
-static void
+static int
 genl_cmd_full_to_split(struct genl_split_ops *op,
 		       const struct genl_family *family,
 		       const struct genl_ops *full, u8 flags)
 {
+	if ((flags & GENL_CMD_CAP_DO && !full->doit) ||
+	    (flags & GENL_CMD_CAP_DUMP && !full->dumpit)) {
+		memset(op, 0, sizeof(*op));
+		return -ENOENT;
+	}
+
 	if (flags & GENL_CMD_CAP_DUMP) {
 		op->start	= full->start;
 		op->dumpit	= full->dumpit;
@@ -220,6 +226,8 @@ genl_cmd_full_to_split(struct genl_split_ops *op,
 
 	/* Make sure flags include the GENL_CMD_CAP_DO / GENL_CMD_CAP_DUMP */
 	op->flags		|= flags;
+
+	return 0;
 }
 
 static int
@@ -235,9 +243,7 @@ genl_get_cmd_split(u32 cmd, u8 flags, const struct genl_family *family,
 		return err;
 	}
 
-	genl_cmd_full_to_split(op, family, &full, flags);
-
-	return 0;
+	return genl_cmd_full_to_split(op, family, &full, flags);
 }
 
 static void genl_get_cmd_by_index(unsigned int i,
@@ -730,9 +736,6 @@ static int genl_family_rcv_msg_dumpit(const struct genl_family *family,
 	struct genl_start_context ctx;
 	int err;
 
-	if (!ops->dumpit)
-		return -EOPNOTSUPP;
-
 	ctx.family = family;
 	ctx.nlh = nlh;
 	ctx.extack = extack;
@@ -777,9 +780,6 @@ static int genl_family_rcv_msg_doit(const struct genl_family *family,
 	struct genl_info info;
 	int err;
 
-	if (!ops->doit)
-		return -EOPNOTSUPP;
-
 	attrbuf = genl_family_rcv_msg_attrs_parse(family, nlh, extack,
 						  ops, hdrlen,
 						  GENL_DONT_VALIDATE_STRICT);
-- 
2.38.1

[PATCH net-next v2 06/13] genetlink: add policies for both doit and dumpit in ctrl_dumppolicy_start()

[Jakub Kicinski]

Separate adding doit and dumpit policies for CTRL_CMD_GETPOLICY.
This has no effect until we actually allow do and dump to come
from different sources as netlink_policy_dump_add_policy()
does deduplication.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/netlink/genetlink.c | 48 ++++++++++++++++++++++++++++++++---------
 1 file changed, 38 insertions(+), 10 deletions(-)

diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index 7c04df1bee2b..d0c35738839b 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -1260,29 +1260,57 @@ static int ctrl_dumppolicy_start(struct netlink_callback *cb)
 	ctx->rt = rt;
 
 	if (tb[CTRL_ATTR_OP]) {
+		struct genl_split_ops doit, dump;
+
 		ctx->single_op = true;
 		ctx->op = nla_get_u32(tb[CTRL_ATTR_OP]);
 
-		err = genl_get_cmd(ctx->op, rt, &op);
-		if (err) {
+		if (genl_get_cmd_split(ctx->op, GENL_CMD_CAP_DO, rt, &doit) &&
+		    genl_get_cmd_split(ctx->op, GENL_CMD_CAP_DUMP, rt, &dump)) {
 			NL_SET_BAD_ATTR(cb->extack, tb[CTRL_ATTR_OP]);
-			return err;
+			return -ENOENT;
 		}
 
-		if (!op.policy)
-			return -ENODATA;
+		if (doit.policy) {
+			err = netlink_policy_dump_add_policy(&ctx->state,
+							     doit.policy,
+							     doit.maxattr);
+			if (err)
+				goto err_free_state;
+		}
+		if (dump.policy) {
+			err = netlink_policy_dump_add_policy(&ctx->state,
+							     dump.policy,
+							     dump.maxattr);
+			if (err)
+				goto err_free_state;
+		}
 
-		return netlink_policy_dump_add_policy(&ctx->state, op.policy,
-						      op.maxattr);
+		if (!ctx->state)
+			return -ENODATA;
+		return 0;
 	}
 
 	for (i = 0; i < genl_get_cmd_cnt(rt); i++) {
+		struct genl_split_ops doit, dumpit;
+
 		genl_get_cmd_by_index(i, rt, &op);
 
-		if (op.policy) {
+		genl_cmd_full_to_split(&doit, ctx->rt, &op, GENL_CMD_CAP_DO);
+		genl_cmd_full_to_split(&dumpit, ctx->rt,
+				       &op, GENL_CMD_CAP_DUMP);
+
+		if (doit.policy) {
+			err = netlink_policy_dump_add_policy(&ctx->state,
+							     doit.policy,
+							     doit.maxattr);
+			if (err)
+				goto err_free_state;
+		}
+		if (dumpit.policy) {
 			err = netlink_policy_dump_add_policy(&ctx->state,
-							     op.policy,
-							     op.maxattr);
+							     dumpit.policy,
+							     dumpit.maxattr);
 			if (err)
 				goto err_free_state;
 		}
-- 
2.38.1

[PATCH net-next v2 07/13] genetlink: support split policies in ctrl_dumppolicy_put_op()

[Jakub Kicinski]

Pass do and dump versions of the op to ctrl_dumppolicy_put_op()
so that it can provide a different policy index for the two.

Since we now look at policies, and those are set appropriately
there's no need to look at the GENL_DONT_VALIDATE_DUMP flag.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
---
v2: while at it fix the whitespace to appease checkpatch
---
 net/netlink/genetlink.c | 55 ++++++++++++++++++++++++-----------------
 1 file changed, 33 insertions(+), 22 deletions(-)

diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index d0c35738839b..93e33e20a0e8 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -1345,7 +1345,8 @@ static void *ctrl_dumppolicy_prep(struct sk_buff *skb,
 
 static int ctrl_dumppolicy_put_op(struct sk_buff *skb,
 				  struct netlink_callback *cb,
-			          struct genl_ops *op)
+				  struct genl_split_ops *doit,
+				  struct genl_split_ops *dumpit)
 {
 	struct ctrl_dump_policy_ctx *ctx = (void *)cb->ctx;
 	struct nlattr *nest_pol, *nest_op;
@@ -1353,10 +1354,7 @@ static int ctrl_dumppolicy_put_op(struct sk_buff *skb,
 	int idx;
 
 	/* skip if we have nothing to show */
-	if (!op->policy)
-		return 0;
-	if (!op->doit &&
-	    (!op->dumpit || op->validate & GENL_DONT_VALIDATE_DUMP))
+	if (!doit->policy && !dumpit->policy)
 		return 0;
 
 	hdr = ctrl_dumppolicy_prep(skb, cb);
@@ -1367,21 +1365,26 @@ static int ctrl_dumppolicy_put_op(struct sk_buff *skb,
 	if (!nest_pol)
 		goto err;
 
-	nest_op = nla_nest_start(skb, op->cmd);
+	nest_op = nla_nest_start(skb, doit->cmd);
 	if (!nest_op)
 		goto err;
 
-	/* for now both do/dump are always the same */
-	idx = netlink_policy_dump_get_policy_idx(ctx->state,
-						 op->policy,
-						 op->maxattr);
+	if (doit->policy) {
+		idx = netlink_policy_dump_get_policy_idx(ctx->state,
+							 doit->policy,
+							 doit->maxattr);
 
-	if (op->doit && nla_put_u32(skb, CTRL_ATTR_POLICY_DO, idx))
-		goto err;
+		if (nla_put_u32(skb, CTRL_ATTR_POLICY_DO, idx))
+			goto err;
+	}
+	if (dumpit->policy) {
+		idx = netlink_policy_dump_get_policy_idx(ctx->state,
+							 dumpit->policy,
+							 dumpit->maxattr);
 
-	if (op->dumpit && !(op->validate & GENL_DONT_VALIDATE_DUMP) &&
-	    nla_put_u32(skb, CTRL_ATTR_POLICY_DUMP, idx))
-		goto err;
+		if (nla_put_u32(skb, CTRL_ATTR_POLICY_DUMP, idx))
+			goto err;
+	}
 
 	nla_nest_end(skb, nest_op);
 	nla_nest_end(skb, nest_pol);
@@ -1399,16 +1402,19 @@ static int ctrl_dumppolicy(struct sk_buff *skb, struct netlink_callback *cb)
 	void *hdr;
 
 	if (!ctx->policies) {
+		struct genl_split_ops doit, dumpit;
 		struct genl_ops op;
 
 		if (ctx->single_op) {
-			int err;
-
-			err = genl_get_cmd(ctx->op, ctx->rt, &op);
-			if (WARN_ON(err))
-				return err;
+			if (genl_get_cmd_split(ctx->op, GENL_CMD_CAP_DO,
+					       ctx->rt, &doit) &&
+			    genl_get_cmd_split(ctx->op, GENL_CMD_CAP_DUMP,
+					       ctx->rt, &dumpit)) {
+				WARN_ON(1);
+				return -ENOENT;
+			}
 
-			if (ctrl_dumppolicy_put_op(skb, cb, &op))
+			if (ctrl_dumppolicy_put_op(skb, cb, &doit, &dumpit))
 				return skb->len;
 
 			/* don't enter the loop below */
@@ -1418,7 +1424,12 @@ static int ctrl_dumppolicy(struct sk_buff *skb, struct netlink_callback *cb)
 		while (ctx->opidx < genl_get_cmd_cnt(ctx->rt)) {
 			genl_get_cmd_by_index(ctx->opidx, ctx->rt, &op);
 
-			if (ctrl_dumppolicy_put_op(skb, cb, &op))
+			genl_cmd_full_to_split(&doit, ctx->rt,
+					       &op, GENL_CMD_CAP_DO);
+			genl_cmd_full_to_split(&dumpit, ctx->rt,
+					       &op, GENL_CMD_CAP_DUMP);
+
+			if (ctrl_dumppolicy_put_op(skb, cb, &doit, &dumpit))
 				return skb->len;
 
 			ctx->opidx++;
-- 
2.38.1

[PATCH net-next v2 08/13] genetlink: inline genl_get_cmd()

[Jakub Kicinski]

All callers go via genl_get_cmd_split() now,
so merge genl_get_cmd() into it.

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/netlink/genetlink.c | 30 ++++++++++++------------------
 1 file changed, 12 insertions(+), 18 deletions(-)

diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index 93e33e20a0e8..ec32b6063a3f 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -181,14 +181,6 @@ static int genl_get_cmd_small(u32 cmd, const struct genl_family *family,
 	return -ENOENT;
 }
 
-static int genl_get_cmd(u32 cmd, const struct genl_family *family,
-			struct genl_ops *op)
-{
-	if (!genl_get_cmd_full(cmd, family, op))
-		return 0;
-	return genl_get_cmd_small(cmd, family, op);
-}
-
 static int
 genl_cmd_full_to_split(struct genl_split_ops *op,
 		       const struct genl_family *family,
@@ -231,13 +223,15 @@ genl_cmd_full_to_split(struct genl_split_ops *op,
 }
 
 static int
-genl_get_cmd_split(u32 cmd, u8 flags, const struct genl_family *family,
-		   struct genl_split_ops *op)
+genl_get_cmd(u32 cmd, u8 flags, const struct genl_family *family,
+	     struct genl_split_ops *op)
 {
 	struct genl_ops full;
 	int err;
 
-	err = genl_get_cmd(cmd, family, &full);
+	err = genl_get_cmd_full(cmd, family, &full);
+	if (err == -ENOENT)
+		err = genl_get_cmd_small(cmd, family, &full);
 	if (err) {
 		memset(op, 0, sizeof(*op));
 		return err;
@@ -867,7 +861,7 @@ static int genl_family_rcv_msg(const struct genl_family *family,
 
 	flags = (nlh->nlmsg_flags & NLM_F_DUMP) == NLM_F_DUMP ?
 		GENL_CMD_CAP_DUMP : GENL_CMD_CAP_DO;
-	if (genl_get_cmd_split(hdr->cmd, flags, family, &op))
+	if (genl_get_cmd(hdr->cmd, flags, family, &op))
 		return -EOPNOTSUPP;
 
 	if ((op.flags & GENL_ADMIN_PERM) &&
@@ -1265,8 +1259,8 @@ static int ctrl_dumppolicy_start(struct netlink_callback *cb)
 		ctx->single_op = true;
 		ctx->op = nla_get_u32(tb[CTRL_ATTR_OP]);
 
-		if (genl_get_cmd_split(ctx->op, GENL_CMD_CAP_DO, rt, &doit) &&
-		    genl_get_cmd_split(ctx->op, GENL_CMD_CAP_DUMP, rt, &dump)) {
+		if (genl_get_cmd(ctx->op, GENL_CMD_CAP_DO, rt, &doit) &&
+		    genl_get_cmd(ctx->op, GENL_CMD_CAP_DUMP, rt, &dump)) {
 			NL_SET_BAD_ATTR(cb->extack, tb[CTRL_ATTR_OP]);
 			return -ENOENT;
 		}
@@ -1406,10 +1400,10 @@ static int ctrl_dumppolicy(struct sk_buff *skb, struct netlink_callback *cb)
 		struct genl_ops op;
 
 		if (ctx->single_op) {
-			if (genl_get_cmd_split(ctx->op, GENL_CMD_CAP_DO,
-					       ctx->rt, &doit) &&
-			    genl_get_cmd_split(ctx->op, GENL_CMD_CAP_DUMP,
-					       ctx->rt, &dumpit)) {
+			if (genl_get_cmd(ctx->op, GENL_CMD_CAP_DO,
+					 ctx->rt, &doit) &&
+			    genl_get_cmd(ctx->op, GENL_CMD_CAP_DUMP,
+					 ctx->rt, &dumpit)) {
 				WARN_ON(1);
 				return -ENOENT;
 			}
-- 
2.38.1

[PATCH net-next v2 09/13] genetlink: add iterator for walking family ops

[Jakub Kicinski]

Subsequent changes will expose split op structures to users,
so walking the family ops with just an index will get harder.
Add a structured iterator, convert the simple cases.
Policy dumping needs more careful conversion.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/netlink/genetlink.c | 117 ++++++++++++++++++++++++++--------------
 1 file changed, 76 insertions(+), 41 deletions(-)

diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index ec32b6063a3f..06bf091c1b8a 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -252,6 +252,57 @@ static void genl_get_cmd_by_index(unsigned int i,
 		WARN_ON_ONCE(1);
 }
 
+struct genl_op_iter {
+	const struct genl_family *family;
+	struct genl_split_ops doit;
+	struct genl_split_ops dumpit;
+	int i;
+	u32 cmd;
+	u8 flags;
+};
+
+static bool
+genl_op_iter_init(const struct genl_family *family, struct genl_op_iter *iter)
+{
+	iter->family = family;
+	iter->i = 0;
+
+	iter->flags = 0;
+
+	return genl_get_cmd_cnt(iter->family);
+}
+
+static bool genl_op_iter_next(struct genl_op_iter *iter)
+{
+	struct genl_ops op;
+
+	if (iter->i >= genl_get_cmd_cnt(iter->family))
+		return false;
+
+	genl_get_cmd_by_index(iter->i, iter->family, &op);
+	iter->i++;
+
+	genl_cmd_full_to_split(&iter->doit, iter->family, &op, GENL_CMD_CAP_DO);
+	genl_cmd_full_to_split(&iter->dumpit, iter->family,
+			       &op, GENL_CMD_CAP_DUMP);
+
+	iter->cmd = iter->doit.cmd | iter->dumpit.cmd;
+	iter->flags = iter->doit.flags | iter->dumpit.flags;
+
+	return true;
+}
+
+static void
+genl_op_iter_copy(struct genl_op_iter *dst, struct genl_op_iter *src)
+{
+	*dst = *src;
+}
+
+static unsigned int genl_op_iter_idx(struct genl_op_iter *iter)
+{
+	return iter->i;
+}
+
 static int genl_allocate_reserve_groups(int n_groups, int *first_id)
 {
 	unsigned long *new_groups;
@@ -419,25 +470,23 @@ static void genl_unregister_mc_groups(const struct genl_family *family)
 
 static int genl_validate_ops(const struct genl_family *family)
 {
-	int i, j;
+	struct genl_op_iter i, j;
 
 	if (WARN_ON(family->n_ops && !family->ops) ||
 	    WARN_ON(family->n_small_ops && !family->small_ops))
 		return -EINVAL;
 
-	for (i = 0; i < genl_get_cmd_cnt(family); i++) {
-		struct genl_ops op;
-
-		genl_get_cmd_by_index(i, family, &op);
-		if (op.dumpit == NULL && op.doit == NULL)
+	for (genl_op_iter_init(family, &i); genl_op_iter_next(&i); ) {
+		if (!(i.flags & (GENL_CMD_CAP_DO | GENL_CMD_CAP_DUMP)))
 			return -EINVAL;
-		if (WARN_ON(op.cmd >= family->resv_start_op && op.validate))
+
+		if (WARN_ON(i.cmd >= family->resv_start_op &&
+			    (i.doit.validate || i.dumpit.validate)))
 			return -EINVAL;
-		for (j = i + 1; j < genl_get_cmd_cnt(family); j++) {
-			struct genl_ops op2;
 
-			genl_get_cmd_by_index(j, family, &op2);
-			if (op.cmd == op2.cmd)
+		genl_op_iter_copy(&j, &i);
+		while (genl_op_iter_next(&j)) {
+			if (i.cmd == j.cmd)
 				return -EINVAL;
 		}
 	}
@@ -917,6 +966,7 @@ static struct genl_family genl_ctrl;
 static int ctrl_fill_info(const struct genl_family *family, u32 portid, u32 seq,
 			  u32 flags, struct sk_buff *skb, u8 cmd)
 {
+	struct genl_op_iter i;
 	void *hdr;
 
 	hdr = genlmsg_put(skb, portid, seq, &genl_ctrl, flags, cmd);
@@ -930,33 +980,26 @@ static int ctrl_fill_info(const struct genl_family *family, u32 portid, u32 seq,
 	    nla_put_u32(skb, CTRL_ATTR_MAXATTR, family->maxattr))
 		goto nla_put_failure;
 
-	if (genl_get_cmd_cnt(family)) {
+	if (genl_op_iter_init(family, &i)) {
 		struct nlattr *nla_ops;
-		int i;
 
 		nla_ops = nla_nest_start_noflag(skb, CTRL_ATTR_OPS);
 		if (nla_ops == NULL)
 			goto nla_put_failure;
 
-		for (i = 0; i < genl_get_cmd_cnt(family); i++) {
+		while (genl_op_iter_next(&i)) {
 			struct nlattr *nest;
-			struct genl_ops op;
 			u32 op_flags;
 
-			genl_get_cmd_by_index(i, family, &op);
-			op_flags = op.flags;
-			if (op.dumpit)
-				op_flags |= GENL_CMD_CAP_DUMP;
-			if (op.doit)
-				op_flags |= GENL_CMD_CAP_DO;
-			if (op.policy)
+			op_flags = i.flags;
+			if (i.doit.policy || i.dumpit.policy)
 				op_flags |= GENL_CMD_CAP_HASPOL;
 
-			nest = nla_nest_start_noflag(skb, i + 1);
+			nest = nla_nest_start_noflag(skb, genl_op_iter_idx(&i));
 			if (nest == NULL)
 				goto nla_put_failure;
 
-			if (nla_put_u32(skb, CTRL_ATTR_OP_ID, op.cmd) ||
+			if (nla_put_u32(skb, CTRL_ATTR_OP_ID, i.cmd) ||
 			    nla_put_u32(skb, CTRL_ATTR_OP_FLAGS, op_flags))
 				goto nla_put_failure;
 
@@ -1229,8 +1272,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, i;
+	struct genl_op_iter i;
+	int err;
 
 	BUILD_BUG_ON(sizeof(*ctx) > sizeof(cb->ctx));
 
@@ -1285,26 +1328,18 @@ static int ctrl_dumppolicy_start(struct netlink_callback *cb)
 		return 0;
 	}
 
-	for (i = 0; i < genl_get_cmd_cnt(rt); i++) {
-		struct genl_split_ops doit, dumpit;
-
-		genl_get_cmd_by_index(i, rt, &op);
-
-		genl_cmd_full_to_split(&doit, ctx->rt, &op, GENL_CMD_CAP_DO);
-		genl_cmd_full_to_split(&dumpit, ctx->rt,
-				       &op, GENL_CMD_CAP_DUMP);
-
-		if (doit.policy) {
+	for (genl_op_iter_init(rt, &i); genl_op_iter_next(&i); ) {
+		if (i.doit.policy) {
 			err = netlink_policy_dump_add_policy(&ctx->state,
-							     doit.policy,
-							     doit.maxattr);
+							     i.doit.policy,
+							     i.doit.maxattr);
 			if (err)
 				goto err_free_state;
 		}
-		if (dumpit.policy) {
+		if (i.dumpit.policy) {
 			err = netlink_policy_dump_add_policy(&ctx->state,
-							     dumpit.policy,
-							     dumpit.maxattr);
+							     i.dumpit.policy,
+							     i.dumpit.maxattr);
 			if (err)
 				goto err_free_state;
 		}
-- 
2.38.1

[PATCH net-next v2 10/13] genetlink: use iterator in the op to policy map dumping

[Jakub Kicinski]

We can't put the full iterator in the struct ctrl_dump_policy_ctx
because dump context is statically sized by netlink core.
Allocate it dynamically.

Rename policy to dump_map to make the logic a little easier to follow.

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/netlink/genetlink.c | 49 ++++++++++++++++++++++-------------------
 1 file changed, 26 insertions(+), 23 deletions(-)

diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index 06bf091c1b8a..4b8c65d9e9d3 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -1252,10 +1252,10 @@ static int genl_ctrl_event(int event, const struct genl_family *family,
 struct ctrl_dump_policy_ctx {
 	struct netlink_policy_dump_state *state;
 	const struct genl_family *rt;
-	unsigned int opidx;
+	struct genl_op_iter *op_iter;
 	u32 op;
 	u16 fam_id;
-	u8 policies:1,
+	u8 dump_map:1,
 	   single_op:1;
 };
 
@@ -1325,9 +1325,16 @@ static int ctrl_dumppolicy_start(struct netlink_callback *cb)
 
 		if (!ctx->state)
 			return -ENODATA;
+
+		ctx->dump_map = 1;
 		return 0;
 	}
 
+	ctx->op_iter = kmalloc(sizeof(*ctx->op_iter), GFP_KERNEL);
+	if (!ctx->op_iter)
+		return -ENOMEM;
+	ctx->dump_map = genl_op_iter_init(rt, ctx->op_iter);
+
 	for (genl_op_iter_init(rt, &i); genl_op_iter_next(&i); ) {
 		if (i.doit.policy) {
 			err = netlink_policy_dump_add_policy(&ctx->state,
@@ -1345,12 +1352,16 @@ static int ctrl_dumppolicy_start(struct netlink_callback *cb)
 		}
 	}
 
-	if (!ctx->state)
-		return -ENODATA;
+	if (!ctx->state) {
+		err = -ENODATA;
+		goto err_free_op_iter;
+	}
 	return 0;
 
 err_free_state:
 	netlink_policy_dump_free(ctx->state);
+err_free_op_iter:
+	kfree(ctx->op_iter);
 	return err;
 }
 
@@ -1430,11 +1441,10 @@ static int ctrl_dumppolicy(struct sk_buff *skb, struct netlink_callback *cb)
 	struct ctrl_dump_policy_ctx *ctx = (void *)cb->ctx;
 	void *hdr;
 
-	if (!ctx->policies) {
-		struct genl_split_ops doit, dumpit;
-		struct genl_ops op;
-
+	if (ctx->dump_map) {
 		if (ctx->single_op) {
+			struct genl_split_ops doit, dumpit;
+
 			if (genl_get_cmd(ctx->op, GENL_CMD_CAP_DO,
 					 ctx->rt, &doit) &&
 			    genl_get_cmd(ctx->op, GENL_CMD_CAP_DUMP,
@@ -1446,26 +1456,18 @@ static int ctrl_dumppolicy(struct sk_buff *skb, struct netlink_callback *cb)
 			if (ctrl_dumppolicy_put_op(skb, cb, &doit, &dumpit))
 				return skb->len;
 
-			/* don't enter the loop below */
-			ctx->opidx = genl_get_cmd_cnt(ctx->rt);
+			/* done with the per-op policy index list */
+			ctx->dump_map = 0;
 		}
 
-		while (ctx->opidx < genl_get_cmd_cnt(ctx->rt)) {
-			genl_get_cmd_by_index(ctx->opidx, ctx->rt, &op);
-
-			genl_cmd_full_to_split(&doit, ctx->rt,
-					       &op, GENL_CMD_CAP_DO);
-			genl_cmd_full_to_split(&dumpit, ctx->rt,
-					       &op, GENL_CMD_CAP_DUMP);
-
-			if (ctrl_dumppolicy_put_op(skb, cb, &doit, &dumpit))
+		while (ctx->dump_map) {
+			if (ctrl_dumppolicy_put_op(skb, cb,
+						   &ctx->op_iter->doit,
+						   &ctx->op_iter->dumpit))
 				return skb->len;
 
-			ctx->opidx++;
+			ctx->dump_map = genl_op_iter_next(ctx->op_iter);
 		}
-
-		/* completed with the per-op policy index list */
-		ctx->policies = true;
 	}
 
 	while (netlink_policy_dump_loop(ctx->state)) {
@@ -1498,6 +1500,7 @@ static int ctrl_dumppolicy_done(struct netlink_callback *cb)
 {
 	struct ctrl_dump_policy_ctx *ctx = (void *)cb->ctx;
 
+	kfree(ctx->op_iter);
 	netlink_policy_dump_free(ctx->state);
 	return 0;
 }
-- 
2.38.1

[PATCH net-next v2 11/13] genetlink: inline old iteration helpers

[Jakub Kicinski]

All dumpers use the iterators now, inline the cmd by index
stuff into iterator code.

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/netlink/genetlink.c | 32 +++++++++-----------------------
 1 file changed, 9 insertions(+), 23 deletions(-)

diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index 4b8c65d9e9d3..0a4f1470f442 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -118,11 +118,6 @@ static const struct genl_family *genl_family_find_byname(char *name)
 	return NULL;
 }
 
-static int genl_get_cmd_cnt(const struct genl_family *family)
-{
-	return family->n_ops + family->n_small_ops;
-}
-
 static void genl_op_from_full(const struct genl_family *family,
 			      unsigned int i, struct genl_ops *op)
 {
@@ -240,18 +235,6 @@ genl_get_cmd(u32 cmd, u8 flags, const struct genl_family *family,
 	return genl_cmd_full_to_split(op, family, &full, flags);
 }
 
-static void genl_get_cmd_by_index(unsigned int i,
-				  const struct genl_family *family,
-				  struct genl_ops *op)
-{
-	if (i < family->n_ops)
-		genl_op_from_full(family, i, op);
-	else if (i < family->n_ops + family->n_small_ops)
-		genl_op_from_small(family, i - family->n_ops, op);
-	else
-		WARN_ON_ONCE(1);
-}
-
 struct genl_op_iter {
 	const struct genl_family *family;
 	struct genl_split_ops doit;
@@ -269,22 +252,25 @@ genl_op_iter_init(const struct genl_family *family, struct genl_op_iter *iter)
 
 	iter->flags = 0;
 
-	return genl_get_cmd_cnt(iter->family);
+	return iter->family->n_ops + iter->family->n_small_ops;
 }
 
 static bool genl_op_iter_next(struct genl_op_iter *iter)
 {
+	const struct genl_family *family = iter->family;
 	struct genl_ops op;
 
-	if (iter->i >= genl_get_cmd_cnt(iter->family))
+	if (iter->i < family->n_ops)
+		genl_op_from_full(family, iter->i, &op);
+	else if (iter->i < family->n_ops + family->n_small_ops)
+		genl_op_from_small(family, iter->i - family->n_ops, &op);
+	else
 		return false;
 
-	genl_get_cmd_by_index(iter->i, iter->family, &op);
 	iter->i++;
 
-	genl_cmd_full_to_split(&iter->doit, iter->family, &op, GENL_CMD_CAP_DO);
-	genl_cmd_full_to_split(&iter->dumpit, iter->family,
-			       &op, GENL_CMD_CAP_DUMP);
+	genl_cmd_full_to_split(&iter->doit, family, &op, GENL_CMD_CAP_DO);
+	genl_cmd_full_to_split(&iter->dumpit, family, &op, GENL_CMD_CAP_DUMP);
 
 	iter->cmd = iter->doit.cmd | iter->dumpit.cmd;
 	iter->flags = iter->doit.flags | iter->dumpit.flags;
-- 
2.38.1

[PATCH net-next v2 12/13] genetlink: allow families to use split ops directly

[Jakub Kicinski]

Let families to hook in the new split ops.

They are more flexible and should not be much larger than
full ops. Each split op is 40B while full op is 48B.
Devlink for example has 54 dos and 19 dumps, 2 of the dumps
do not have a do -> 56 full commands = 2688B.
Split ops would have taken 2920B, so 9% more space while
allowing individual per/post doit and per-type policies.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 include/net/genetlink.h |   5 ++
 net/netlink/genetlink.c | 158 +++++++++++++++++++++++++++++++++-------
 2 files changed, 137 insertions(+), 26 deletions(-)

diff --git a/include/net/genetlink.h b/include/net/genetlink.h
index 4be7989c451b..d21210709f84 100644
--- a/include/net/genetlink.h
+++ b/include/net/genetlink.h
@@ -46,6 +46,9 @@ struct genl_info;
  * @n_ops: number of operations supported by this family
  * @small_ops: the small-struct operations supported by this family
  * @n_small_ops: number of small-struct operations supported by this family
+ * @split_ops: the split do/dump form of operation definition
+ * @n_split_ops: number of entries in @split_ops, not that with split do/dump
+ *	ops the number of entries is not the same as number of commands
  *
  * Attribute policies (the combination of @policy and @maxattr fields)
  * can be attached at the family level or at the operation level.
@@ -63,6 +66,7 @@ struct genl_family {
 	u8			parallel_ops:1;
 	u8			n_ops;
 	u8			n_small_ops;
+	u8			n_split_ops;
 	u8			n_mcgrps;
 	u8			resv_start_op;
 	const struct nla_policy *policy;
@@ -74,6 +78,7 @@ struct genl_family {
 					     struct genl_info *info);
 	const struct genl_ops *	ops;
 	const struct genl_small_ops *small_ops;
+	const struct genl_split_ops *split_ops;
 	const struct genl_multicast_group *mcgrps;
 	struct module		*module;
 
diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index 0a4f1470f442..e95b984fcfe6 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -118,6 +118,16 @@ static const struct genl_family *genl_family_find_byname(char *name)
 	return NULL;
 }
 
+struct genl_op_iter {
+	const struct genl_family *family;
+	struct genl_split_ops doit;
+	struct genl_split_ops dumpit;
+	int cmd_idx;
+	int entry_idx;
+	u32 cmd;
+	u8 flags;
+};
+
 static void genl_op_from_full(const struct genl_family *family,
 			      unsigned int i, struct genl_ops *op)
 {
@@ -176,6 +186,47 @@ static int genl_get_cmd_small(u32 cmd, const struct genl_family *family,
 	return -ENOENT;
 }
 
+static void genl_op_from_split(struct genl_op_iter *iter)
+{
+	const struct genl_family *family = iter->family;
+	int i, cnt = 0;
+
+	i = iter->entry_idx - family->n_ops - family->n_small_ops;
+
+	if (family->split_ops[i + cnt].flags & GENL_CMD_CAP_DO) {
+		iter->doit = family->split_ops[i + cnt];
+		cnt++;
+	} else {
+		memset(&iter->doit, 0, sizeof(iter->doit));
+	}
+
+	if (family->split_ops[i + cnt].flags & GENL_CMD_CAP_DUMP) {
+		iter->dumpit = family->split_ops[i + cnt];
+		cnt++;
+	} else {
+		memset(&iter->dumpit, 0, sizeof(iter->dumpit));
+	}
+
+	WARN_ON(!cnt);
+	iter->entry_idx += cnt;
+}
+
+static int
+genl_get_cmd_split(u32 cmd, u8 flag, const struct genl_family *family,
+		   struct genl_split_ops *op)
+{
+	int i;
+
+	for (i = 0; i < family->n_split_ops; i++)
+		if (family->split_ops[i].cmd == cmd &&
+		    family->split_ops[i].flags & flag) {
+			*op = family->split_ops[i];
+			return 0;
+		}
+
+	return -ENOENT;
+}
+
 static int
 genl_cmd_full_to_split(struct genl_split_ops *op,
 		       const struct genl_family *family,
@@ -227,50 +278,60 @@ genl_get_cmd(u32 cmd, u8 flags, const struct genl_family *family,
 	err = genl_get_cmd_full(cmd, family, &full);
 	if (err == -ENOENT)
 		err = genl_get_cmd_small(cmd, family, &full);
-	if (err) {
-		memset(op, 0, sizeof(*op));
-		return err;
-	}
+	/* Found one of legacy forms */
+	if (err == 0)
+		return genl_cmd_full_to_split(op, family, &full, flags);
 
-	return genl_cmd_full_to_split(op, family, &full, flags);
+	err = genl_get_cmd_split(cmd, flags, family, op);
+	if (err)
+		memset(op, 0, sizeof(*op));
+	return err;
 }
 
-struct genl_op_iter {
-	const struct genl_family *family;
-	struct genl_split_ops doit;
-	struct genl_split_ops dumpit;
-	int i;
-	u32 cmd;
-	u8 flags;
-};
-
 static bool
 genl_op_iter_init(const struct genl_family *family, struct genl_op_iter *iter)
 {
 	iter->family = family;
-	iter->i = 0;
+	iter->cmd_idx = 0;
+	iter->entry_idx = 0;
 
 	iter->flags = 0;
 
-	return iter->family->n_ops + iter->family->n_small_ops;
+	return iter->family->n_ops +
+		iter->family->n_small_ops +
+		iter->family->n_split_ops;
 }
 
 static bool genl_op_iter_next(struct genl_op_iter *iter)
 {
 	const struct genl_family *family = iter->family;
+	bool legacy_op = true;
 	struct genl_ops op;
 
-	if (iter->i < family->n_ops)
-		genl_op_from_full(family, iter->i, &op);
-	else if (iter->i < family->n_ops + family->n_small_ops)
-		genl_op_from_small(family, iter->i - family->n_ops, &op);
-	else
+	if (iter->entry_idx < family->n_ops) {
+		genl_op_from_full(family, iter->entry_idx, &op);
+	} else if (iter->entry_idx < family->n_ops + family->n_small_ops) {
+		genl_op_from_small(family, iter->entry_idx - family->n_ops,
+				   &op);
+	} else if (iter->entry_idx <
+		   family->n_ops + family->n_small_ops + family->n_split_ops) {
+		legacy_op = false;
+		/* updates entry_idx */
+		genl_op_from_split(iter);
+	} else {
 		return false;
+	}
 
-	iter->i++;
+	iter->cmd_idx++;
 
-	genl_cmd_full_to_split(&iter->doit, family, &op, GENL_CMD_CAP_DO);
-	genl_cmd_full_to_split(&iter->dumpit, family, &op, GENL_CMD_CAP_DUMP);
+	if (legacy_op) {
+		iter->entry_idx++;
+
+		genl_cmd_full_to_split(&iter->doit, family,
+				       &op, GENL_CMD_CAP_DO);
+		genl_cmd_full_to_split(&iter->dumpit, family,
+				       &op, GENL_CMD_CAP_DUMP);
+	}
 
 	iter->cmd = iter->doit.cmd | iter->dumpit.cmd;
 	iter->flags = iter->doit.flags | iter->dumpit.flags;
@@ -286,7 +347,7 @@ genl_op_iter_copy(struct genl_op_iter *dst, struct genl_op_iter *src)
 
 static unsigned int genl_op_iter_idx(struct genl_op_iter *iter)
 {
-	return iter->i;
+	return iter->cmd_idx;
 }
 
 static int genl_allocate_reserve_groups(int n_groups, int *first_id)
@@ -454,12 +515,24 @@ static void genl_unregister_mc_groups(const struct genl_family *family)
 	}
 }
 
+static bool genl_split_op_check(const struct genl_split_ops *op)
+{
+	if (WARN_ON(hweight8(op->flags & (GENL_CMD_CAP_DO |
+					  GENL_CMD_CAP_DUMP)) != 1))
+		return true;
+	if (WARN_ON(!op->maxattr || !op->policy))
+		return true;
+	return false;
+}
+
 static int genl_validate_ops(const struct genl_family *family)
 {
 	struct genl_op_iter i, j;
+	unsigned int s;
 
 	if (WARN_ON(family->n_ops && !family->ops) ||
-	    WARN_ON(family->n_small_ops && !family->small_ops))
+	    WARN_ON(family->n_small_ops && !family->small_ops) ||
+	    WARN_ON(family->n_split_ops && !family->split_ops))
 		return -EINVAL;
 
 	for (genl_op_iter_init(family, &i); genl_op_iter_next(&i); ) {
@@ -477,6 +550,39 @@ static int genl_validate_ops(const struct genl_family *family)
 		}
 	}
 
+	if (family->n_split_ops) {
+		if (genl_split_op_check(&family->split_ops[0]))
+			return -EINVAL;
+	}
+
+	for (s = 1; s < family->n_split_ops; s++) {
+		const struct genl_split_ops *a, *b;
+
+		a = &family->split_ops[s - 1];
+		b = &family->split_ops[s];
+
+		if (genl_split_op_check(b))
+			return -EINVAL;
+
+		/* Check sort order */
+		if (a->cmd < b->cmd)
+			continue;
+
+		if (a->internal_flags != b->internal_flags ||
+		    ((a->flags ^ b->flags) & ~(GENL_CMD_CAP_DO |
+					       GENL_CMD_CAP_DUMP))) {
+			WARN_ON(1);
+			return -EINVAL;
+		}
+
+		if ((a->flags & GENL_CMD_CAP_DO) &&
+		    (b->flags & GENL_CMD_CAP_DUMP))
+			continue;
+
+		WARN_ON(1);
+		return -EINVAL;
+	}
+
 	return 0;
 }
 
-- 
2.38.1

[PATCH net-next v2 13/13] genetlink: convert control family to split ops

[Jakub Kicinski]

Prove that the split ops work.
Sadly we need to keep bug-wards compatibility and specify
the same policy for dump as do, even tho we don't parse
inputs for the dump.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/netlink/genetlink.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index e95b984fcfe6..1fb04496d94a 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -1597,14 +1597,22 @@ static int ctrl_dumppolicy_done(struct netlink_callback *cb)
 	return 0;
 }
 
-static const struct genl_ops genl_ctrl_ops[] = {
+static const struct genl_split_ops genl_ctrl_ops[] = {
 	{
 		.cmd		= CTRL_CMD_GETFAMILY,
-		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
+		.validate	= GENL_DONT_VALIDATE_STRICT,
 		.policy		= ctrl_policy_family,
 		.maxattr	= ARRAY_SIZE(ctrl_policy_family) - 1,
 		.doit		= ctrl_getfamily,
+		.flags		= GENL_CMD_CAP_DO,
+	},
+	{
+		.cmd		= CTRL_CMD_GETFAMILY,
+		.validate	= GENL_DONT_VALIDATE_DUMP,
+		.policy		= ctrl_policy_family,
+		.maxattr	= ARRAY_SIZE(ctrl_policy_family) - 1,
 		.dumpit		= ctrl_dumpfamily,
+		.flags		= GENL_CMD_CAP_DUMP,
 	},
 	{
 		.cmd		= CTRL_CMD_GETPOLICY,
@@ -1613,6 +1621,7 @@ static const struct genl_ops genl_ctrl_ops[] = {
 		.start		= ctrl_dumppolicy_start,
 		.dumpit		= ctrl_dumppolicy,
 		.done		= ctrl_dumppolicy_done,
+		.flags		= GENL_CMD_CAP_DUMP,
 	},
 };
 
@@ -1622,8 +1631,8 @@ static const struct genl_multicast_group genl_ctrl_groups[] = {
 
 static struct genl_family genl_ctrl __ro_after_init = {
 	.module = THIS_MODULE,
-	.ops = genl_ctrl_ops,
-	.n_ops = ARRAY_SIZE(genl_ctrl_ops),
+	.split_ops = genl_ctrl_ops,
+	.n_split_ops = ARRAY_SIZE(genl_ctrl_ops),
 	.resv_start_op = CTRL_CMD_GETPOLICY + 1,
 	.mcgrps = genl_ctrl_groups,
 	.n_mcgrps = ARRAY_SIZE(genl_ctrl_groups),
-- 
2.38.1

Re: [PATCH net-next v2 01/13] genetlink: refactor the cmd <> policy mapping dump

[Jacob Keller]

On 11/2/2022 2:33 PM, Jakub Kicinski wrote:
> The code at the top of ctrl_dumppolicy() dumps mappings between
> ops and policies. It supports dumping both the entire family and
> single op if dump is filtered. But both of those cases are handled
> inside a loop, which makes the logic harder to follow and change.
> Refactor to split the two cases more clearly.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> v2: bring the comment back
> ---
>   net/netlink/genetlink.c | 27 +++++++++++++++------------
>   1 file changed, 15 insertions(+), 12 deletions(-)
> 
> diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
> index 3e16527beb91..0a7a856e9ce0 100644
> --- a/net/netlink/genetlink.c
> +++ b/net/netlink/genetlink.c
> @@ -1319,21 +1319,24 @@ static int ctrl_dumppolicy(struct sk_buff *skb, struct netlink_callback *cb)
>   	void *hdr;
>   
>   	if (!ctx->policies) {
> -		while (ctx->opidx < genl_get_cmd_cnt(ctx->rt)) {
> -			struct genl_ops op;
> +		struct genl_ops op;
>   
> -			if (ctx->single_op) {
> -				int err;
> +		if (ctx->single_op) {
> +			int err;
>   
> -				err = genl_get_cmd(ctx->op, ctx->rt, &op);
> -				if (WARN_ON(err))
> -					return skb->len;
> +			err = genl_get_cmd(ctx->op, ctx->rt, &op);
> +			if (WARN_ON(err))
> +				return err;
>   
> -				/* break out of the loop after this one */
> -				ctx->opidx = genl_get_cmd_cnt(ctx->rt);
> -			} else {
> -				genl_get_cmd_by_index(ctx->opidx, ctx->rt, &op);
> -			}
> +			if (ctrl_dumppolicy_put_op(skb, cb, &op))
> +				return skb->len;
> +
> +			/* don't enter the loop below */
> +			ctx->opidx = genl_get_cmd_cnt(ctx->rt);
> +		}
> +
> +		while (ctx->opidx < genl_get_cmd_cnt(ctx->rt)) {
> +

Does the change to ctx->opidx have any other side effects we care about? 
if not it might be more legible to write this as:

/* don't modify ctx->opidx */
}

while (!ctx->single_op && ctx->opidx < genl_get_cmd_cnt(ctx->r)) {


That makes the intent a bit more clear and shouldn't need a comment 
about entering the loop. It also means we don't need to modify 
ctx->opidx, though I'm not sure if those other side effects matter or 
not.. we were modifying it before..

I don't know what else depends on the opidx.

Thanks,
Jake
			genl_get_cmd_by_index(ctx->opidx, ctx->rt, &op);
>   
>   			if (ctrl_dumppolicy_put_op(skb, cb, &op))
>   				return skb->len;

Re: [PATCH net-next v2 01/13] genetlink: refactor the cmd <> policy mapping dump

[Jakub Kicinski]

On Wed, 2 Nov 2022 16:52:21 -0700 Jacob Keller wrote:
> Does the change to ctx->opidx have any other side effects we care about? 
> if not it might be more legible to write this as:
> 
> /* don't modify ctx->opidx */
> }
> 
> while (!ctx->single_op && ctx->opidx < genl_get_cmd_cnt(ctx->r)) {
> 
> 
> That makes the intent a bit more clear and shouldn't need a comment 
> about entering the loop. It also means we don't need to modify 
> ctx->opidx, though I'm not sure if those other side effects matter or 
> not.. we were modifying it before..
> 
> I don't know what else depends on the opidx.

I was just trying to make the patches slightly easier to read.
This chunk gets rewritten again in patch 10, and the opidx thing 
is gone completely. I maintain a "keep dumping" boolean called
dump_map (because this code is dumping a mapping rather than 
the policies which come later)

LMK if I should try harder to improve this patch or what patch 10 
does makes this moot.

RE: [PATCH net-next v2 01/13] genetlink: refactor the cmd <> policy mapping dump

[Keller, Jacob E]

> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Wednesday, November 2, 2022 6:53 PM
> To: Keller, Jacob E <jacob.e.keller@intel.com>
> Cc: davem@davemloft.net; netdev@vger.kernel.org; edumazet@google.com;
> pabeni@redhat.com; jiri@resnulli.us; razor@blackwall.org;
> nicolas.dichtel@6wind.com; gnault@redhat.com; fw@strlen.de
> Subject: Re: [PATCH net-next v2 01/13] genetlink: refactor the cmd <> policy
> mapping dump
> 
> On Wed, 2 Nov 2022 16:52:21 -0700 Jacob Keller wrote:
> > Does the change to ctx->opidx have any other side effects we care about?
> > if not it might be more legible to write this as:
> >
> > /* don't modify ctx->opidx */
> > }
> >
> > while (!ctx->single_op && ctx->opidx < genl_get_cmd_cnt(ctx->r)) {
> >
> >
> > That makes the intent a bit more clear and shouldn't need a comment
> > about entering the loop. It also means we don't need to modify
> > ctx->opidx, though I'm not sure if those other side effects matter or
> > not.. we were modifying it before..
> >
> > I don't know what else depends on the opidx.
> 
> I was just trying to make the patches slightly easier to read.
> This chunk gets rewritten again in patch 10, and the opidx thing
> is gone completely. I maintain a "keep dumping" boolean called
> dump_map (because this code is dumping a mapping rather than
> the policies which come later)
> 
> LMK if I should try harder to improve this patch or what patch 10
> does makes this moot.

If patch 10 makes things moot lets go with this as-is.

Thanks,
Jake

Re: [PATCH net-next v2 08/13] genetlink: inline genl_get_cmd()

[Jacob Keller]

On 11/2/2022 2:33 PM, Jakub Kicinski wrote:
> All callers go via genl_get_cmd_split() now,
> so merge genl_get_cmd() into it.

In some sense this is merging genl_get_cmd_split into genl_get_cmd since 
thats the name we end up with, but practically/code-wise its merging the 
other way then renaming back to genl_get_cmd.

Still looks good.
> 
> Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>   net/netlink/genetlink.c | 30 ++++++++++++------------------
>   1 file changed, 12 insertions(+), 18 deletions(-)
> 
> diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
> index 93e33e20a0e8..ec32b6063a3f 100644
> --- a/net/netlink/genetlink.c
> +++ b/net/netlink/genetlink.c
> @@ -181,14 +181,6 @@ static int genl_get_cmd_small(u32 cmd, const struct genl_family *family,
>   	return -ENOENT;
>   }
>   
> -static int genl_get_cmd(u32 cmd, const struct genl_family *family,
> -			struct genl_ops *op)
> -{
> -	if (!genl_get_cmd_full(cmd, family, op))
> -		return 0;
> -	return genl_get_cmd_small(cmd, family, op);
> -}
> -
>   static int
>   genl_cmd_full_to_split(struct genl_split_ops *op,
>   		       const struct genl_family *family,
> @@ -231,13 +223,15 @@ genl_cmd_full_to_split(struct genl_split_ops *op,
>   }
>   
>   static int
> -genl_get_cmd_split(u32 cmd, u8 flags, const struct genl_family *family,
> -		   struct genl_split_ops *op)
> +genl_get_cmd(u32 cmd, u8 flags, const struct genl_family *family,
> +	     struct genl_split_ops *op)
>   {
>   	struct genl_ops full;
>   	int err;
>   
> -	err = genl_get_cmd(cmd, family, &full);
> +	err = genl_get_cmd_full(cmd, family, &full);
> +	if (err == -ENOENT)
> +		err = genl_get_cmd_small(cmd, family, &full);
>   	if (err) {
>   		memset(op, 0, sizeof(*op));
>   		return err;
> @@ -867,7 +861,7 @@ static int genl_family_rcv_msg(const struct genl_family *family,
>   
>   	flags = (nlh->nlmsg_flags & NLM_F_DUMP) == NLM_F_DUMP ?
>   		GENL_CMD_CAP_DUMP : GENL_CMD_CAP_DO;
> -	if (genl_get_cmd_split(hdr->cmd, flags, family, &op))
> +	if (genl_get_cmd(hdr->cmd, flags, family, &op))
>   		return -EOPNOTSUPP;
>   
>   	if ((op.flags & GENL_ADMIN_PERM) &&
> @@ -1265,8 +1259,8 @@ static int ctrl_dumppolicy_start(struct netlink_callback *cb)
>   		ctx->single_op = true;
>   		ctx->op = nla_get_u32(tb[CTRL_ATTR_OP]);
>   
> -		if (genl_get_cmd_split(ctx->op, GENL_CMD_CAP_DO, rt, &doit) &&
> -		    genl_get_cmd_split(ctx->op, GENL_CMD_CAP_DUMP, rt, &dump)) {
> +		if (genl_get_cmd(ctx->op, GENL_CMD_CAP_DO, rt, &doit) &&
> +		    genl_get_cmd(ctx->op, GENL_CMD_CAP_DUMP, rt, &dump)) {
>   			NL_SET_BAD_ATTR(cb->extack, tb[CTRL_ATTR_OP]);
>   			return -ENOENT;
>   		}
> @@ -1406,10 +1400,10 @@ static int ctrl_dumppolicy(struct sk_buff *skb, struct netlink_callback *cb)
>   		struct genl_ops op;
>   
>   		if (ctx->single_op) {
> -			if (genl_get_cmd_split(ctx->op, GENL_CMD_CAP_DO,
> -					       ctx->rt, &doit) &&
> -			    genl_get_cmd_split(ctx->op, GENL_CMD_CAP_DUMP,
> -					       ctx->rt, &dumpit)) {
> +			if (genl_get_cmd(ctx->op, GENL_CMD_CAP_DO,
> +					 ctx->rt, &doit) &&
> +			    genl_get_cmd(ctx->op, GENL_CMD_CAP_DUMP,
> +					 ctx->rt, &dumpit)) {
>   				WARN_ON(1);
>   				return -ENOENT;
>   			}

Re: [PATCH net-next v2 00/13] genetlink: support per op type policies

[Jacob Keller]

On 11/2/2022 2:33 PM, Jakub Kicinski wrote:
> While writing new genetlink families I was increasingly annoyed by the fact
> that we don't support different policies for do and dump callbacks.
> This makes it hard to do proper input validation for dumps which usually
> have a lot more narrow range of accepted attributes.
> 
> There is also a minor inconvenience of not supporting different per_doit
> and post_doit callbacks per op.
> 
> This series addresses those problems by introducing another op format.
> 
> v2:
>   - wait for net changes to propagate
>   - restore the missing comment in patch 1
>   - drop extra space in patch 3
>   - improve commit message in patch 4


The whole series looks good to me, thanks for improving the commit 
messages, it helps process the changes better. I like the end result, 
and it gets us some good ability without too much cost in size.


Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

Thanks,
Jake

> v1: https://lore.kernel.org/all/20221018230728.1039524-1-kuba@kernel.org/
> 
> Jakub Kicinski (13):
>    genetlink: refactor the cmd <> policy mapping dump
>    genetlink: move the private fields in struct genl_family
>    genetlink: introduce split op representation
>    genetlink: load policy based on validation flags
>    genetlink: check for callback type at op load time
>    genetlink: add policies for both doit and dumpit in
>      ctrl_dumppolicy_start()
>    genetlink: support split policies in ctrl_dumppolicy_put_op()
>    genetlink: inline genl_get_cmd()
>    genetlink: add iterator for walking family ops
>    genetlink: use iterator in the op to policy map dumping
>    genetlink: inline old iteration helpers
>    genetlink: allow families to use split ops directly
>    genetlink: convert control family to split ops
> 
>   include/net/genetlink.h   |  76 +++++-
>   net/batman-adv/netlink.c  |   6 +-
>   net/core/devlink.c        |   4 +-
>   net/core/drop_monitor.c   |   4 +-
>   net/ieee802154/nl802154.c |   6 +-
>   net/netlink/genetlink.c   | 473 ++++++++++++++++++++++++++++----------
>   net/wireless/nl80211.c    |   6 +-
>   7 files changed, 433 insertions(+), 142 deletions(-)
> 

Re: [PATCH net-next v2 06/13] genetlink: add policies for both doit and dumpit in ctrl_dumppolicy_start()

[Jacob Keller]

On 11/2/2022 2:33 PM, Jakub Kicinski wrote:
> Separate adding doit and dumpit policies for CTRL_CMD_GETPOLICY.
> This has no effect until we actually allow do and dump to come
> from different sources as netlink_policy_dump_add_policy()
> does deduplication.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>   net/netlink/genetlink.c | 48 ++++++++++++++++++++++++++++++++---------
>   1 file changed, 38 insertions(+), 10 deletions(-)
> 
> diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
> index 7c04df1bee2b..d0c35738839b 100644
> --- a/net/netlink/genetlink.c
> +++ b/net/netlink/genetlink.c
> @@ -1260,29 +1260,57 @@ static int ctrl_dumppolicy_start(struct netlink_callback *cb)
>   	ctx->rt = rt;
>   
>   	if (tb[CTRL_ATTR_OP]) {
> +		struct genl_split_ops doit, dump;
> +
>   		ctx->single_op = true;
>   		ctx->op = nla_get_u32(tb[CTRL_ATTR_OP]);
>   
> -		err = genl_get_cmd(ctx->op, rt, &op);
> -		if (err) {
> +		if (genl_get_cmd_split(ctx->op, GENL_CMD_CAP_DO, rt, &doit) &&
> +		    genl_get_cmd_split(ctx->op, GENL_CMD_CAP_DUMP, rt, &dump)) {
>   			NL_SET_BAD_ATTR(cb->extack, tb[CTRL_ATTR_OP]);
> -			return err;
> +			return -ENOENT;

We do lose the specific error of genl_get_cmd_split here, but honestly 
not a big deal since I think it is always ENOENT anyways

Re: [PATCH net-next v2 12/13] genetlink: allow families to use split ops directly

[Nicolas Dichtel]

Le 02/11/2022 à 22:33, Jakub Kicinski a écrit :
> Let families to hook in the new split ops.
> 
> They are more flexible and should not be much larger than
> full ops. Each split op is 40B while full op is 48B.
> Devlink for example has 54 dos and 19 dumps, 2 of the dumps
> do not have a do -> 56 full commands = 2688B.
> Split ops would have taken 2920B, so 9% more space while
> allowing individual per/post doit and per-type policies.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  include/net/genetlink.h |   5 ++
>  net/netlink/genetlink.c | 158 +++++++++++++++++++++++++++++++++-------
>  2 files changed, 137 insertions(+), 26 deletions(-)
> 
> diff --git a/include/net/genetlink.h b/include/net/genetlink.h
> index 4be7989c451b..d21210709f84 100644
> --- a/include/net/genetlink.h
> +++ b/include/net/genetlink.h
> @@ -46,6 +46,9 @@ struct genl_info;
>   * @n_ops: number of operations supported by this family
>   * @small_ops: the small-struct operations supported by this family
>   * @n_small_ops: number of small-struct operations supported by this family
> + * @split_ops: the split do/dump form of operation definition
> + * @n_split_ops: number of entries in @split_ops, not that with split do/dump
> + *	ops the number of entries is not the same as number of commands
>   *
>   * Attribute policies (the combination of @policy and @maxattr fields)
>   * can be attached at the family level or at the operation level.
> @@ -63,6 +66,7 @@ struct genl_family {
>  	u8			parallel_ops:1;
>  	u8			n_ops;
>  	u8			n_small_ops;
> +	u8			n_split_ops;
>  	u8			n_mcgrps;
>  	u8			resv_start_op;
>  	const struct nla_policy *policy;
> @@ -74,6 +78,7 @@ struct genl_family {
>  					     struct genl_info *info);
>  	const struct genl_ops *	ops;
>  	const struct genl_small_ops *small_ops;
> +	const struct genl_split_ops *split_ops;
>  	const struct genl_multicast_group *mcgrps;
>  	struct module		*module;
>  
> diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
> index 0a4f1470f442..e95b984fcfe6 100644
> --- a/net/netlink/genetlink.c
> +++ b/net/netlink/genetlink.c
> @@ -118,6 +118,16 @@ static const struct genl_family *genl_family_find_byname(char *name)
>  	return NULL;
>  }
>  
> +struct genl_op_iter {
> +	const struct genl_family *family;
> +	struct genl_split_ops doit;
> +	struct genl_split_ops dumpit;
> +	int cmd_idx;
> +	int entry_idx;
> +	u32 cmd;
> +	u8 flags;
> +};
> +
>  static void genl_op_from_full(const struct genl_family *family,
>  			      unsigned int i, struct genl_ops *op)
>  {
> @@ -176,6 +186,47 @@ static int genl_get_cmd_small(u32 cmd, const struct genl_family *family,
>  	return -ENOENT;
>  }
>  
> +static void genl_op_from_split(struct genl_op_iter *iter)
> +{
> +	const struct genl_family *family = iter->family;
> +	int i, cnt = 0;
> +
> +	i = iter->entry_idx - family->n_ops - family->n_small_ops;
> +
> +	if (family->split_ops[i + cnt].flags & GENL_CMD_CAP_DO) {
> +		iter->doit = family->split_ops[i + cnt];
> +		cnt++;
> +	} else {
> +		memset(&iter->doit, 0, sizeof(iter->doit));
> +	}
> +
> +	if (family->split_ops[i + cnt].flags & GENL_CMD_CAP_DUMP) {
> +		iter->dumpit = family->split_ops[i + cnt];
> +		cnt++;
> +	} else {
> +		memset(&iter->dumpit, 0, sizeof(iter->dumpit));
> +	}
> +
> +	WARN_ON(!cnt);
> +	iter->entry_idx += cnt;
> +}
> +
> +static int
> +genl_get_cmd_split(u32 cmd, u8 flag, const struct genl_family *family,
> +		   struct genl_split_ops *op)
> +{
> +	int i;
> +
> +	for (i = 0; i < family->n_split_ops; i++)
> +		if (family->split_ops[i].cmd == cmd &&
> +		    family->split_ops[i].flags & flag) {
> +			*op = family->split_ops[i];
> +			return 0;
> +		}
> +
> +	return -ENOENT;
> +}
> +
>  static int
>  genl_cmd_full_to_split(struct genl_split_ops *op,
>  		       const struct genl_family *family,
> @@ -227,50 +278,60 @@ genl_get_cmd(u32 cmd, u8 flags, const struct genl_family *family,
>  	err = genl_get_cmd_full(cmd, family, &full);
>  	if (err == -ENOENT)
>  		err = genl_get_cmd_small(cmd, family, &full);
> -	if (err) {
> -		memset(op, 0, sizeof(*op));
> -		return err;
> -	}
> +	/* Found one of legacy forms */
> +	if (err == 0)
> +		return genl_cmd_full_to_split(op, family, &full, flags);
>  
> -	return genl_cmd_full_to_split(op, family, &full, flags);
> +	err = genl_get_cmd_split(cmd, flags, family, op);
> +	if (err)
> +		memset(op, 0, sizeof(*op));
> +	return err;
>  }
>  
> -struct genl_op_iter {
> -	const struct genl_family *family;
> -	struct genl_split_ops doit;
> -	struct genl_split_ops dumpit;
> -	int i;
> -	u32 cmd;
> -	u8 flags;
> -};
> -
>  static bool
>  genl_op_iter_init(const struct genl_family *family, struct genl_op_iter *iter)
>  {
>  	iter->family = family;
> -	iter->i = 0;
> +	iter->cmd_idx = 0;
> +	iter->entry_idx = 0;
>  
>  	iter->flags = 0;
>  
> -	return iter->family->n_ops + iter->family->n_small_ops;
> +	return iter->family->n_ops +
> +		iter->family->n_small_ops +
> +		iter->family->n_split_ops;
>  }
>  
>  static bool genl_op_iter_next(struct genl_op_iter *iter)
>  {
>  	const struct genl_family *family = iter->family;
> +	bool legacy_op = true;
>  	struct genl_ops op;
>  
> -	if (iter->i < family->n_ops)
> -		genl_op_from_full(family, iter->i, &op);
> -	else if (iter->i < family->n_ops + family->n_small_ops)
> -		genl_op_from_small(family, iter->i - family->n_ops, &op);
> -	else
> +	if (iter->entry_idx < family->n_ops) {
> +		genl_op_from_full(family, iter->entry_idx, &op);
> +	} else if (iter->entry_idx < family->n_ops + family->n_small_ops) {
> +		genl_op_from_small(family, iter->entry_idx - family->n_ops,
> +				   &op);
> +	} else if (iter->entry_idx <
> +		   family->n_ops + family->n_small_ops + family->n_split_ops) {
> +		legacy_op = false;
> +		/* updates entry_idx */
> +		genl_op_from_split(iter);
> +	} else {
>  		return false;
> +	}
>  
> -	iter->i++;
> +	iter->cmd_idx++;
>  
> -	genl_cmd_full_to_split(&iter->doit, family, &op, GENL_CMD_CAP_DO);
> -	genl_cmd_full_to_split(&iter->dumpit, family, &op, GENL_CMD_CAP_DUMP);
> +	if (legacy_op) {
> +		iter->entry_idx++;
> +
> +		genl_cmd_full_to_split(&iter->doit, family,
> +				       &op, GENL_CMD_CAP_DO);
> +		genl_cmd_full_to_split(&iter->dumpit, family,
> +				       &op, GENL_CMD_CAP_DUMP);
> +	}
>  
>  	iter->cmd = iter->doit.cmd | iter->dumpit.cmd;
>  	iter->flags = iter->doit.flags | iter->dumpit.flags;
> @@ -286,7 +347,7 @@ genl_op_iter_copy(struct genl_op_iter *dst, struct genl_op_iter *src)
>  
>  static unsigned int genl_op_iter_idx(struct genl_op_iter *iter)
>  {
> -	return iter->i;
> +	return iter->cmd_idx;
>  }
>  
>  static int genl_allocate_reserve_groups(int n_groups, int *first_id)
> @@ -454,12 +515,24 @@ static void genl_unregister_mc_groups(const struct genl_family *family)
>  	}
>  }
>  
> +static bool genl_split_op_check(const struct genl_split_ops *op)
> +{
> +	if (WARN_ON(hweight8(op->flags & (GENL_CMD_CAP_DO |
> +					  GENL_CMD_CAP_DUMP)) != 1))
> +		return true;
> +	if (WARN_ON(!op->maxattr || !op->policy))
> +		return true;
> +	return false;
> +}
> +
>  static int genl_validate_ops(const struct genl_family *family)
>  {
>  	struct genl_op_iter i, j;
> +	unsigned int s;
>  
>  	if (WARN_ON(family->n_ops && !family->ops) ||
> -	    WARN_ON(family->n_small_ops && !family->small_ops))
> +	    WARN_ON(family->n_small_ops && !family->small_ops) ||
> +	    WARN_ON(family->n_split_ops && !family->split_ops))
>  		return -EINVAL;
>  
>  	for (genl_op_iter_init(family, &i); genl_op_iter_next(&i); ) {
> @@ -477,6 +550,39 @@ static int genl_validate_ops(const struct genl_family *family)
>  		}
>  	}
>  
> +	if (family->n_split_ops) {
> +		if (genl_split_op_check(&family->split_ops[0]))
> +			return -EINVAL;
> +	}
> +
> +	for (s = 1; s < family->n_split_ops; s++) {
> +		const struct genl_split_ops *a, *b;
> +
> +		a = &family->split_ops[s - 1];
> +		b = &family->split_ops[s];
> +
> +		if (genl_split_op_check(b))
> +			return -EINVAL;
> +
> +		/* Check sort order */
> +		if (a->cmd < b->cmd)
> +			continue;
If I understand correctly, the goal of the below checks, between a and b, is to
enforce flags consitency between the do and the dump.
Does this work if the cmds in the struct genl_split_ops are declared randomly (
ie the do and the dump are separated by another cmd)?

Except that, for the whole series:
Reviewed-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>

> +
> +		if (a->internal_flags != b->internal_flags ||
> +		    ((a->flags ^ b->flags) & ~(GENL_CMD_CAP_DO |
> +					       GENL_CMD_CAP_DUMP))) {
> +			WARN_ON(1);
> +			return -EINVAL;
> +		}
> +
> +		if ((a->flags & GENL_CMD_CAP_DO) &&
> +		    (b->flags & GENL_CMD_CAP_DUMP))
> +			continue;
> +
> +		WARN_ON(1);
> +		return -EINVAL;
> +	}
> +
>  	return 0;
>  }
>  

Re: [PATCH net-next v2 12/13] genetlink: allow families to use split ops directly

[Jakub Kicinski]

On Fri, 4 Nov 2022 23:10:57 +0100 Nicolas Dichtel wrote:
> > +		/* Check sort order */
> > +		if (a->cmd < b->cmd)
> > +			continue;  
> If I understand correctly, the goal of the below checks, between a and b, is to
> enforce flags consitency between the do and the dump.
> Does this work if the cmds in the struct genl_split_ops are declared randomly (
> ie the do and the dump are separated by another cmd)?

I'm trying to go further and enforce sort order as weel (see comment
above the check), so that we can use binary search if we ever get to 
a large enough family for it to matter.

Re: [PATCH net-next v2 12/13] genetlink: allow families to use split ops directly

[Nicolas Dichtel]

Le 04/11/2022 à 23:19, Jakub Kicinski a écrit :
> On Fri, 4 Nov 2022 23:10:57 +0100 Nicolas Dichtel wrote:
>>> +		/* Check sort order */
>>> +		if (a->cmd < b->cmd)
>>> +			continue;  
>> If I understand correctly, the goal of the below checks, between a and b, is to
>> enforce flags consitency between the do and the dump.
>> Does this work if the cmds in the struct genl_split_ops are declared randomly (
>> ie the do and the dump are separated by another cmd)?
> 
> I'm trying to go further and enforce sort order as weel (see comment
> above the check), so that we can use binary search if we ever get to 
> a large enough family for it to matter.
Ok, thanks for the details. TBH, the comment before the check confuses me, I
read it as an explanation of the check, not like a TODO ;-)