[PATCH 0/5] genetlink: complete policy dumping

[PATCH 0/5] genetlink: complete policy dumping

[Johannes Berg]

Hi,

So ... Jakub added per-op policy retrieval, so you could retrieve the
policy for a single op.

This then adds - as discussed - support for dumping *everything*, which
has basically first an [op] -> [policy-idx] mapping, followed by all the
policies. When a single op is requested, you get a convenience [op] -> 0
mapping entry, but you might as well ignore it since the policy for the
requested op is guaranteed to be 0.

This series applies on top of Jakub's series, but I've fixed up his to
apply on top of my bugfix (let me know how you want to handle that).

For convenience, I've pushed the entire series here:

https://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211-next.git/log/?h=genetlink-op-policy-export
(I hope the cache will be invalidated soon, but anyway, in mac80211-next
genetlink-op-policy-export branch)

I didn't want to repost Jakub's slightly modified patches just for that.
Depending on how we decide to deal with the conflicts, we may or may not
need that series.

johannes

[PATCH 1/5] netlink: simplify netlink_policy_dump_start() prototype

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

Since moving the call to this to a dump start() handler we no
longer need this to deal with being called after having been
called already. Since that is the preferred way of doing things
anyway, remove the code necessary for that and simply return
the pointer (or an ERR_PTR()).

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 include/net/netlink.h   |  6 +++---
 net/netlink/genetlink.c |  5 ++++-
 net/netlink/policy.c    | 19 +++++++------------
 3 files changed, 14 insertions(+), 16 deletions(-)

diff --git a/include/net/netlink.h b/include/net/netlink.h
index 00258590f2cb..4be0ad237e57 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -1937,9 +1937,9 @@ void nla_get_range_signed(const struct nla_policy *pt,
 
 struct netlink_policy_dump_state;
 
-int netlink_policy_dump_start(const struct nla_policy *policy,
-			      unsigned int maxtype,
-			      struct netlink_policy_dump_state **state);
+struct netlink_policy_dump_state *
+netlink_policy_dump_start(const struct nla_policy *policy,
+			  unsigned int maxtype);
 bool netlink_policy_dump_loop(struct netlink_policy_dump_state *state);
 int netlink_policy_dump_write(struct sk_buff *skb,
 			      struct netlink_policy_dump_state *state);
diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index fad691037402..537472342781 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -1164,7 +1164,10 @@ static int ctrl_dumppolicy_start(struct netlink_callback *cb)
 	if (!op.policy)
 		return -ENODATA;
 
-	return netlink_policy_dump_start(op.policy, op.maxattr, &ctx->state);
+	ctx->state = netlink_policy_dump_start(op.policy, op.maxattr);
+	if (IS_ERR(ctx->state))
+		return PTR_ERR(ctx->state);
+	return 0;
 }
 
 static int ctrl_dumppolicy(struct sk_buff *skb, struct netlink_callback *cb)
diff --git a/net/netlink/policy.c b/net/netlink/policy.c
index 0dc804afdb25..2a0d85cbc0a2 100644
--- a/net/netlink/policy.c
+++ b/net/netlink/policy.c
@@ -76,17 +76,14 @@ static unsigned int get_policy_idx(struct netlink_policy_dump_state *state,
 	return -1;
 }
 
