diff mbox series

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

Message ID 20201002110205.2d0d1bd5027d.I525cd130f9c78d7a6acd90d735a67974e51fb73c@changeid
State New
Headers show
Series genetlink: complete policy dumping | expand

Commit Message

Johannes Berg Oct. 2, 2020, 9:09 a.m. UTC 
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(-)

Comments

Jakub Kicinski Oct. 2, 2020, 3:39 p.m. UTC | #1 
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
Johannes Berg Oct. 2, 2020, 8:11 p.m. UTC | #2 
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
Johannes Berg Oct. 2, 2020, 8:13 p.m. UTC | #3 
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
Jakub Kicinski Oct. 2, 2020, 8:37 p.m. UTC | #4 
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.
Johannes Berg Oct. 2, 2020, 8:38 p.m. UTC | #5 
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
Jakub Kicinski Oct. 2, 2020, 8:42 p.m. UTC | #6 
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 :)
diff mbox series

Patch

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