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

[PATCH net-next 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.

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   | 461 ++++++++++++++++++++++++++++----------
 net/wireless/nl80211.c    |   6 +-
 7 files changed, 426 insertions(+), 137 deletions(-)

-- 
2.37.3

[PATCH net-next 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>
---
 net/netlink/genetlink.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index 39b7c00e4cef..43cc31fb3d25 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -1294,21 +1294,23 @@ 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;
+
+			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.37.3

[PATCH net-next 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>
---
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 8f780170e2f8..f2366b463597 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
@@ -41,20 +40,16 @@ struct genl_info;
  * @n_mcgrps: number of multicast groups
  * @resv_start_op: first operation for which reserved fields of the header
  *	can be validated, 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
  * @n_small_ops: number of small-struct operations supported by this family
  */
 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;
@@ -72,6 +67,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.37.3

[PATCH net-next 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>
---
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   | 70 +++++++++++++++++++++++++++++++++------
 net/wireless/nl80211.c    |  6 ++--
 7 files changed, 131 insertions(+), 25 deletions(-)

diff --git a/include/net/genetlink.h b/include/net/genetlink.h
index f2366b463597..373a99984cfe 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;
 
 /**
@@ -57,10 +57,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;
@@ -173,6 +173,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_info - info that is available during dumpit op call
  * @family: generic netlink family - for internal genl code usage
@@ -181,7 +233,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 89baa7c0938b..744d5879914f 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 f084a4a6b7ab..cb25e58e3882 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 43cc31fb3d25..23d3682d8f94 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -166,6 +166,50 @@ 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;
+
+	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)
@@ -519,7 +563,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)
 {
@@ -555,18 +599,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;
 
@@ -608,7 +653,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();
@@ -620,7 +665,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) {
@@ -636,7 +681,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)
@@ -650,7 +695,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;
@@ -696,7 +741,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;
@@ -776,8 +821,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))
@@ -790,7 +836,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) &&
@@ -801,7 +849,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 597c52236514..250305061543 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -16139,7 +16139,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;
@@ -16240,7 +16241,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.37.3

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

[Jakub Kicinski]

Set the policy and maxattr pointers based on validation flags.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 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 23d3682d8f94..26ddbd23549d 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -181,8 +181,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;
@@ -612,10 +618,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,
@@ -624,7 +628,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.37.3

[PATCH net-next 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 26ddbd23549d..9dfb3cf89b97 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -166,11 +166,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;
@@ -196,6 +202,8 @@ genl_cmd_full_to_split(struct genl_split_ops *op,
 	op->validate		= full->validate;
 
 	op->flags		|= flags;
+
+	return 0;
 }
 
 static int
@@ -211,9 +219,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,
@@ -704,9 +710,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;
@@ -751,9 +754,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.37.3

[PATCH net-next 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 9dfb3cf89b97..234b27977013 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -1234,29 +1234,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.37.3

[PATCH net-next 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.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 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 234b27977013..3e821c346577 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -1319,7 +1319,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;
@@ -1327,10 +1328,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);
@@ -1341,21 +1339,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);
@@ -1373,16 +1376,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;
 
 			ctx->opidx = genl_get_cmd_cnt(ctx->rt);
@@ -1391,7 +1397,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.37.3

[PATCH net-next 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.

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 3e821c346577..9bfb110053c7 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -158,14 +158,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,
@@ -207,13 +199,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;
@@ -841,7 +835,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) &&
@@ -1239,8 +1233,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;
 		}
@@ -1380,10 +1374,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.37.3

[PATCH net-next 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 | 114 ++++++++++++++++++++++++++--------------
 1 file changed, 74 insertions(+), 40 deletions(-)

diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index 9bfb110053c7..c5fcb7b9c383 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -193,6 +193,7 @@ genl_cmd_full_to_split(struct genl_split_ops *op,
 	op->flags		= full->flags;
 	op->validate		= full->validate;
 
+	/* Make sure flags include the GENL_CMD_CAP_DO / GENL_CMD_CAP_DUMP */
 	op->flags		|= flags;
 
 	return 0;
@@ -228,6 +229,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;
@@ -395,23 +447,19 @@ 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;
-		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;
 		}
 	}
@@ -891,6 +939,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);
@@ -904,33 +953,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;
 
@@ -1203,8 +1245,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));
 