-int netlink_policy_dump_start(const struct nla_policy *policy,
-			      unsigned int maxtype,
-                              struct netlink_policy_dump_state **statep)
+struct netlink_policy_dump_state *
+netlink_policy_dump_start(const struct nla_policy *policy,
+			  unsigned int maxtype)
 {
 	struct netlink_policy_dump_state *state;
 	unsigned int policy_idx;
 	int err;
 
-	if (*statep)
-		return 0;
-
 	/*
 	 * walk the policies and nested ones first, and build
 	 * a linear list of them.
@@ -95,12 +92,12 @@ int netlink_policy_dump_start(const struct nla_policy *policy,
 	state = kzalloc(struct_size(state, policies, INITIAL_POLICIES_ALLOC),
 			GFP_KERNEL);
 	if (!state)
-		return -ENOMEM;
+		return ERR_PTR(-ENOMEM);
 	state->n_alloc = INITIAL_POLICIES_ALLOC;
 
 	err = add_policy(&state, policy, maxtype);
 	if (err)
-		return err;
+		return ERR_PTR(err);
 
 	for (policy_idx = 0;
 	     policy_idx < state->n_alloc && state->policies[policy_idx].policy;
@@ -120,7 +117,7 @@ int netlink_policy_dump_start(const struct nla_policy *policy,
 						 policy[type].nested_policy,
 						 policy[type].len);
 				if (err)
-					return err;
+					return ERR_PTR(err);
 				break;
 			default:
 				break;
@@ -128,9 +125,7 @@ int netlink_policy_dump_start(const struct nla_policy *policy,
 		}
 	}
 
-	*statep = state;
-
-	return 0;
+	return state;
 }
 
 static bool
-- 
2.26.2

Re: [PATCH 1/5] netlink: simplify netlink_policy_dump_start() prototype

[Jakub Kicinski]

On Fri,  2 Oct 2020 11:09:40 +0200 Johannes Berg wrote:
> From: Johannes Berg <johannes.berg@intel.com>
> 
> Since moving the call to this to a dump start() handler we no
> longer need this to deal with being called after having been
> called already. Since that is the preferred way of doing things
> anyway, remove the code necessary for that and simply return
> the pointer (or an ERR_PTR()).
> 
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>

Reviewed-by: Jakub Kicinski <kuba@kernel.org>

> -	return netlink_policy_dump_start(op.policy, op.maxattr, &ctx->state);
> +	ctx->state = netlink_policy_dump_start(op.policy, op.maxattr);
> +	if (IS_ERR(ctx->state))
> +		return PTR_ERR(ctx->state);
> +	return 0;

PTR_ERR_OR_ZERO()?

Re: [PATCH 1/5] netlink: simplify netlink_policy_dump_start() prototype

[Johannes Berg]

On Fri, 2020-10-02 at 08:31 -0700, Jakub Kicinski wrote:
> On Fri,  2 Oct 2020 11:09:40 +0200 Johannes Berg wrote:
> > From: Johannes Berg <johannes.berg@intel.com>
> > 
> > Since moving the call to this to a dump start() handler we no
> > longer need this to deal with being called after having been
> > called already. Since that is the preferred way of doing things
> > anyway, remove the code necessary for that and simply return
> > the pointer (or an ERR_PTR()).
> > 
> > Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> 
> Reviewed-by: Jakub Kicinski <kuba@kernel.org>
> 
> > -	return netlink_policy_dump_start(op.policy, op.maxattr, &ctx->state);
> > +	ctx->state = netlink_policy_dump_start(op.policy, op.maxattr);
> > +	if (IS_ERR(ctx->state))
> > +		return PTR_ERR(ctx->state);
> > +	return 0;
> 
> PTR_ERR_OR_ZERO()?

Hah! I didn't even know about that, thanks.

johannes

[PATCH 2/5] netlink: compare policy more accurately

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

The maxtype is really an integral part of the policy, and while we
haven't gotten into a situation yet where this happens, it seems
that some developer might eventually have two places pointing to
identical policies, with different maxattr to exclude some attrs
in one of the places.

Even if not, it's really the right thing to compare both since the
two data items fundamentally belong together.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 net/netlink/policy.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/netlink/policy.c b/net/netlink/policy.c
index 2a0d85cbc0a2..7bc8f81ecc43 100644
--- a/net/netlink/policy.c
+++ b/net/netlink/policy.c
@@ -35,7 +35,8 @@ static int add_policy(struct netlink_policy_dump_state **statep,
 		return 0;
 
 	for (i = 0; i < state->n_alloc; i++) {
-		if (state->policies[i].policy == policy)
+		if (state->policies[i].policy == policy &&
+		    state->policies[i].maxtype == maxtype)
 			return 0;
 
 		if (!state->policies[i].policy) {
-- 
2.26.2

Re: [PATCH 2/5] netlink: compare policy more accurately

[Jakub Kicinski]

On Fri,  2 Oct 2020 11:09:41 +0200 Johannes Berg wrote:
> From: Johannes Berg <johannes.berg@intel.com>
> 
> The maxtype is really an integral part of the policy, and while we
> haven't gotten into a situation yet where this happens, it seems
> that some developer might eventually have two places pointing to
> identical policies, with different maxattr to exclude some attrs
> in one of the places.
> 
> Even if not, it's really the right thing to compare both since the
> two data items fundamentally belong together.
> 
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>

Reviewed-by: Jakub Kicinski <kuba@kernel.org>

[PATCH 3/5] netlink: rework policy dump to support multiple policies

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

Rework the policy dump code a bit to support adding multiple
policies to a single dump, in order to e.g. support per-op
policies in generic netlink.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 include/net/netlink.h   | 62 +++++++++++++++++++++++++++++++++++++++--
 net/netlink/genetlink.c |  6 ++--
 net/netlink/policy.c    | 57 +++++++++++++++++++++++++++++--------
 3 files changed, 106 insertions(+), 19 deletions(-)

diff --git a/include/net/netlink.h b/include/net/netlink.h
index 4be0ad237e57..a929759a03f5 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -1937,12 +1937,68 @@ void nla_get_range_signed(const struct nla_policy *pt,
 
 struct netlink_policy_dump_state;
 
-struct netlink_policy_dump_state *
-netlink_policy_dump_start(const struct nla_policy *policy,
-			  unsigned int maxtype);
+/**
+ * netlink_policy_dump_add_policy - add a policy to the dump
+ * @pstate: state to add to, may be reallocated, must be %NULL the first time
+ * @policy: the new policy to add to the dump
+ * @maxtype: the new policy's max attr type
+ *
+ * Returns: 0 on success, a negative error code otherwise.
+ *
+ * Call this to allocate a policy dump state, and to add policies to it. This
+ * should be called from the dump start() callback.
+ *
+ * Note: on failures, any previously allocated state is freed.
+ */
+int netlink_policy_dump_add_policy(struct netlink_policy_dump_state **pstate,
+				   const struct nla_policy *policy,
+				   unsigned int maxtype);
+
+/**
+ * netlink_policy_dump_get_policy_idx - retrieve policy index
+ * @state: the policy dump state
+ * @policy: the policy to find
+ * @maxattr: the policy's maxattr
+ *
+ * Returns: the index of the given policy in the dump state
+ *
+ * Call this to find a policy index when you've added multiple and e.g.
+ * need to tell userspace which command has which policy (by index).
+ *
+ * Note: this will WARN and return 0 if the policy isn't found, which
+ *	 means it wasn't added in the first place, which would be an
+ *	 internal consistency bug.
+ */
+int netlink_policy_dump_get_policy_idx(struct netlink_policy_dump_state *state,
+				       const struct nla_policy *policy,
+				       unsigned int maxtype);
+
+/**
+ * netlink_policy_dump_loop - dumping loop indicator
+ * @state: the policy dump state
+ *
+ * Returns: %true if the dump continues, %false otherwise
+ *
+ * Note: this frees the dump state when finishing
+ */
 bool netlink_policy_dump_loop(struct netlink_policy_dump_state *state);
+
+/**
+ * netlink_policy_dump_write - write current policy dump attributes
+ * @skb: the message skb to write to
+ * @state: the policy dump state
+ *
+ * Returns: 0 on success, an error code otherwise
+ */
 int netlink_policy_dump_write(struct sk_buff *skb,
 			      struct netlink_policy_dump_state *state);
+
+/**
+ * netlink_policy_dump_free - free policy dump state
+ * @state: the policy dump state to free
+ *
+ * Call this from the done() method to ensure dump state is freed.
+ */
 void netlink_policy_dump_free(struct netlink_policy_dump_state *state);
 
 #endif
diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index 537472342781..42777749d4d8 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -1164,10 +1164,8 @@ static int ctrl_dumppolicy_start(struct netlink_callback *cb)
 	if (!op.policy)
 		return -ENODATA;
 
-	ctx->state = netlink_policy_dump_start(op.policy, op.maxattr);
-	if (IS_ERR(ctx->state))
-		return PTR_ERR(ctx->state);
-	return 0;
+	return netlink_policy_dump_add_policy(&ctx->state, op.policy,
+					      op.maxattr);
 }
 
 static int ctrl_dumppolicy(struct sk_buff *skb, struct netlink_callback *cb)
