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

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: <20240208182731.682985dd@kernel.org>
Date: Thu, 8 Feb 2024 18:27:31 -0800
From: Jakub Kicinski <kuba@...nel.org>
To: Stephen Hemminger <stephen@...workplumber.org>
Cc: netdev@...r.kernel.org, Alexei Starovoitov <ast@...nel.org>, Daniel
 Borkmann <daniel@...earbox.net>, Andrii Nakryiko <andrii@...nel.org>,
 Martin KaFai Lau <martin.lau@...ux.dev>, Eduard Zingerman
 <eddyz87@...il.com>, Song Liu <song@...nel.org>, Yonghong Song
 <yonghong.song@...ux.dev>, John Fastabend <john.fastabend@...il.com>, KP
 Singh <kpsingh@...nel.org>, Stanislav Fomichev <sdf@...gle.com>, Hao Luo
 <haoluo@...gle.com>, Jiri Olsa <jolsa@...nel.org>, Jamal Hadi Salim
 <jhs@...atatu.com>, Cong Wang <xiyou.wangcong@...il.com>, Jiri Pirko
 <jiri@...nulli.us>, "David S. Miller" <davem@...emloft.net>, Eric Dumazet
 <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>, bpf@...r.kernel.org
 (open list:BPF [GENERAL] (Safe Dynamic Programs and Tools)),
 linux-kernel@...r.kernel.org (open list)
Subject: Re: [PATCH net-next v2] net/sched: actions report errors with
 extack

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;