@@ -1259,26 +1301,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.37.3

[PATCH net-next 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.

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

diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index c5fcb7b9c383..63807204805a 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -1225,10 +1225,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;
 };
 
@@ -1298,9 +1298,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,
@@ -1318,12 +1325,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;
 }
 
@@ -1403,11 +1414,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,
@@ -1419,25 +1429,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;
 
-			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)) {
@@ -1470,6 +1473,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.37.3

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

[Jakub Kicinski]

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

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 63807204805a..301981bae83d 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -99,11 +99,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)
 {
@@ -217,18 +212,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;
@@ -246,22 +229,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.37.3

[PATCH net-next 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 note 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 373a99984cfe..38ad0dc89240 100644
--- a/include/net/genetlink.h
+++ b/include/net/genetlink.h
@@ -44,6 +44,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
  */
 struct genl_family {
 	unsigned int		hdrsize;
@@ -54,6 +57,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;
@@ -65,6 +69,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 301981bae83d..53cc5cfcdc57 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -99,6 +99,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)
 {
@@ -153,6 +163,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,
@@ -204,50 +255,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;
@@ -263,7 +324,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)
@@ -431,12 +492,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); ) {
@@ -450,6 +523,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.37.3

[PATCH net-next 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 53cc5cfcdc57..05b5e22561f2 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -1570,14 +1570,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,
@@ -1586,6 +1594,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,
 	},
 };
 
@@ -1595,8 +1604,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.37.3

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

[Johannes Berg]

On Tue, 2022-10-18 at 16:07 -0700, 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.

Hmm. Yeah, fair, it's nicer now :)

However,

>  	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;
> +
> +			ctx->opidx = genl_get_cmd_cnt(ctx->rt);

This (now without a comment that you removed rather than changed), still
strikes me as odd.

I guess if we add a comment /* don't enter the loop below */ that'd be
nicer, but I feel maybe putting the loop into the else instead would be
nicer?


johannes

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

[Johannes Berg]

On Tue, 2022-10-18 at 16:07 -0700, Jakub Kicinski wrote:
> 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>
> ---
> I did this cleanup to add more private fields but ended up
> not needing them. Still I think the commit makes sense?
> 

Agree.

Reviewed-by: Johannes Berg <johannes@sipsolutions.net>

johannes

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

[Johannes Berg]

On Tue, 2022-10-18 at 16:07 -0700, Jakub Kicinski wrote:
> 
> +/**
> + * 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
> +  *

nit: extra space here

> + * 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


Is that really worth it? I mean, if you need pre/post for a *specific*
op, you can just roll that into it.

Maybe the use case would be something like "groups" where some commands
need one set of pre/post, and some other commands need another set, and
then it's still simpler to do as pre/post rather than calling them
inside the doit()?

(and you also have space for the pointers given the dump part of the
union, so ...)

> +static void
> +genl_cmd_full_to_split(struct genl_split_ops *op,
> +		       const struct genl_family *family,
> +		       const struct genl_ops *full, u8 flags)
> +{

[...]


> +	op->flags		|= flags;


why |= ?

> @@ -776,8 +821,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;

it's not even initialized?


> +	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;

before being used

or am I misreading something?

johannes

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

[Johannes Berg]

On Tue, 2022-10-18 at 16:07 -0700, Jakub Kicinski wrote:
> Set the policy and maxattr pointers based on validation flags.


I feel like you could have more commit message here

>  	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,

especially since this actually changes things to *have* the attrs, but
not have *validated* them, which feels a bit strange and error-prone in
the future maybe?

johannes

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

[Johannes Berg]

On Tue, 2022-10-18 at 16:07 -0700, Jakub Kicinski wrote:
> 
>  
> -		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) {

nit: to me a blank line in places like that would be nicer, but ymmv

>  	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) {

same here

johannes

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

[Johannes Berg]

On Tue, 2022-10-18 at 16:07 -0700, Jakub Kicinski wrote:
> Let families to hook in the new split ops.
> 
> They are more flexible and should note be much larger than

"note" -> "not"

> 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.

You mean "Full ops would have [...] while split ops allow individual
[...]" or so?

johannes

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

[Jakub Kicinski]

On Wed, 19 Oct 2022 09:50:16 +0200 Johannes Berg wrote:
> > +			if (ctrl_dumppolicy_put_op(skb, cb, &op))
> > +				return skb->len;
> > +
> > +			ctx->opidx = genl_get_cmd_cnt(ctx->rt);  
> 
> This (now without a comment that you removed rather than changed), still
> strikes me as odd.
> 
> I guess if we add a comment /* don't enter the loop below */ that'd be
> nicer, but I feel maybe putting the loop into the else instead would be
> nicer?