diff --git a/net/netlink/policy.c b/net/netlink/policy.c
index 7bc8f81ecc43..d930064cbaf3 100644
--- a/net/netlink/policy.c
+++ b/net/netlink/policy.c
@@ -77,28 +77,41 @@ static unsigned int get_policy_idx(struct netlink_policy_dump_state *state,
 	return -1;
 }
 
-struct netlink_policy_dump_state *
-netlink_policy_dump_start(const struct nla_policy *policy,
-			  unsigned int maxtype)
+static struct netlink_policy_dump_state *alloc_state(void)
 {
 	struct netlink_policy_dump_state *state;
+
+	state = kzalloc(struct_size(state, policies, INITIAL_POLICIES_ALLOC),
+			GFP_KERNEL);
+	if (!state)
+		return ERR_PTR(-ENOMEM);
+	state->n_alloc = INITIAL_POLICIES_ALLOC;
+
+	return state;
+}
+
+int netlink_policy_dump_add_policy(struct netlink_policy_dump_state **pstate,
+				   const struct nla_policy *policy,
+				   unsigned int maxtype)
+{
+	struct netlink_policy_dump_state *state = *pstate;
 	unsigned int policy_idx;
 	int err;
 
+	if (!state) {
+		state = alloc_state();
+		if (IS_ERR(state))
+			return PTR_ERR(state);
+	}
+
 	/*
 	 * walk the policies and nested ones first, and build
 	 * a linear list of them.
 	 */
 
-	state = kzalloc(struct_size(state, policies, INITIAL_POLICIES_ALLOC),
-			GFP_KERNEL);
-	if (!state)
-		return ERR_PTR(-ENOMEM);
-	state->n_alloc = INITIAL_POLICIES_ALLOC;
-
 	err = add_policy(&state, policy, maxtype);
 	if (err)
-		return ERR_PTR(err);
+		return err;
 
 	for (policy_idx = 0;
 	     policy_idx < state->n_alloc && state->policies[policy_idx].policy;
@@ -118,7 +131,7 @@ netlink_policy_dump_start(const struct nla_policy *policy,
 						 policy[type].nested_policy,
 						 policy[type].len);
 				if (err)
-					return ERR_PTR(err);
+					return err;
 				break;
 			default:
 				break;
@@ -126,7 +139,27 @@ netlink_policy_dump_start(const struct nla_policy *policy,
 		}
 	}
 
-	return state;
+	*pstate = state;
+	return 0;
+}
+
+int netlink_policy_dump_get_policy_idx(struct netlink_policy_dump_state *state,
+				       const struct nla_policy *policy,
+				       unsigned int maxtype)
+{
+	unsigned int i;
+
+	if (WARN_ON(!policy || !maxtype))
+                return 0;
+
+	for (i = 0; i < state->n_alloc; i++) {
+		if (state->policies[i].policy == policy &&
+		    state->policies[i].maxtype == maxtype)
+			return i;
+	}
+
+	WARN_ON(1);
+	return 0;
 }
 
 static bool
