Re: [PATCH net-next v2] net/sched: actions report errors with extack

[Jakub Kicinski]

On Mon,  5 Feb 2024 10:52:40 -0800 Stephen Hemminger wrote:
> -static int tcf_bpf_init_from_ops(struct nlattr **tb, struct tcf_bpf_cfg *cfg)
> +static int tcf_bpf_init_from_ops(struct nlattr **tb, struct tcf_bpf_cfg *cfg,
> +				 struct netlink_ext_ack *extack)
>  {
>  	struct sock_filter *bpf_ops;
>  	struct sock_fprog_kern fprog_tmp;
> @@ -193,12 +194,17 @@ static int tcf_bpf_init_from_ops(struct nlattr **tb, struct tcf_bpf_cfg *cfg)
>  	int ret;
>  
>  	bpf_num_ops = nla_get_u16(tb[TCA_ACT_BPF_OPS_LEN]);
> -	if (bpf_num_ops	> BPF_MAXINSNS || bpf_num_ops == 0)
> +	if (bpf_num_ops	> BPF_MAXINSNS || bpf_num_ops == 0) {
> +		NL_SET_ERR_MSG_FMT_MOD(extack,
> +				       "Invalid number of BPF instructions %u", bpf_num_ops);

out of range seems better than invalid.
In fact it should be added to the policy.

>  		return -EINVAL;
> +	}
>  
>  	bpf_size = bpf_num_ops * sizeof(*bpf_ops);
> -	if (bpf_size != nla_len(tb[TCA_ACT_BPF_OPS]))
> +	if (bpf_size != nla_len(tb[TCA_ACT_BPF_OPS])) {
> +		NL_SET_ERR_MSG_FMT_MOD(extack, "BPF instruction size %u", bpf_size);

Doesn't sound like an error.
Something about number of instructions not matching the program size
would be better

>  		return -EINVAL;
> +	}
>  
>  	bpf_ops = kmemdup(nla_data(tb[TCA_ACT_BPF_OPS]), bpf_size, GFP_KERNEL);
>  	if (bpf_ops == NULL)
> @@ -221,7 +227,8 @@ static int tcf_bpf_init_from_ops(struct nlattr **tb, struct tcf_bpf_cfg *cfg)
>  	return 0;
>  }
>  
> -static int tcf_bpf_init_from_efd(struct nlattr **tb, struct tcf_bpf_cfg *cfg)
> +static int tcf_bpf_init_from_efd(struct nlattr **tb, struct tcf_bpf_cfg *cfg,
> +				 struct netlink_ext_ack *extack)
>  {
>  	struct bpf_prog *fp;
>  	char *name = NULL;
> @@ -230,8 +237,10 @@ static int tcf_bpf_init_from_efd(struct nlattr **tb, struct tcf_bpf_cfg *cfg)
>  	bpf_fd = nla_get_u32(tb[TCA_ACT_BPF_FD]);
>  
>  	fp = bpf_prog_get_type(bpf_fd, BPF_PROG_TYPE_SCHED_ACT);
> -	if (IS_ERR(fp))
> +	if (IS_ERR(fp)) {
> +		NL_SET_ERR_MSG_MOD(extack, "BPF program type mismatch");
>  		return PTR_ERR(fp);
> +	}
>  
>  	if (tb[TCA_ACT_BPF_NAME]) {
>  		name = nla_memdup(tb[TCA_ACT_BPF_NAME], GFP_KERNEL);
> @@ -292,16 +301,20 @@ static int tcf_bpf_init(struct net *net, struct nlattr *nla,
>  	int ret, res = 0;
>  	u32 index;
>  
> -	if (!nla)
> +	if (!nla) {
> +		NL_SET_ERR_MSG_MOD(extack, "Bpf requires attributes to be passed");

You use "BPF" (capitals) elsewhere. Also not sure the "BPF" prefix is
actually needed, given the _MOD() will prefix this with cls_bpf already.

>  		return -EINVAL;
> +	}
>  
>  	ret = nla_parse_nested_deprecated(tb, TCA_ACT_BPF_MAX, nla,
>  					  act_bpf_policy, NULL);
>  	if (ret < 0)
>  		return ret;
>  
> -	if (!tb[TCA_ACT_BPF_PARMS])
> +	if (NL_REQ_ATTR_CHECK(extack, nla, tb, TCA_ACT_BPF_PARMS)) {
> +		NL_SET_ERR_MSG(extack, "Missing required attribute");

Please fix the userspace to support missing attr parsing instead.

>  		return -EINVAL;
> +	}
>  
>  	parm = nla_data(tb[TCA_ACT_BPF_PARMS]);
>  	index = parm->index;
> @@ -336,14 +349,15 @@ static int tcf_bpf_init(struct net *net, struct nlattr *nla,
>  	is_ebpf = tb[TCA_ACT_BPF_FD];
>  
>  	if (is_bpf == is_ebpf) {
> +		NL_SET_ERR_MSG_MOD(extack, "Can not specify both BPF fd and ops");

bytecode would be better than ops

>  		ret = -EINVAL;
>  		goto put_chain;
>  	}
>  
>  	memset(&cfg, 0, sizeof(cfg));
>  
> -	ret = is_bpf ? tcf_bpf_init_from_ops(tb, &cfg) :
> -		       tcf_bpf_init_from_efd(tb, &cfg);
> +	ret = is_bpf ? tcf_bpf_init_from_ops(tb, &cfg, extack) :
> +		       tcf_bpf_init_from_efd(tb, &cfg, extack);
>  	if (ret < 0)
>  		goto put_chain;