Re: [PATCH v2 1/2] netlink: add NLA_REJECT policy type

[Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>]

On Mon, Sep 17, 2018 at 11:57:28AM +0200, Johannes Berg wrote:
> From: Johannes Berg <johannes.berg@intel.com>
> 
> In some situations some netlink attributes may be used for output
> only (kernel->userspace) or may be reserved for future use. It's
> then helpful to be able to prevent userspace from using them in
> messages sent to the kernel, since they'd otherwise be ignored and
> any future will become impossible if this happens.
> 
> Add NLA_REJECT to the policy which does nothing but reject (with
> EINVAL) validation of any messages containing this attribute.
> Allow for returning a specific extended ACK error message in the
> validation_data pointer.
> 
> While at it clear up the documentation a bit - the NLA_BITFIELD32
> documentation was added to the list of len field descriptions.
> 
> Also, use NL_SET_BAD_ATTR() in one place where it's open-coded.
> 
> The specific case I have in mind now is a shared nested attribute
> containing request/response data, and it would be pointless and
> potentially confusing to have userspace include response data in
> the messages that actually contain a request.
> 
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>

Reviewed-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>

> ---
> v2: preserve behaviour of overwriting the extack message, with
>     either the generic or the specific one now
> ---
>  include/net/netlink.h | 13 ++++++++++++-
>  lib/nlattr.c          | 23 ++++++++++++++++-------
>  2 files changed, 28 insertions(+), 8 deletions(-)
> 
> diff --git a/include/net/netlink.h b/include/net/netlink.h
> index 0c154f98e987..b318b0a9f6c3 100644
> --- a/include/net/netlink.h
> +++ b/include/net/netlink.h
> @@ -180,6 +180,7 @@ enum {
>  	NLA_S32,
>  	NLA_S64,
>  	NLA_BITFIELD32,
> +	NLA_REJECT,
>  	__NLA_TYPE_MAX,
>  };
>  
> @@ -208,9 +209,19 @@ enum {
>   *    NLA_MSECS            Leaving the length field zero will verify the
>   *                         given type fits, using it verifies minimum length
>   *                         just like "All other"
> - *    NLA_BITFIELD32      A 32-bit bitmap/bitselector attribute
> + *    NLA_BITFIELD32       Unused
> + *    NLA_REJECT           Unused
>   *    All other            Minimum length of attribute payload
>   *
> + * Meaning of `validation_data' field:
> + *    NLA_BITFIELD32       This is a 32-bit bitmap/bitselector attribute and
> + *                         validation data must point to a u32 value of valid
> + *                         flags
> + *    NLA_REJECT           This attribute is always rejected and validation data
> + *                         may point to a string to report as the error instead
> + *                         of the generic one in extended ACK.
> + *    All other            Unused
> + *
>   * Example:
>   * static const struct nla_policy my_policy[ATTR_MAX+1] = {
>   * 	[ATTR_FOO] = { .type = NLA_U16 },
> diff --git a/lib/nlattr.c b/lib/nlattr.c
> index e335bcafa9e4..36d74b079151 100644
> --- a/lib/nlattr.c
> +++ b/lib/nlattr.c
> @@ -69,7 +69,8 @@ static int validate_nla_bitfield32(const struct nlattr *nla,
>  }
>  
>  static int validate_nla(const struct nlattr *nla, int maxtype,
> -			const struct nla_policy *policy)
> +			const struct nla_policy *policy,
> +			const char **error_msg)
>  {
>  	const struct nla_policy *pt;
>  	int minlen = 0, attrlen = nla_len(nla), type = nla_type(nla);
> @@ -87,6 +88,11 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
>  	}
>  
>  	switch (pt->type) {
> +	case NLA_REJECT:
> +		if (pt->validation_data && error_msg)
> +			*error_msg = pt->validation_data;
> +		return -EINVAL;
> +
>  	case NLA_FLAG:
>  		if (attrlen > 0)
>  			return -ERANGE;
> @@ -180,11 +186,10 @@ int nla_validate(const struct nlattr *head, int len, int maxtype,
>  	int rem;
>  
>  	nla_for_each_attr(nla, head, len, rem) {
> -		int err = validate_nla(nla, maxtype, policy);
> +		int err = validate_nla(nla, maxtype, policy, NULL);
>  
>  		if (err < 0) {
> -			if (extack)
> -				extack->bad_attr = nla;
> +			NL_SET_BAD_ATTR(extack, nla);
>  			return err;
>  		}
>  	}
> @@ -250,11 +255,15 @@ int nla_parse(struct nlattr **tb, int maxtype, const struct nlattr *head,
>  		u16 type = nla_type(nla);
>  
>  		if (type > 0 && type <= maxtype) {
> +			static const char _msg[] = "Attribute failed policy validation";
> +			const char *msg = _msg;
> +
>  			if (policy) {
> -				err = validate_nla(nla, maxtype, policy);
> +				err = validate_nla(nla, maxtype, policy, &msg);
>  				if (err < 0) {
> -					NL_SET_ERR_MSG_ATTR(extack, nla,
> -							    "Attribute failed policy validation");
> +					NL_SET_BAD_ATTR(extack, nla);
> +					if (extack)
> +						extack->_msg = msg;
>  					goto errout;
>  				}
>  			}
> -- 
> 2.14.4
>