-- 
2.26.2

Re: [PATCH 3/5] netlink: rework policy dump to support multiple policies

[Jakub Kicinski]

On Fri,  2 Oct 2020 11:09:42 +0200 Johannes Berg wrote:
> From: Johannes Berg <johannes.berg@intel.com>
> 
> Rework the policy dump code a bit to support adding multiple
> policies to a single dump, in order to e.g. support per-op
> policies in generic netlink.
> 
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>

Reviewed-by: Jakub Kicinski <kuba@kernel.org>

with a side of nits..

> diff --git a/include/net/netlink.h b/include/net/netlink.h
> index 4be0ad237e57..a929759a03f5 100644
> --- a/include/net/netlink.h
> +++ b/include/net/netlink.h
> @@ -1937,12 +1937,68 @@ void nla_get_range_signed(const struct nla_policy *pt,
>  
>  struct netlink_policy_dump_state;
>  
> -struct netlink_policy_dump_state *
> -netlink_policy_dump_start(const struct nla_policy *policy,
> -			  unsigned int maxtype);
> +/**
> + * netlink_policy_dump_add_policy - add a policy to the dump
> + * @pstate: state to add to, may be reallocated, must be %NULL the first time
> + * @policy: the new policy to add to the dump
> + * @maxtype: the new policy's max attr type
> + *
> + * Returns: 0 on success, a negative error code otherwise.
> + *
> + * Call this to allocate a policy dump state, and to add policies to it. This
> + * should be called from the dump start() callback.
> + *
> + * Note: on failures, any previously allocated state is freed.
> + */
> +int netlink_policy_dump_add_policy(struct netlink_policy_dump_state **pstate,
> +				   const struct nla_policy *policy,
> +				   unsigned int maxtype);

Personal preference perhaps, but I prefer kdoc with the definition.

> diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
> index 537472342781..42777749d4d8 100644
> --- a/net/netlink/genetlink.c
> +++ b/net/netlink/genetlink.c
> @@ -1164,10 +1164,8 @@ static int ctrl_dumppolicy_start(struct netlink_callback *cb)
>  	if (!op.policy)
>  		return -ENODATA;
>  
> -	ctx->state = netlink_policy_dump_start(op.policy, op.maxattr);
> -	if (IS_ERR(ctx->state))
> -		return PTR_ERR(ctx->state);
> -	return 0;
> +	return netlink_policy_dump_add_policy(&ctx->state, op.policy,
> +					      op.maxattr);

Looks like we flip-flopped between int and pointer return between
patches 1 and this one?

>  }

