Re: [PATCH v2 net-next] taprio: validate TCA_TAPRIO_ATTR_FLAGS through policy instead of open-coding

[Simon Horman <horms@kernel.org>]

On Wed, Jan 24, 2024 at 10:21:18AM +0100, Alessandro Marcolini wrote:
> As of now, the field TCA_TAPRIO_ATTR_FLAGS is being validated by manually
> checking its value, using the function taprio_flags_valid().
> 
> With this patch, the field will be validated through the netlink policy
> NLA_POLICY_MASK, where the mask is defined by TAPRIO_SUPPORTED_FLAGS.
> The mutual exclusivity of the two flags TCA_TAPRIO_ATTR_FLAG_FULL_OFFLOAD
> and TCA_TAPRIO_ATTR_FLAG_TXTIME_ASSIST is still checked manually.
> 
> Changes since RFC:
> - fixed reversed xmas tree
> - use NL_SET_ERR_MSG_MOD() for both invalid configuration
> 
> Changes since v1 (https://lore.kernel.org/netdev/b90a8935-ab4b-48e2-a21d-1efc528b2788@gmail.com/T/#t):
> - Changed NL_SET_ERR_MSG_MOD to NL_SET_ERR_MSG_ATTR when wrong flags
>   issued
> - Changed __u32 to u32
> 
> Signed-off-by: Alessandro Marcolini <alessandromarcolini99@gmail.com>

...

> @@ -1863,12 +1827,28 @@ static int taprio_change(struct Qdisc *sch, struct nlattr *opt,
>  	if (tb[TCA_TAPRIO_ATTR_PRIOMAP])
>  		mqprio = nla_data(tb[TCA_TAPRIO_ATTR_PRIOMAP]);
>  
> -	err = taprio_new_flags(tb[TCA_TAPRIO_ATTR_FLAGS],
> -			       q->flags, extack);
> -	if (err < 0)
> -		return err;
> +	/* The semantics of the 'flags' argument in relation to 'change()'
> +	 * requests, are interpreted following two rules (which are applied in
> +	 * this order): (1) an omitted 'flags' argument is interpreted as
> +	 * zero; (2) the 'flags' of a "running" taprio instance cannot be
> +	 * changed.
> +	 */
> +	taprio_flags = tb[TCA_TAPRIO_ATTR_FLAGS] ? nla_get_u32(tb[TCA_TAPRIO_ATTR_FLAGS]) : 0;
>  
> -	q->flags = err;
> +	/* txtime-assist and full offload are mutually exclusive */
> +	if ((taprio_flags & TCA_TAPRIO_ATTR_FLAG_TXTIME_ASSIST) &&
> +	    (taprio_flags & TCA_TAPRIO_ATTR_FLAG_FULL_OFFLOAD)) {
> +		NL_SET_ERR_MSG_ATTR(extack,
> +				    "TXTIME_ASSIST and FULL_OFFLOAD are mutually exclusive");

Hi Alessandro,

Perhaps there was an uncommitted local change, but
I think an attribute is required as the second argument to
NL_SET_ERR_MSG_ATTR(). Without that I see this code fails to compile.

> +		return -EINVAL;
> +	}
> +
> +	if (q->flags != TAPRIO_FLAGS_INVALID && q->flags != taprio_flags) {
> +		NL_SET_ERR_MSG_MOD(extack,
> +				   "Changing 'flags' of a running schedule is not supported");
> +		return -EOPNOTSUPP;
> +	}
> +	q->flags = taprio_flags;
>  
>  	err = taprio_parse_mqprio_opt(dev, mqprio, extack, q->flags);
>  	if (err < 0)

-- 
pw-bot: changes-requested