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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20201002083926.603adbcb@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Fri, 2 Oct 2020 08:39:26 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Johannes Berg <johannes@...solutions.net>
Cc:     netdev@...r.kernel.org, Johannes Berg <johannes.berg@...el.com>
Subject: Re: [PATCH 3/5] netlink: rework policy dump to support multiple
 policies

On Fri,  2 Oct 2020 11:09:42 +0200 Johannes Berg wrote:
> From: Johannes Berg <johannes.berg@...el.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@...el.com>

Reviewed-by: Jakub Kicinski <kuba@...nel.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