> +int netlink_policy_dump_get_policy_idx(struct netlink_policy_dump_state *state,
> +				       const struct nla_policy *policy,
> +				       unsigned int maxtype)
> +{
> +	unsigned int i;
> +
> +	if (WARN_ON(!policy || !maxtype))
> +                return 0;

Would this warning make sense in add() (if not already there)?
If null/0 is never added it can't match and we'd just hit the
warning below.

> +	for (i = 0; i < state->n_alloc; i++) {
> +		if (state->policies[i].policy == policy &&
> +		    state->policies[i].maxtype == maxtype)
> +			return i;
> +	}
> +
> +	WARN_ON(1);
> +	return 0;
>  }
>  
>  static bool

Re: [PATCH 3/5] netlink: rework policy dump to support multiple policies

[Johannes Berg]

On Fri, 2020-10-02 at 08:39 -0700, Jakub Kicinski wrote:
> 
> > -	ctx->state = netlink_policy_dump_start(op.policy, op.maxattr);
> > -	if (IS_ERR(ctx->state))
> > -		return PTR_ERR(ctx->state);
> > -	return 0;
> > +	return netlink_policy_dump_add_policy(&ctx->state, op.policy,
> > +					      op.maxattr);
> 
> Looks like we flip-flopped between int and pointer return between
> patches 1 and this one?

Huh, yeah, that was kinda dumb. I started going down one path and then
...

I'll probably just squash the first patch or something. Will figure
something out, thanks.