The comment got eaten in rebases, it comes back in patch 10 apparently..
I'll put it back in v2.

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

[Jakub Kicinski]

On Wed, 19 Oct 2022 09:59:24 +0200 Johannes Berg wrote:
> On Tue, 2022-10-18 at 16:07 -0700, Jakub Kicinski wrote:
> > + * 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  
> 
> Is that really worth it? I mean, if you need pre/post for a *specific*
> op, you can just roll that into it.
> 
> Maybe the use case would be something like "groups" where some commands
> need one set of pre/post, and some other commands need another set, and

Exactly - groups of ops. Different groups of ops need different
handling. But as I think I mentioned in the commit messages the 
separate policies are the real reason, this is just done "while 
at it".

> then it's still simpler to do as pre/post rather than calling them
> inside the doit()?

A little bit, it simplifies the unwind in case you need to take some
references and some locks you save dealing with success path vs error 
path handling in the doit.

> (and you also have space for the pointers given the dump part of the
> union, so ...)

Yes, we have the space... I think I lost your thread of thought..
Do you want to define more info for each group than just the pre/post?

> > +static void
> > +genl_cmd_full_to_split(struct genl_split_ops *op,
> > +		       const struct genl_family *family,
> > +		       const struct genl_ops *full, u8 flags)
> > +{  
> 
> [...]
> 
> 
> > +	op->flags		|= flags;  
> 
> why |= ?

op->flags should already have all the existing flags (i.e. ADMIN_PERM)
from the op, I'm adding the DO/DUMP to them.

> > @@ -776,8 +821,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;  
> 
> it's not even initialized?
> 
> > +	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;  
> 
> before being used
> 
> or am I misreading something?

It's used as an output argument here, so that's what initializes it.
genl_get_cmd* should always init the split command because in policy
dumping we don't care about the errors, we just want the structure
to be zeroed if do/dump is not implemented, and we'll skip accordingly.
Wiping the 40B just to write all the fields felt... wrong. 
Let KASAN catch us if we fail to init something.

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

[Jakub Kicinski]

On Wed, 19 Oct 2022 10:01:04 +0200 Johannes Berg wrote:
> On Tue, 2022-10-18 at 16:07 -0700, Jakub Kicinski wrote:
> > Set the policy and maxattr pointers based on validation flags.  
> 
> I feel like you could have more commit message here
> 
> >  	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,  
> 
> especially since this actually changes things to *have* the attrs, but
> not have *validated* them, which feels a bit strange and error-prone in
> the future maybe?

Do you mean that we no longer populate op->maxattr / op->policy and
some op may be reading those? I don't see any family code looking at
info->op.policy / maxattr.

First thing genl_family_rcv_msg_attrs_parse() does is:

	if (!ops->maxattr)
		return NULL;

So whether we skip it or call it - no difference.

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

[Jakub Kicinski]

On Wed, 19 Oct 2022 10:08:01 +0200 Johannes Berg wrote:
> > +		}
> > +		if (dump.policy) {  
> 
> nit: to me a blank line in places like that would be nicer, but ymmv

mmmm, if you don't mind i'll keep as is... :)

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

[Jakub Kicinski]

On Wed, 19 Oct 2022 10:15:05 +0200 Johannes Berg wrote:
> > 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.  
> 
> You mean "Full ops would have [...] while split ops allow individual
> [...]" or so?

Split ops end up being larger as we need a separate entry for each 
do and dump. So I think it's right?

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

[Johannes Berg]

