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;
