Received-SPF: pass (google.com: domain of linux-kernel+bounces-58935-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Thu, 8 Feb 2024 18:27:31 -0800
From: Jakub Kicinski <kuba@kernel.org>
To: Stephen Hemminger <stephen@networkplumber.org>
Cc: netdev@vger.kernel.org, Alexei Starovoitov <ast@kernel.org>, Daniel
 Borkmann <daniel@iogearbox.net>, Andrii Nakryiko <andrii@kernel.org>,
 Martin KaFai Lau <martin.lau@linux.dev>, Eduard Zingerman
 <eddyz87@gmail.com>, Song Liu <song@kernel.org>, Yonghong Song
 <yonghong.song@linux.dev>, John Fastabend <john.fastabend@gmail.com>, KP
 Singh <kpsingh@kernel.org>, Stanislav Fomichev <sdf@google.com>, Hao Luo
 <haoluo@google.com>, Jiri Olsa <jolsa@kernel.org>, Jamal Hadi Salim
 <jhs@mojatatu.com>, Cong Wang <xiyou.wangcong@gmail.com>, Jiri Pirko
 <jiri@resnulli.us>, "David S. Miller" <davem@davemloft.net>, Eric Dumazet
 <edumazet@google.com>, Paolo Abeni <pabeni@redhat.com>, bpf@vger.kernel.org
 (open list:BPF [GENERAL] (Safe Dynamic Programs and Tools)),
 linux-kernel@vger.kernel.org (open list)
Subject: Re: [PATCH net-next v2] net/sched: actions report errors with
 extack
Message-ID: <20240208182731.682985dd@kernel.org>
In-Reply-To: <20240205185537.216873-1-stephen@networkplumber.org>
References: <20240205185537.216873-1-stephen@networkplumber.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

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;