On Wed, 2022-10-19 at 12:20 -0700, Jakub Kicinski wrote:
> On Wed, 19 Oct 2022 10:01:04 +0200 Johannes Berg wrote:
> > On Tue, 2022-10-18 at 16:07 -0700, Jakub Kicinski wrote:
> > > Set the policy and maxattr pointers based on validation flags.  
> > 
> > I feel like you could have more commit message here
> > 
> > >  	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,  
> > 
> > especially since this actually changes things to *have* the attrs, but
> > not have *validated* them, which feels a bit strange and error-prone in
> > the future maybe?
> 
> Do you mean that we no longer populate op->maxattr / op->policy and
> some op may be reading those? I don't see any family code looking at
> info->op.policy / maxattr.
> 
> First thing genl_family_rcv_msg_attrs_parse() does is:
> 
> 	if (!ops->maxattr)
> 		return NULL;
> 
> So whether we skip it or call it - no difference.
> 

Oh. I missed that, ok, thanks for clearing that up!

johannes

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

[Johannes Berg]

On Wed, 2022-10-19 at 12:14 -0700, Jakub Kicinski wrote:
> 
> Yes, we have the space... I think I lost your thread of thought..
> Do you want to define more info for each group than just the pre/post?

Nah. I guess I was thinking why bother, but anyway we have the space,
it's easy, and it might simplify things. So yeah, makes sense. If we
didn't have the space anyway I might've argued against it I guess :)