> >  }
> > +int netlink_policy_dump_get_policy_idx(struct netlink_policy_dump_state *state,
> > +				       const struct nla_policy *policy,
> > +				       unsigned int maxtype)
> > +{
> > +	unsigned int i;
> > +
> > +	if (WARN_ON(!policy || !maxtype))
> > +                return 0;
> 
> Would this warning make sense in add() (if not already there)?
> If null/0 is never added it can't match and we'd just hit the
> warning below.

It's not there, because had originally thought it should be OK to just
blindly add a policy of a family even if it has none. But that makes no
sense.

However, it's not true that it can't match, because

> > +	for (i = 0; i < state->n_alloc; i++) {

we go to n_alloc here, and don't separately track n_used, but n_alloc
grows in tens (or so), not singles.

johannes

Re: [PATCH 3/5] netlink: rework policy dump to support multiple policies

[Johannes Berg]

Oh, and ...

> > +/**
> > + * netlink_policy_dump_add_policy - add a policy to the dump
> > + * @pstate: state to add to, may be reallocated, must be %NULL the first time
> > + * @policy: the new policy to add to the dump
> > + * @maxtype: the new policy's max attr type
> > + *
> > + * Returns: 0 on success, a negative error code otherwise.
> > + *
> > + * Call this to allocate a policy dump state, and to add policies to it. This
> > + * should be called from the dump start() callback.
> > + *
> > + * Note: on failures, any previously allocated state is freed.
> > + */
> > +int netlink_policy_dump_add_policy(struct netlink_policy_dump_state **pstate,
> > +				   const struct nla_policy *policy,
> > +				   unsigned int maxtype);
> 
> Personal preference perhaps, but I prefer kdoc with the definition.

I realized recently that this is actually better, because then "make
W=1" will in fact check the kernel-doc for consistency ... but it
doesn't do it in header files.

Just have to get into the habit now ...

johannes

Re: [PATCH 3/5] netlink: rework policy dump to support multiple policies

[Jakub Kicinski]

On Fri, 02 Oct 2020 22:13:36 +0200 Johannes Berg wrote:
> > > +int netlink_policy_dump_add_policy(struct netlink_policy_dump_state **pstate,
> > > +				   const struct nla_policy *policy,
> > > +				   unsigned int maxtype);  
> > 
> > Personal preference perhaps, but I prefer kdoc with the definition.  
> 
> I realized recently that this is actually better, because then "make
> W=1" will in fact check the kernel-doc for consistency ... but it
> doesn't do it in header files.
> 
> Just have to get into the habit now ...

:o 

I was wondering why I didn't see errors from headers in the past!

I guess it's because of the volume of messages this would cause.

Re: [PATCH 3/5] netlink: rework policy dump to support multiple policies

[Johannes Berg]

On Fri, 2020-10-02 at 13:37 -0700, Jakub Kicinski wrote:
> 
> I guess it's because of the volume of messages this would cause.

There's actually been a large cleanup, so it wouldn't be too bad.

I had really just suspected tooling/build system issues, you know which
C files you're compiling, but not easily which headers they're
including, so how to run the checker script on them?

Anyway, never really looked into it.

johannes

Re: [PATCH 3/5] netlink: rework policy dump to support multiple policies

[Jakub Kicinski]

On Fri, 02 Oct 2020 22:38:49 +0200 Johannes Berg wrote:
> On Fri, 2020-10-02 at 13:37 -0700, Jakub Kicinski wrote:
> > I guess it's because of the volume of messages this would cause.  
> 
> There's actually been a large cleanup, so it wouldn't be too bad.
> 
> I had really just suspected tooling/build system issues, you know which
> C files you're compiling, but not easily which headers they're
> including, so how to run the checker script on them?
> 
> Anyway, never really looked into it.

Good point, in any case it should be simple enough to add a separate
test for headers to a CI, now that I know it's the case :)

[PATCH 4/5] genetlink: factor skb preparation out of ctrl_dumppolicy()

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

We'll need this later for the per-op policy index dump.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 net/netlink/genetlink.c | 26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)

diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index 42777749d4d8..0ab9549e30ee 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -1168,23 +1168,35 @@ static int ctrl_dumppolicy_start(struct netlink_callback *cb)
 					      op.maxattr);
 }
 