> > > +static void
> > > +genl_cmd_full_to_split(struct genl_split_ops *op,
> > > +		       const struct genl_family *family,
> > > +		       const struct genl_ops *full, u8 flags)
> > > +{  
> > 
> > [...]
> > 
> > 
> > > +	op->flags		|= flags;  
> > 
> > why |= ?
> 
> op->flags should already have all the existing flags (i.e. ADMIN_PERM)
> from the op, I'm adding the DO/DUMP to them.

OK, I guess I missed where that was being set.

> It's used as an output argument here, so that's what initializes it.
> genl_get_cmd* should always init the split command because in policy
> dumping we don't care about the errors, we just want the structure
> to be zeroed if do/dump is not implemented, and we'll skip accordingly.
> Wiping the 40B just to write all the fields felt... wrong. 
> Let KASAN catch us if we fail to init something.

KASAN doesn't, I think, you'd need KMSAN?

johannes

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

[Johannes Berg]

On Wed, 2022-10-19 at 12:25 -0700, Jakub Kicinski wrote:
> On Wed, 19 Oct 2022 10:15:05 +0200 Johannes Berg wrote:
> > > 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.  
> > 
> > You mean "Full ops would have [...] while split ops allow individual
> > [...]" or so?
> 
> Split ops end up being larger as we need a separate entry for each 
> do and dump. So I think it's right?
> 

Indeed.

Oh, I see now, you were basically saying "it's only 9% bigger for all
that extra flexibility" ... didn't read that right before.

johannes

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

[Jakub Kicinski]

On Wed, 19 Oct 2022 21:33:43 +0200 Johannes Berg wrote:
> > Do you mean that we no longer populate op->maxattr / op->policy and
> > some op may be reading those? I don't see any family code looking at
> > info->op.policy / maxattr.
> > 
> > First thing genl_family_rcv_msg_attrs_parse() does is:
> > 
> > 	if (!ops->maxattr)
> > 		return NULL;
> > 
> > So whether we skip it or call it - no difference.
> >   
> 
> Oh. I missed that, ok, thanks for clearing that up!

I'll put this info in the commit message, as requested.

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

[Jakub Kicinski]

On Wed, 19 Oct 2022 21:36:00 +0200 Johannes Berg wrote:
> > It's used as an output argument here, so that's what initializes it.
> > genl_get_cmd* should always init the split command because in policy
> > dumping we don't care about the errors, we just want the structure
> > to be zeroed if do/dump is not implemented, and we'll skip accordingly.
> > Wiping the 40B just to write all the fields felt... wrong. 
> > Let KASAN catch us if we fail to init something.  
> 
> KASAN doesn't, I think, you'd need KMSAN?

Quite possibly :S  Too many SANs :S

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

[Jakub Kicinski]

On Wed, 19 Oct 2022 21:37:41 +0200 Johannes Berg wrote:
> > > You mean "Full ops would have [...] while split ops allow individual
> > > [...]" or so?  
> > 
> > Split ops end up being larger as we need a separate entry for each 
> > do and dump. So I think it's right?
> 
> Indeed.
> 
> Oh, I see now, you were basically saying "it's only 9% bigger for all
> that extra flexibility" ... didn't read that right before.

Yup, BTW one annoying bit is that we treat maxattr == 0 as 
"no validation" rather than "reject everything".

Right now I add a reject-all policy in the family itself (with two
entries, argh), and hook it up to parameter-less dumps. But we could 
do something else - like modify the behavior in case the op was declared
as split at the family level.

I opted for having family add the reject-all policy because I code gen
the policies based on YAML spec, anyway, so not much extra effort, and
the uniformity between different type of ops seems worth maintaining.

WDYT?

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

[Jacob Keller]

On 10/19/2022 8:59 AM, Jakub Kicinski wrote:
> On Wed, 19 Oct 2022 09:50:16 +0200 Johannes Berg wrote:
>>> +			if (ctrl_dumppolicy_put_op(skb, cb, &op))
>>> +				return skb->len;
>>> +
>>> +			ctx->opidx = genl_get_cmd_cnt(ctx->rt);  
>>
>> This (now without a comment that you removed rather than changed), still
>> strikes me as odd.
>>
>> I guess if we add a comment /* don't enter the loop below */ that'd be
>> nicer, but I feel maybe putting the loop into the else instead would be
>> nicer?
> 
> The comment got eaten in rebases, it comes back in patch 10 apparently..
> I'll put it back in v2.

Thanks, I had the same confusion when reviewing.

-Jake

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

[Jacob Keller]

On 10/18/2022 4:07 PM, Jakub Kicinski wrote:
> 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.
> 

Neat, I didn't know about this.

> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> I did this cleanup to add more private fields but ended up
> not needing them. Still I think the commit makes sense?
> ---

Yea it makes sense to me.

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

>  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 8f780170e2f8..f2366b463597 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
> @@ -41,20 +40,16 @@ struct genl_info;
>   * @n_mcgrps: number of multicast groups
>   * @resv_start_op: first operation for which reserved fields of the header
>   *	can be validated, 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
>   * @n_small_ops: number of small-struct operations supported by this family
>   */
>  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;
> @@ -72,6 +67,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;
>  };
>  
>  /**

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

[Jacob Keller]

On 10/19/2022 12:36 PM, Johannes Berg wrote:
> On Wed, 2022-10-19 at 12:14 -0700, Jakub Kicinski wrote:
>>
>> Yes, we have the space... I think I lost your thread of thought..
>> Do you want to define more info for each group than just the pre/post?
> 
> Nah. I guess I was thinking why bother, but anyway we have the space,
> it's easy, and it might simplify things. So yeah, makes sense. If we
> didn't have the space anyway I might've argued against it I guess :)
> 

One could argue that we could use the space for something else.. but I
can't actually think of anything else here.

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

[Jacob Keller]

On 10/18/2022 4:07 PM, Jakub Kicinski wrote:
> 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 26ddbd23549d..9dfb3cf89b97 100644
> --- a/net/netlink/genetlink.c
> +++ b/net/netlink/genetlink.c
> @@ -166,11 +166,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;
> +	}
> +

Should this check that exactly one of GENL_CMD_CAP_DO and
GENL_CMD_CAP_DUMP is set? Or is some earlier flow enforcing this?

Thanks,
Jake

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

[Jacob Keller]

On 10/18/2022 4:07 PM, Jakub Kicinski wrote:
> 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.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Makes sense to me.

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

> ---
>  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 234b27977013..3e821c346577 100644
> --- a/net/netlink/genetlink.c
> +++ b/net/netlink/genetlink.c
> @@ -1319,7 +1319,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;
> @@ -1327,10 +1328,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;
>  

We don't need to check the GENL_DONT_VALIDATE_DUMP because the previous
code for getting the split op checked this and set some other fields, right?

>  	hdr = ctrl_dumppolicy_prep(skb, cb);
> @@ -1341,21 +1339,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);

We always use doit->cmd, but that shouldn't matter since the dump and
doit are always the same command here. Ok.

>  	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);
> @@ -1373,16 +1376,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;
>  
>  			ctx->opidx = genl_get_cmd_cnt(ctx->rt);
> @@ -1391,7 +1397,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++;

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

[Jakub Kicinski]

On Wed, 19 Oct 2022 14:33:39 -0700 Jacob Keller wrote:
> > +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;
> > +	}
> > +  
> 
> Should this check that exactly one of GENL_CMD_CAP_DO and
> GENL_CMD_CAP_DUMP is set? Or is some earlier flow enforcing this?

I check at registration time that the family-provided flags only 
have one of those two set. Internally, inside genetlink.c it'd be 
a programming error to request both at the same time, IOW it'd be
defensive programming to check for it.

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

[Jacob Keller]

On 10/18/2022 4:07 PM, Jakub Kicinski wrote:
> All callers go via genl_get_cmd_split() now,
> so merge genl_get_cmd() into it.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Ah nice. A lot of the time I would see code like this where the first
refactor and this merge were just done as a single thing. (it is easier
to manage but much harder to review). Thanks for keeping it split out.

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

> ---
>  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 3e821c346577..9bfb110053c7 100644
> --- a/net/netlink/genetlink.c
> +++ b/net/netlink/genetlink.c
> @@ -158,14 +158,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,
> @@ -207,13 +199,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;
> @@ -841,7 +835,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;

Ahhh this is where the GENL_CMD_CAP_DUMP vs GENL_CMD_CAP_DO is kept
separate. Either NLM_F_DUMP is set or not set so only one of these flags
will be set. Ok.

> -	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) &&
> @@ -1239,8 +1233,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;
>  		}
> @@ -1380,10 +1374,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 07/13] genetlink: support split policies in ctrl_dumppolicy_put_op()

[Jakub Kicinski]

On Wed, 19 Oct 2022 14:38:41 -0700 Jacob Keller wrote:
> > +	if (!doit->policy && !dumpit->policy)
> >  		return 0;
> >    
> 
> We don't need to check the GENL_DONT_VALIDATE_DUMP because the previous
> code for getting the split op checked this and set some other fields, right?

Yes - let me amend the commit message.

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

[Jacob Keller]

On 10/18/2022 4:07 PM, Jakub Kicinski wrote:
> 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 | 114 ++++++++++++++++++++++++++--------------
>  1 file changed, 74 insertions(+), 40 deletions(-)
> 
> diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
> index 9bfb110053c7..c5fcb7b9c383 100644
> --- a/net/netlink/genetlink.c
> +++ b/net/netlink/genetlink.c
> @@ -193,6 +193,7 @@ genl_cmd_full_to_split(struct genl_split_ops *op,
>  	op->flags		= full->flags;
>  	op->validate		= full->validate;
>  
> +	/* Make sure flags include the GENL_CMD_CAP_DO / GENL_CMD_CAP_DUMP */
>  	op->flags		|= flags;
>  

This seems like it could belong in an earlier patch?

Otherwise this looks good to me.

>  	return 0;
> @@ -228,6 +229,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;
> @@ -395,23 +447,19 @@ 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;
> -		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;
>  		}
>  	}
> @@ -891,6 +939,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);
> @@ -904,33 +953,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;
>  
> @@ -1203,8 +1245,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));
>  
> @@ -1259,26 +1301,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;
>  		}

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

[Jacob Keller]

On 10/18/2022 4:07 PM, Jakub Kicinski wrote:
> 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.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  net/netlink/genetlink.c | 48 ++++++++++++++++++++++-------------------
>  1 file changed, 26 insertions(+), 22 deletions(-)
> 
> diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
> index c5fcb7b9c383..63807204805a 100644
> --- a/net/netlink/genetlink.c
> +++ b/net/netlink/genetlink.c
> @@ -1225,10 +1225,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;
>  };

It might be easier to review this if the change to dump_map was done
separately, but I can understand the difficulty in splitting this.

Overall I think this makes sense.

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

>  
> @@ -1298,9 +1298,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,
> @@ -1318,12 +1325,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;
>  }
>  
> @@ -1403,11 +1414,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,
> @@ -1419,25 +1429,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;
>  
> -			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)) {
> @@ -1470,6 +1473,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;
>  }

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

[Jacob Keller]

On 10/18/2022 4:07 PM, Jakub Kicinski wrote:
> All dumpers use the iterators now, inline the cmd by index
> stuff into iterator code.
> 