+static void *ctrl_dumppolicy_prep(struct sk_buff *skb,
+				  struct netlink_callback *cb)
+{
+	struct ctrl_dump_policy_ctx *ctx = (void *)cb->ctx;
+	void *hdr;
+
+	hdr = genlmsg_put(skb, NETLINK_CB(cb->skb).portid,
+			  cb->nlh->nlmsg_seq, &genl_ctrl,
+			  NLM_F_MULTI, CTRL_CMD_GETPOLICY);
+	if (!hdr)
+		return NULL;
+
+	if (nla_put_u16(skb, CTRL_ATTR_FAMILY_ID, ctx->fam_id))
+		return NULL;
+
+	return hdr;
+}
+
 static int ctrl_dumppolicy(struct sk_buff *skb, struct netlink_callback *cb)
 {
 	struct ctrl_dump_policy_ctx *ctx = (void *)cb->ctx;
 
 	while (netlink_policy_dump_loop(ctx->state)) {
-		void *hdr;
+		void *hdr = ctrl_dumppolicy_prep(skb, cb);
 		struct nlattr *nest;
 
-		hdr = genlmsg_put(skb, NETLINK_CB(cb->skb).portid,
-				  cb->nlh->nlmsg_seq, &genl_ctrl,
-				  NLM_F_MULTI, CTRL_CMD_GETPOLICY);
 		if (!hdr)
 			goto nla_put_failure;
 
-		if (nla_put_u16(skb, CTRL_ATTR_FAMILY_ID, ctx->fam_id))
-			goto nla_put_failure;
-
 		nest = nla_nest_start(skb, CTRL_ATTR_POLICY);
 		if (!nest)
 			goto nla_put_failure;
-- 
2.26.2

Re: [PATCH 4/5] genetlink: factor skb preparation out of ctrl_dumppolicy()

[Jakub Kicinski]

On Fri,  2 Oct 2020 11:09:43 +0200 Johannes Berg wrote:
> From: Johannes Berg <johannes.berg@intel.com>
> 
> We'll need this later for the per-op policy index dump.
> 
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>

Reviewed-by: Jakub Kicinski <kuba@kernel.org>

>  	while (netlink_policy_dump_loop(ctx->state)) {
> -		void *hdr;
> +		void *hdr = ctrl_dumppolicy_prep(skb, cb);
>  		struct nlattr *nest;
>  
> -		hdr = genlmsg_put(skb, NETLINK_CB(cb->skb).portid,
> -				  cb->nlh->nlmsg_seq, &genl_ctrl,
> -				  NLM_F_MULTI, CTRL_CMD_GETPOLICY);
>  		if (!hdr)
>  			goto nla_put_failure;

bike shedding, but I find it less pretty when functions which require
error checking are called as variable init (if it's not the only
variable declared).

[PATCH 5/5] genetlink: properly support per-op policy dumping

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

Add support for per-op policy dumping. The data is pretty much
as before, except that now the assumption that the policy with
index 0 is "the" policy no longer holds - you now need to look
at the new CTRL_ATTR_OP_POLICY attribute which is a nested attr
containing the cmd -> policy index mapping.

When a single op is requested, the CTRL_ATTR_OP_POLICY will be
added but the policy for this op is still guaranteed to be at
index 0, so that's just for convenience.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 include/uapi/linux/genetlink.h |   1 +
 net/netlink/genetlink.c        | 107 ++++++++++++++++++++++++++++-----
 2 files changed, 93 insertions(+), 15 deletions(-)

diff --git a/include/uapi/linux/genetlink.h b/include/uapi/linux/genetlink.h
index 7dbe2d5d7d46..583dcc737250 100644
--- a/include/uapi/linux/genetlink.h
+++ b/include/uapi/linux/genetlink.h
@@ -65,6 +65,7 @@ enum {
 	CTRL_ATTR_MCAST_GROUPS,
 	CTRL_ATTR_POLICY,
 	CTRL_ATTR_OP,
+	CTRL_ATTR_OP_POLICY,
 	__CTRL_ATTR_MAX,
 };
 
diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index 0ab9549e30ee..b2a87fbd3875 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -1112,7 +1112,12 @@ 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;
+	u32 op;
 	u16 fam_id;
+	u8 policies:1,
+	   single_op:1;
 };
 
 static const struct nla_policy ctrl_policy_policy[] = {
@@ -1129,7 +1134,7 @@ static int ctrl_dumppolicy_start(struct netlink_callback *cb)
 	struct nlattr **tb = info->attrs;
 	const struct genl_family *rt;
 	struct genl_ops op;
-	int err;
+	int err, i;
 
 	BUILD_BUG_ON(sizeof(*ctx) > sizeof(cb->ctx));
 
@@ -1150,22 +1155,40 @@ static int ctrl_dumppolicy_start(struct netlink_callback *cb)
 	if (!rt)
 		return -ENOENT;
 
+	ctx->rt = rt;
+
 	if (tb[CTRL_ATTR_OP]) {
-		err = genl_get_cmd(nla_get_u32(tb[CTRL_ATTR_OP]), rt, &op);
+		ctx->single_op = true;
+		ctx->op = nla_get_u32(tb[CTRL_ATTR_OP]);
+
+		err = genl_get_cmd(ctx->op, rt, &op);
 		if (err) {
 			NL_SET_BAD_ATTR(cb->extack, tb[CTRL_ATTR_OP]);
 			return err;
 		}
-	} else {
-		op.policy = rt->policy;
-		op.maxattr = rt->maxattr;
+
+		if (!op.policy)
+			return -ENODATA;
+
+		return netlink_policy_dump_add_policy(&ctx->state, op.policy,
+						      op.maxattr);
 	}
 
-	if (!op.policy)
-		return -ENODATA;
+	for (i = 0; i < genl_get_cmd_cnt(rt); i++) {
+		genl_get_cmd_by_index(i, rt, &op);
+
+		if (op.policy) {
+			err = netlink_policy_dump_add_policy(&ctx->state,
+							     op.policy,
+							     op.maxattr);
+			if (err)
+				return err;
+		}
+	}
 
-	return netlink_policy_dump_add_policy(&ctx->state, op.policy,
-					      op.maxattr);
+	if (!ctx->state)
+		return -ENODATA;
+	return 0;
 }
 
 static void *ctrl_dumppolicy_prep(struct sk_buff *skb,
@@ -1189,11 +1212,65 @@ static void *ctrl_dumppolicy_prep(struct sk_buff *skb,
 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) {
+		if (ctx->single_op) {
+			struct nlattr *nest;
+
+			hdr = ctrl_dumppolicy_prep(skb, cb);
+			if (!hdr)
+				goto nla_put_failure;
+
+			nest = nla_nest_start(skb, CTRL_ATTR_OP_POLICY);
+			if (!nest)
+				goto nla_put_failure;
+
+			if (nla_put_u32(skb, ctx->op, 0))
+				goto nla_put_failure;
+
+			nla_nest_end(skb, nest);
+			genlmsg_end(skb, hdr);
+
+			/* skip loop below */
+			ctx->opidx = genl_get_cmd_cnt(ctx->rt);
+		}
+
+		while (ctx->opidx < genl_get_cmd_cnt(ctx->rt)) {
+			struct genl_ops op;
+			struct nlattr *nest;
+			int idx;
+
+			hdr = ctrl_dumppolicy_prep(skb, cb);
+			if (!hdr)
+				goto nla_put_failure;
+
+			genl_get_cmd_by_index(ctx->opidx, ctx->rt, &op);
+
+			nest = nla_nest_start(skb, CTRL_ATTR_OP_POLICY);
+			if (!nest)
+				goto nla_put_failure;
+
+			idx = netlink_policy_dump_get_policy_idx(ctx->state,
+								 op.policy,
+								 op.maxattr);
+			if (nla_put_u32(skb, op.cmd, idx))
+				goto nla_put_failure;
+
+			ctx->opidx++;
+
+			nla_nest_end(skb, nest);
+			genlmsg_end(skb, hdr);
+		}
+
+		/* completed with the per-op policy index list */
+		ctx->policies = true;
+	}
 
 	while (netlink_policy_dump_loop(ctx->state)) {
-		void *hdr = ctrl_dumppolicy_prep(skb, cb);
 		struct nlattr *nest;
 
+		hdr = ctrl_dumppolicy_prep(skb, cb);
 		if (!hdr)
 			goto nla_put_failure;
 
@@ -1201,20 +1278,20 @@ static int ctrl_dumppolicy(struct sk_buff *skb, struct netlink_callback *cb)
 		if (!nest)
 			goto nla_put_failure;
 
+
 		if (netlink_policy_dump_write(skb, ctx->state))
 			goto nla_put_failure;
 
 		nla_nest_end(skb, nest);
 
 		genlmsg_end(skb, hdr);
-		continue;
-
-nla_put_failure:
-		genlmsg_cancel(skb, hdr);
-		break;
 	}
 
 	return skb->len;
+
+nla_put_failure:
+	genlmsg_cancel(skb, hdr);
+	return skb->len;
 }
 
 static int ctrl_dumppolicy_done(struct netlink_callback *cb)
-- 
2.26.2