Makes sense.

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 63807204805a..301981bae83d 100644
> --- a/net/netlink/genetlink.c
> +++ b/net/netlink/genetlink.c
> @@ -99,11 +99,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)
>  {
> @@ -217,18 +212,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;
> @@ -246,22 +229,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;

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

[Johannes Berg]

On Wed, 2022-10-19 at 12:57 -0700, Jakub Kicinski wrote:
> > Oh, I see now, you were basically saying "it's only 9% bigger for all
> > that extra flexibility" ... didn't read that right before.
> 
> Yup, BTW one annoying bit is that we treat maxattr == 0 as 
> "no validation" rather than "reject everything".
> 
> Right now I add a reject-all policy in the family itself (with two
> entries, argh), and hook it up to parameter-less dumps. But we could 
> do something else - like modify the behavior in case the op was declared
> as split at the family level.
> 
> I opted for having family add the reject-all policy because I code gen
> the policies based on YAML spec, anyway, so not much extra effort, and
> the uniformity between different type of ops seems worth maintaining.
> 
> WDYT?

Hmm. The codegen/YAML part likely won't really happen for all of the
families so perhaps some simplification would be good?

I feel like I probably should've changed this when adding
GENL_DONT_VALIDATE_DUMP_STRICT / GENL_DONT_VALIDATE_STRICT, but I guess
that's too late now :(

I guess we could add another set of flags, but that'd be annoying.

OTOH, it's nicer if future things are better, and we don't need to add a
"reject all" policy to all of them?

johannes

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

[Jakub Kicinski]

On Thu, 20 Oct 2022 09:32:17 +0200 Johannes Berg wrote:
> Hmm. The codegen/YAML part likely won't really happen for all of the
> families so perhaps some simplification would be good?
> 
> I feel like I probably should've changed this when adding
> GENL_DONT_VALIDATE_DUMP_STRICT / GENL_DONT_VALIDATE_STRICT, but I guess
> that's too late now :(
> 
> I guess we could add another set of flags, but that'd be annoying.

Perhaps we could hang it of the .resv_start_op as well?
Any op past that would treat policy == NULL as reject all?

We'd need to add GENL_DONT_VALIDATE_DO for families which 
want to parse inside the callbacks. I wonder if people would
get annoyed.

> OTOH, it's nicer if future things are better, and we don't need to add a
> "reject all" policy to all of them?

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

[Johannes Berg]

On Thu, 2022-10-20 at 11:09 -0700, Jakub Kicinski wrote:
> On Thu, 20 Oct 2022 09:32:17 +0200 Johannes Berg wrote:
> > Hmm. The codegen/YAML part likely won't really happen for all of the
> > families so perhaps some simplification would be good?
> > 
> > I feel like I probably should've changed this when adding
> > GENL_DONT_VALIDATE_DUMP_STRICT / GENL_DONT_VALIDATE_STRICT, but I guess
> > that's too late now :(
> > 
> > I guess we could add another set of flags, but that'd be annoying.
> 
> Perhaps we could hang it of the .resv_start_op as well?

Yes, hopefully? Maybe?

> Any op past that would treat policy == NULL as reject all?

Right. The only danger is that someone already added new stuff somewhere
and bad/broken userspace already used it with garbage attrs.

But the chances of that are probably low.

So I'd say go for it, and worst case we bump up the resv_start_op for
anything that breaks? Wouldn't be a huge loss either.

> We'd need to add GENL_DONT_VALIDATE_DO for families which 
> want to parse inside the callbacks. I wonder if people would
> get annoyed.

Why would anyone really want to _parse_ in the callbacks?

johannes

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

[Jakub Kicinski]

On Fri, 21 Oct 2022 13:02:31 +0200 Johannes Berg wrote:
> > Perhaps we could hang it of the .resv_start_op as well?  
> 
> Yes, hopefully? Maybe?
> 
> > Any op past that would treat policy == NULL as reject all?  
> 
> Right. The only danger is that someone already added new stuff somewhere
> and bad/broken userspace already used it with garbage attrs.
> 
> But the chances of that are probably low.
> 
> So I'd say go for it, and worst case we bump up the resv_start_op for
> anything that breaks? Wouldn't be a huge loss either.

resv_start_op are only present in -rc kernels, so I think we can break
things risking only common anger not uAPI wrath :)

> > We'd need to add GENL_DONT_VALIDATE_DO for families which 
> > want to parse inside the callbacks. I wonder if people would
> > get annoyed.  
> 
> Why would anyone really want to _parse_ in the callbacks?

Until recently that was the only way to do per-op policies, I don't
know if anyone actually used per-op policies outside of ethtool tho.