| Message ID | 20240131170019.106122-1-stephen@networkplumber.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net/sched: report errors with extack | expand |
On Wed, Jan 31, 2024 at 12:00 PM Stephen Hemminger <stephen@networkplumber.org> wrote: > > While working a BPF action, found that the error handling was > limited. The support of external ack was only added to some > but not all actions. > > When an action detects invalid parameters, it should > be adding an external ack to netlink so that the user is > able to diagnose the issue. > > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> > --- > net/sched/act_bpf.c | 31 ++++++++++++++++++++++--------- > net/sched/act_connmark.c | 8 ++++++-- > net/sched/act_csum.c | 8 ++++++-- > net/sched/act_gact.c | 14 +++++++++++--- > net/sched/act_gate.c | 15 +++++++++++---- > net/sched/act_ife.c | 8 ++++++-- > net/sched/act_nat.c | 9 +++++++-- > net/sched/act_police.c | 13 ++++++++++--- > net/sched/act_sample.c | 8 ++++++-- > net/sched/act_simple.c | 9 +++++++-- > net/sched/act_skbedit.c | 13 ++++++++++--- > net/sched/act_skbmod.c | 9 +++++++-- > net/sched/act_vlan.c | 8 ++++++-- > 13 files changed, 115 insertions(+), 38 deletions(-) > > diff --git a/net/sched/act_bpf.c b/net/sched/act_bpf.c > index 6cfee6658103..f8a03d3bbf20 100644 > --- a/net/sched/act_bpf.c > +++ b/net/sched/act_bpf.c > @@ -184,7 +184,8 @@ static const struct nla_policy act_bpf_policy[TCA_ACT_BPF_MAX + 1] = { > .len = sizeof(struct sock_filter) * BPF_MAXINSNS }, > }; > > -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,16 @@ 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_MOD(extack, "Invalid number of BPF instructions"); > 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_MOD(extack, "BPF instruction size"); > return -EINVAL; > + } > > bpf_ops = kmemdup(nla_data(tb[TCA_ACT_BPF_OPS]), bpf_size, GFP_KERNEL); > if (bpf_ops == NULL) > @@ -221,7 +226,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 +236,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 +300,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"); > 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 (!tb[TCA_ACT_BPF_PARMS]) { if (NL_REQ_ATTR_CHECK(extack, NULL, tb, TCA_ACT_BPF_PARMS) to set the other extack fields > + NL_SET_ERR_MSG_MOD(extack, "Missing required bpf parameters"); > return -EINVAL; > + } > > parm = nla_data(tb[TCA_ACT_BPF_PARMS]); > index = parm->index; > @@ -336,14 +348,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"); > 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; > > diff --git a/net/sched/act_connmark.c b/net/sched/act_connmark.c > index f8762756657d..0964d10dfc04 100644 > --- a/net/sched/act_connmark.c > +++ b/net/sched/act_connmark.c > @@ -110,16 +110,20 @@ static int tcf_connmark_init(struct net *net, struct nlattr *nla, > int ret = 0, err; > u32 index; > > - if (!nla) > + if (!nla) { > + NL_SET_ERR_MSG_MOD(extack, "Connmark requires attributes to be passed"); > return -EINVAL; > + } > > ret = nla_parse_nested_deprecated(tb, TCA_CONNMARK_MAX, nla, > connmark_policy, NULL); > if (ret < 0) > return ret; > > - if (!tb[TCA_CONNMARK_PARMS]) > + if (!tb[TCA_CONNMARK_PARMS]) { Same thing.. > + NL_SET_ERR_MSG(extack, "Missing required connmark parameters"); > return -EINVAL; > + } > > nparms = kzalloc(sizeof(*nparms), GFP_KERNEL); > if (!nparms) > diff --git a/net/sched/act_csum.c b/net/sched/act_csum.c > index 7f8b1f2f2ed9..7c7f74e37528 100644 > --- a/net/sched/act_csum.c > +++ b/net/sched/act_csum.c > @@ -55,16 +55,20 @@ static int tcf_csum_init(struct net *net, struct nlattr *nla, > int ret = 0, err; > u32 index; > > - if (nla == NULL) > + if (!nla) { > + NL_SET_ERR_MSG_MOD(extack, "Checksum requires attributes to be passed"); > return -EINVAL; > + } > > err = nla_parse_nested_deprecated(tb, TCA_CSUM_MAX, nla, csum_policy, > NULL); > if (err < 0) > return err; > > - if (tb[TCA_CSUM_PARMS] == NULL) > + if (!tb[TCA_CSUM_PARMS]) { Same thing... > + NL_SET_ERR_MSG(extack, "Missing required checksum parameters"); > return -EINVAL; > + } > parm = nla_data(tb[TCA_CSUM_PARMS]); > index = parm->index; > err = tcf_idr_check_alloc(tn, &index, a, bind); > diff --git a/net/sched/act_gact.c b/net/sched/act_gact.c > index 4af3b7ec249f..5d04bcd5115e 100644 > --- a/net/sched/act_gact.c > +++ b/net/sched/act_gact.c > @@ -68,16 +68,21 @@ static int tcf_gact_init(struct net *net, struct nlattr *nla, > struct tc_gact_p *p_parm = NULL; > #endif > > - if (nla == NULL) > + if (!nla) { > + NL_SET_ERR_MSG(extack, "Gact requires attributes to be passed"); > return -EINVAL; > + } > > err = nla_parse_nested_deprecated(tb, TCA_GACT_MAX, nla, gact_policy, > NULL); > if (err < 0) > return err; > > - if (tb[TCA_GACT_PARMS] == NULL) > + if (!tb[TCA_GACT_PARMS]) { Same.. > + NL_SET_ERR_MSG_MOD(extack, "Missing required gact parameters"); > return -EINVAL; > + } > + > parm = nla_data(tb[TCA_GACT_PARMS]); > index = parm->index; > > @@ -87,8 +92,11 @@ static int tcf_gact_init(struct net *net, struct nlattr *nla, > #else > if (tb[TCA_GACT_PROB]) { > p_parm = nla_data(tb[TCA_GACT_PROB]); > - if (p_parm->ptype >= MAX_RAND) > + if (p_parm->ptype >= MAX_RAND) { > + NL_SET_ERR_MSG(extack, "Invalid ptype in gact prob"); > return -EINVAL; > + } > + > if (TC_ACT_EXT_CMP(p_parm->paction, TC_ACT_GOTO_CHAIN)) { > NL_SET_ERR_MSG(extack, > "goto chain not allowed on fallback"); > diff --git a/net/sched/act_gate.c b/net/sched/act_gate.c > index c681cd011afd..c9e32822938c 100644 > --- a/net/sched/act_gate.c > +++ b/net/sched/act_gate.c > @@ -239,8 +239,10 @@ static int parse_gate_list(struct nlattr *list_attr, > int err, rem; > int i = 0; > > - if (!list_attr) > + if (!list_attr) { > + NL_SET_ERR_MSG(extack, "Gate missing attributes"); > return -EINVAL; > + } > > nla_for_each_nested(n, list_attr, rem) { > if (nla_type(n) != TCA_GATE_ONE_ENTRY) { > @@ -317,15 +319,19 @@ static int tcf_gate_init(struct net *net, struct nlattr *nla, > ktime_t start; > u32 index; > > - if (!nla) > + if (!nla) { > + NL_SET_ERR_MSG_MOD(extack, "Gate requires attributes to be passed"); > return -EINVAL; > + } > > err = nla_parse_nested(tb, TCA_GATE_MAX, nla, gate_policy, extack); > if (err < 0) > return err; > > - if (!tb[TCA_GATE_PARMS]) > + if (!tb[TCA_GATE_PARMS]) { Here... > + NL_SET_ERR_MSG_MOD(extack, "Missing required gate parameters"); > return -EINVAL; > + } > > if (tb[TCA_GATE_CLOCKID]) { > clockid = nla_get_s32(tb[TCA_GATE_CLOCKID]); > @@ -343,7 +349,7 @@ static int tcf_gate_init(struct net *net, struct nlattr *nla, > tk_offset = TK_OFFS_TAI; > break; > default: > - NL_SET_ERR_MSG(extack, "Invalid 'clockid'"); > + NL_SET_ERR_MSG_MOD(extack, "Invalid 'clockid'"); > return -EINVAL; > } > } > @@ -409,6 +415,7 @@ static int tcf_gate_init(struct net *net, struct nlattr *nla, > cycle = ktime_add_ns(cycle, entry->interval); > cycletime = cycle; > if (!cycletime) { > + NL_SET_ERR_MSG_MOD(extack, "cycle time is zero"); > err = -EINVAL; > goto chain_put; > } > diff --git a/net/sched/act_ife.c b/net/sched/act_ife.c > index 0e867d13beb5..85a58cfb23f3 100644 > --- a/net/sched/act_ife.c > +++ b/net/sched/act_ife.c > @@ -508,8 +508,10 @@ static int tcf_ife_init(struct net *net, struct nlattr *nla, > if (err < 0) > return err; > > - if (!tb[TCA_IFE_PARMS]) > + if (!tb[TCA_IFE_PARMS]) { Here... Going to stop here - there are more further down. You get the gist.. cheers, jamal > + NL_SET_ERR_MSG_MOD(extack, "Missing required ife parameters"); > return -EINVAL; > + } > > parm = nla_data(tb[TCA_IFE_PARMS]); > > @@ -517,8 +519,10 @@ static int tcf_ife_init(struct net *net, struct nlattr *nla, > * they cannot run as the same time. Check on all other values which > * are not supported right now. > */ > - if (parm->flags & ~IFE_ENCODE) > + if (parm->flags & ~IFE_ENCODE) { > + NL_SET_ERR_MSG_MOD(extack, "Invalid ife flag parameter"); > return -EINVAL; > + } > > p = kzalloc(sizeof(*p), GFP_KERNEL); > if (!p) > diff --git a/net/sched/act_nat.c b/net/sched/act_nat.c > index a180e724634e..a990d0c626cd 100644 > --- a/net/sched/act_nat.c > +++ b/net/sched/act_nat.c > @@ -46,16 +46,21 @@ static int tcf_nat_init(struct net *net, struct nlattr *nla, struct nlattr *est, > struct tcf_nat *p; > u32 index; > > - if (nla == NULL) > + if (!nla) { > + NL_SET_ERR_MSG_MOD(extack, "Nat action requires attributes"); > return -EINVAL; > + } > > err = nla_parse_nested_deprecated(tb, TCA_NAT_MAX, nla, nat_policy, > NULL); > if (err < 0) > return err; > > - if (tb[TCA_NAT_PARMS] == NULL) > + if (!tb[TCA_NAT_PARMS]) { > + NL_SET_ERR_MSG_MOD(extack, "Nat action parameters missing"); > return -EINVAL; > + } > + > parm = nla_data(tb[TCA_NAT_PARMS]); > index = parm->index; > err = tcf_idr_check_alloc(tn, &index, a, bind); > diff --git a/net/sched/act_police.c b/net/sched/act_police.c > index e119b4a3db9f..3eb41233257b 100644 > --- a/net/sched/act_police.c > +++ b/net/sched/act_police.c > @@ -56,19 +56,26 @@ static int tcf_police_init(struct net *net, struct nlattr *nla, > u64 rate64, prate64; > u64 pps, ppsburst; > > - if (nla == NULL) > + if (!nla) { > + NL_SET_ERR_MSG_MOD(extack, "Police requires attributes"); > return -EINVAL; > + } > > err = nla_parse_nested_deprecated(tb, TCA_POLICE_MAX, nla, > police_policy, NULL); > if (err < 0) > return err; > > - if (tb[TCA_POLICE_TBF] == NULL) > + if (!tb[TCA_POLICE_TBF]) { > + NL_SET_ERR_MSG_MOD(extack, "Missing required police action parameters"); > return -EINVAL; > + } > + > size = nla_len(tb[TCA_POLICE_TBF]); > - if (size != sizeof(*parm) && size != sizeof(struct tc_police_compat)) > + if (size != sizeof(*parm) && size != sizeof(struct tc_police_compat)) { > + NL_SET_ERR_MSG_MOD(extack, "Invalid size for police action parameters"); > return -EINVAL; > + } > > parm = nla_data(tb[TCA_POLICE_TBF]); > index = parm->index; > diff --git a/net/sched/act_sample.c b/net/sched/act_sample.c > index c5c61efe6db4..2442e001d92e 100644 > --- a/net/sched/act_sample.c > +++ b/net/sched/act_sample.c > @@ -49,15 +49,19 @@ static int tcf_sample_init(struct net *net, struct nlattr *nla, > bool exists = false; > int ret, err; > > - if (!nla) > + if (!nla) { > + NL_SET_ERR_MSG_MOD(extack, "Sample requires attributes to be passed"); > return -EINVAL; > + } > ret = nla_parse_nested_deprecated(tb, TCA_SAMPLE_MAX, nla, > sample_policy, NULL); > if (ret < 0) > return ret; > > - if (!tb[TCA_SAMPLE_PARMS]) > + if (!tb[TCA_SAMPLE_PARMS]) { > + NL_SET_ERR_MSG_MOD(extack, "Missing required sample action parameters"); > return -EINVAL; > + } > > parm = nla_data(tb[TCA_SAMPLE_PARMS]); > index = parm->index; > diff --git a/net/sched/act_simple.c b/net/sched/act_simple.c > index 0a3e92888295..02b8e42c1bdd 100644 > --- a/net/sched/act_simple.c > +++ b/net/sched/act_simple.c > @@ -100,16 +100,20 @@ static int tcf_simp_init(struct net *net, struct nlattr *nla, > int ret = 0, err; > u32 index; > > - if (nla == NULL) > + if (!nla) { > + NL_SET_ERR_MSG_MOD(extack, "Sample requires attributes to be passed"); > return -EINVAL; > + } > > err = nla_parse_nested_deprecated(tb, TCA_DEF_MAX, nla, simple_policy, > NULL); > if (err < 0) > return err; > > - if (tb[TCA_DEF_PARMS] == NULL) > + if (!tb[TCA_DEF_PARMS]) { > + NL_SET_ERR_MSG_MOD(extack, "Missing required sample action parameters"); > return -EINVAL; > + } > > parm = nla_data(tb[TCA_DEF_PARMS]); > index = parm->index; > @@ -121,6 +125,7 @@ static int tcf_simp_init(struct net *net, struct nlattr *nla, > return ACT_P_BOUND; > > if (tb[TCA_DEF_DATA] == NULL) { > + NL_SET_ERR_MSG_MOD(extack, "Missing simple action default data"); > if (exists) > tcf_idr_release(*a, bind); > else > diff --git a/net/sched/act_skbedit.c b/net/sched/act_skbedit.c > index 754f78b35bb8..671ca64a2c33 100644 > --- a/net/sched/act_skbedit.c > +++ b/net/sched/act_skbedit.c > @@ -133,16 +133,20 @@ static int tcf_skbedit_init(struct net *net, struct nlattr *nla, > int ret = 0, err; > u32 index; > > - if (nla == NULL) > + if (!nla) { > + NL_SET_ERR_MSG_MOD(extack, "Skbedit requires attributes to be passed"); > return -EINVAL; > + } > > err = nla_parse_nested_deprecated(tb, TCA_SKBEDIT_MAX, nla, > skbedit_policy, NULL); > if (err < 0) > return err; > > - if (tb[TCA_SKBEDIT_PARMS] == NULL) > + if (!tb[TCA_SKBEDIT_PARMS]) { > + NL_SET_ERR_MSG_MOD(extack, "Missing required skbedit parameters"); > return -EINVAL; > + } > > if (tb[TCA_SKBEDIT_PRIORITY] != NULL) { > flags |= SKBEDIT_F_PRIORITY; > @@ -161,8 +165,10 @@ static int tcf_skbedit_init(struct net *net, struct nlattr *nla, > > if (tb[TCA_SKBEDIT_PTYPE] != NULL) { > ptype = nla_data(tb[TCA_SKBEDIT_PTYPE]); > - if (!skb_pkt_type_ok(*ptype)) > + if (!skb_pkt_type_ok(*ptype)) { > + NL_SET_ERR_MSG_MOD(extack, "ptype is not a valid"); > return -EINVAL; > + } > flags |= SKBEDIT_F_PTYPE; > } > > @@ -212,6 +218,7 @@ static int tcf_skbedit_init(struct net *net, struct nlattr *nla, > return ACT_P_BOUND; > > if (!flags) { > + NL_SET_ERR_MSG_MOD(extack, "No skbedit action flag"); > if (exists) > tcf_idr_release(*a, bind); > else > diff --git a/net/sched/act_skbmod.c b/net/sched/act_skbmod.c > index bcb673ab0008..c80828cdeb69 100644 > --- a/net/sched/act_skbmod.c > +++ b/net/sched/act_skbmod.c > @@ -119,16 +119,20 @@ static int tcf_skbmod_init(struct net *net, struct nlattr *nla, > u16 eth_type = 0; > int ret = 0, err; > > - if (!nla) > + if (!nla) { > + NL_SET_ERR_MSG_MOD(extack, "Skbmod requires attributes to be passed"); > return -EINVAL; > + } > > err = nla_parse_nested_deprecated(tb, TCA_SKBMOD_MAX, nla, > skbmod_policy, NULL); > if (err < 0) > return err; > > - if (!tb[TCA_SKBMOD_PARMS]) > + if (!tb[TCA_SKBMOD_PARMS]) { > + NL_SET_ERR_MSG_MOD(extack, "Missing required skbmod parameters"); > return -EINVAL; > + } > > if (tb[TCA_SKBMOD_DMAC]) { > daddr = nla_data(tb[TCA_SKBMOD_DMAC]); > @@ -160,6 +164,7 @@ static int tcf_skbmod_init(struct net *net, struct nlattr *nla, > return ACT_P_BOUND; > > if (!lflags) { > + NL_SET_ERR_MSG_MOD(extack, "No skbmod action flag"); > if (exists) > tcf_idr_release(*a, bind); > else > diff --git a/net/sched/act_vlan.c b/net/sched/act_vlan.c > index 836183011a7c..b468a4c8a904 100644 > --- a/net/sched/act_vlan.c > +++ b/net/sched/act_vlan.c > @@ -134,16 +134,20 @@ static int tcf_vlan_init(struct net *net, struct nlattr *nla, > int ret = 0, err; > u32 index; > > - if (!nla) > + if (!nla) { > + NL_SET_ERR_MSG_MOD(extack, "Vlan requires attributes to be passed"); > return -EINVAL; > + } > > err = nla_parse_nested_deprecated(tb, TCA_VLAN_MAX, nla, vlan_policy, > NULL); > if (err < 0) > return err; > > - if (!tb[TCA_VLAN_PARMS]) > + if (!tb[TCA_VLAN_PARMS]) { > + NL_SET_ERR_MSG_MOD(extack, "Missing required vlan action parameters"); > return -EINVAL; > + } > parm = nla_data(tb[TCA_VLAN_PARMS]); > index = parm->index; > err = tcf_idr_check_alloc(tn, &index, a, bind); > -- > 2.43.0 >
diff --git a/net/sched/act_bpf.c b/net/sched/act_bpf.c index 6cfee6658103..f8a03d3bbf20 100644 --- a/net/sched/act_bpf.c +++ b/net/sched/act_bpf.c @@ -184,7 +184,8 @@ static const struct nla_policy act_bpf_policy[TCA_ACT_BPF_MAX + 1] = { .len = sizeof(struct sock_filter) * BPF_MAXINSNS }, }; -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,16 @@ 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_MOD(extack, "Invalid number of BPF instructions"); 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_MOD(extack, "BPF instruction size"); return -EINVAL; + } bpf_ops = kmemdup(nla_data(tb[TCA_ACT_BPF_OPS]), bpf_size, GFP_KERNEL); if (bpf_ops == NULL) @@ -221,7 +226,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 +236,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 +300,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"); 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 (!tb[TCA_ACT_BPF_PARMS]) { + NL_SET_ERR_MSG_MOD(extack, "Missing required bpf parameters"); return -EINVAL; + } parm = nla_data(tb[TCA_ACT_BPF_PARMS]); index = parm->index; @@ -336,14 +348,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"); 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; diff --git a/net/sched/act_connmark.c b/net/sched/act_connmark.c index f8762756657d..0964d10dfc04 100644 --- a/net/sched/act_connmark.c +++ b/net/sched/act_connmark.c @@ -110,16 +110,20 @@ static int tcf_connmark_init(struct net *net, struct nlattr *nla, int ret = 0, err; u32 index; - if (!nla) + if (!nla) { + NL_SET_ERR_MSG_MOD(extack, "Connmark requires attributes to be passed"); return -EINVAL; + } ret = nla_parse_nested_deprecated(tb, TCA_CONNMARK_MAX, nla, connmark_policy, NULL); if (ret < 0) return ret; - if (!tb[TCA_CONNMARK_PARMS]) + if (!tb[TCA_CONNMARK_PARMS]) { + NL_SET_ERR_MSG(extack, "Missing required connmark parameters"); return -EINVAL; + } nparms = kzalloc(sizeof(*nparms), GFP_KERNEL); if (!nparms) diff --git a/net/sched/act_csum.c b/net/sched/act_csum.c index 7f8b1f2f2ed9..7c7f74e37528 100644 --- a/net/sched/act_csum.c +++ b/net/sched/act_csum.c @@ -55,16 +55,20 @@ static int tcf_csum_init(struct net *net, struct nlattr *nla, int ret = 0, err; u32 index; - if (nla == NULL) + if (!nla) { + NL_SET_ERR_MSG_MOD(extack, "Checksum requires attributes to be passed"); return -EINVAL; + } err = nla_parse_nested_deprecated(tb, TCA_CSUM_MAX, nla, csum_policy, NULL); if (err < 0) return err; - if (tb[TCA_CSUM_PARMS] == NULL) + if (!tb[TCA_CSUM_PARMS]) { + NL_SET_ERR_MSG(extack, "Missing required checksum parameters"); return -EINVAL; + } parm = nla_data(tb[TCA_CSUM_PARMS]); index = parm->index; err = tcf_idr_check_alloc(tn, &index, a, bind); diff --git a/net/sched/act_gact.c b/net/sched/act_gact.c index 4af3b7ec249f..5d04bcd5115e 100644 --- a/net/sched/act_gact.c +++ b/net/sched/act_gact.c @@ -68,16 +68,21 @@ static int tcf_gact_init(struct net *net, struct nlattr *nla, struct tc_gact_p *p_parm = NULL; #endif - if (nla == NULL) + if (!nla) { + NL_SET_ERR_MSG(extack, "Gact requires attributes to be passed"); return -EINVAL; + } err = nla_parse_nested_deprecated(tb, TCA_GACT_MAX, nla, gact_policy, NULL); if (err < 0) return err; - if (tb[TCA_GACT_PARMS] == NULL) + if (!tb[TCA_GACT_PARMS]) { + NL_SET_ERR_MSG_MOD(extack, "Missing required gact parameters"); return -EINVAL; + } + parm = nla_data(tb[TCA_GACT_PARMS]); index = parm->index; @@ -87,8 +92,11 @@ static int tcf_gact_init(struct net *net, struct nlattr *nla, #else if (tb[TCA_GACT_PROB]) { p_parm = nla_data(tb[TCA_GACT_PROB]); - if (p_parm->ptype >= MAX_RAND) + if (p_parm->ptype >= MAX_RAND) { + NL_SET_ERR_MSG(extack, "Invalid ptype in gact prob"); return -EINVAL; + } + if (TC_ACT_EXT_CMP(p_parm->paction, TC_ACT_GOTO_CHAIN)) { NL_SET_ERR_MSG(extack, "goto chain not allowed on fallback"); diff --git a/net/sched/act_gate.c b/net/sched/act_gate.c index c681cd011afd..c9e32822938c 100644 --- a/net/sched/act_gate.c +++ b/net/sched/act_gate.c @@ -239,8 +239,10 @@ static int parse_gate_list(struct nlattr *list_attr, int err, rem; int i = 0; - if (!list_attr) + if (!list_attr) { + NL_SET_ERR_MSG(extack, "Gate missing attributes"); return -EINVAL; + } nla_for_each_nested(n, list_attr, rem) { if (nla_type(n) != TCA_GATE_ONE_ENTRY) { @@ -317,15 +319,19 @@ static int tcf_gate_init(struct net *net, struct nlattr *nla, ktime_t start; u32 index; - if (!nla) + if (!nla) { + NL_SET_ERR_MSG_MOD(extack, "Gate requires attributes to be passed"); return -EINVAL; + } err = nla_parse_nested(tb, TCA_GATE_MAX, nla, gate_policy, extack); if (err < 0) return err; - if (!tb[TCA_GATE_PARMS]) + if (!tb[TCA_GATE_PARMS]) { + NL_SET_ERR_MSG_MOD(extack, "Missing required gate parameters"); return -EINVAL; + } if (tb[TCA_GATE_CLOCKID]) { clockid = nla_get_s32(tb[TCA_GATE_CLOCKID]); @@ -343,7 +349,7 @@ static int tcf_gate_init(struct net *net, struct nlattr *nla, tk_offset = TK_OFFS_TAI; break; default: - NL_SET_ERR_MSG(extack, "Invalid 'clockid'"); + NL_SET_ERR_MSG_MOD(extack, "Invalid 'clockid'"); return -EINVAL; } } @@ -409,6 +415,7 @@ static int tcf_gate_init(struct net *net, struct nlattr *nla, cycle = ktime_add_ns(cycle, entry->interval); cycletime = cycle; if (!cycletime) { + NL_SET_ERR_MSG_MOD(extack, "cycle time is zero"); err = -EINVAL; goto chain_put; } diff --git a/net/sched/act_ife.c b/net/sched/act_ife.c index 0e867d13beb5..85a58cfb23f3 100644 --- a/net/sched/act_ife.c +++ b/net/sched/act_ife.c @@ -508,8 +508,10 @@ static int tcf_ife_init(struct net *net, struct nlattr *nla, if (err < 0) return err; - if (!tb[TCA_IFE_PARMS]) + if (!tb[TCA_IFE_PARMS]) { + NL_SET_ERR_MSG_MOD(extack, "Missing required ife parameters"); return -EINVAL; + } parm = nla_data(tb[TCA_IFE_PARMS]); @@ -517,8 +519,10 @@ static int tcf_ife_init(struct net *net, struct nlattr *nla, * they cannot run as the same time. Check on all other values which * are not supported right now. */ - if (parm->flags & ~IFE_ENCODE) + if (parm->flags & ~IFE_ENCODE) { + NL_SET_ERR_MSG_MOD(extack, "Invalid ife flag parameter"); return -EINVAL; + } p = kzalloc(sizeof(*p), GFP_KERNEL); if (!p) diff --git a/net/sched/act_nat.c b/net/sched/act_nat.c index a180e724634e..a990d0c626cd 100644 --- a/net/sched/act_nat.c +++ b/net/sched/act_nat.c @@ -46,16 +46,21 @@ static int tcf_nat_init(struct net *net, struct nlattr *nla, struct nlattr *est, struct tcf_nat *p; u32 index; - if (nla == NULL) + if (!nla) { + NL_SET_ERR_MSG_MOD(extack, "Nat action requires attributes"); return -EINVAL; + } err = nla_parse_nested_deprecated(tb, TCA_NAT_MAX, nla, nat_policy, NULL); if (err < 0) return err; - if (tb[TCA_NAT_PARMS] == NULL) + if (!tb[TCA_NAT_PARMS]) { + NL_SET_ERR_MSG_MOD(extack, "Nat action parameters missing"); return -EINVAL; + } + parm = nla_data(tb[TCA_NAT_PARMS]); index = parm->index; err = tcf_idr_check_alloc(tn, &index, a, bind); diff --git a/net/sched/act_police.c b/net/sched/act_police.c index e119b4a3db9f..3eb41233257b 100644 --- a/net/sched/act_police.c +++ b/net/sched/act_police.c @@ -56,19 +56,26 @@ static int tcf_police_init(struct net *net, struct nlattr *nla, u64 rate64, prate64; u64 pps, ppsburst; - if (nla == NULL) + if (!nla) { + NL_SET_ERR_MSG_MOD(extack, "Police requires attributes"); return -EINVAL; + } err = nla_parse_nested_deprecated(tb, TCA_POLICE_MAX, nla, police_policy, NULL); if (err < 0) return err; - if (tb[TCA_POLICE_TBF] == NULL) + if (!tb[TCA_POLICE_TBF]) { + NL_SET_ERR_MSG_MOD(extack, "Missing required police action parameters"); return -EINVAL; + } + size = nla_len(tb[TCA_POLICE_TBF]); - if (size != sizeof(*parm) && size != sizeof(struct tc_police_compat)) + if (size != sizeof(*parm) && size != sizeof(struct tc_police_compat)) { + NL_SET_ERR_MSG_MOD(extack, "Invalid size for police action parameters"); return -EINVAL; + } parm = nla_data(tb[TCA_POLICE_TBF]); index = parm->index; diff --git a/net/sched/act_sample.c b/net/sched/act_sample.c index c5c61efe6db4..2442e001d92e 100644 --- a/net/sched/act_sample.c +++ b/net/sched/act_sample.c @@ -49,15 +49,19 @@ static int tcf_sample_init(struct net *net, struct nlattr *nla, bool exists = false; int ret, err; - if (!nla) + if (!nla) { + NL_SET_ERR_MSG_MOD(extack, "Sample requires attributes to be passed"); return -EINVAL; + } ret = nla_parse_nested_deprecated(tb, TCA_SAMPLE_MAX, nla, sample_policy, NULL); if (ret < 0) return ret; - if (!tb[TCA_SAMPLE_PARMS]) + if (!tb[TCA_SAMPLE_PARMS]) { + NL_SET_ERR_MSG_MOD(extack, "Missing required sample action parameters"); return -EINVAL; + } parm = nla_data(tb[TCA_SAMPLE_PARMS]); index = parm->index; diff --git a/net/sched/act_simple.c b/net/sched/act_simple.c index 0a3e92888295..02b8e42c1bdd 100644 --- a/net/sched/act_simple.c +++ b/net/sched/act_simple.c @@ -100,16 +100,20 @@ static int tcf_simp_init(struct net *net, struct nlattr *nla, int ret = 0, err; u32 index; - if (nla == NULL) + if (!nla) { + NL_SET_ERR_MSG_MOD(extack, "Sample requires attributes to be passed"); return -EINVAL; + } err = nla_parse_nested_deprecated(tb, TCA_DEF_MAX, nla, simple_policy, NULL); if (err < 0) return err; - if (tb[TCA_DEF_PARMS] == NULL) + if (!tb[TCA_DEF_PARMS]) { + NL_SET_ERR_MSG_MOD(extack, "Missing required sample action parameters"); return -EINVAL; + } parm = nla_data(tb[TCA_DEF_PARMS]); index = parm->index; @@ -121,6 +125,7 @@ static int tcf_simp_init(struct net *net, struct nlattr *nla, return ACT_P_BOUND; if (tb[TCA_DEF_DATA] == NULL) { + NL_SET_ERR_MSG_MOD(extack, "Missing simple action default data"); if (exists) tcf_idr_release(*a, bind); else diff --git a/net/sched/act_skbedit.c b/net/sched/act_skbedit.c index 754f78b35bb8..671ca64a2c33 100644 --- a/net/sched/act_skbedit.c +++ b/net/sched/act_skbedit.c @@ -133,16 +133,20 @@ static int tcf_skbedit_init(struct net *net, struct nlattr *nla, int ret = 0, err; u32 index; - if (nla == NULL) + if (!nla) { + NL_SET_ERR_MSG_MOD(extack, "Skbedit requires attributes to be passed"); return -EINVAL; + } err = nla_parse_nested_deprecated(tb, TCA_SKBEDIT_MAX, nla, skbedit_policy, NULL); if (err < 0) return err; - if (tb[TCA_SKBEDIT_PARMS] == NULL) + if (!tb[TCA_SKBEDIT_PARMS]) { + NL_SET_ERR_MSG_MOD(extack, "Missing required skbedit parameters"); return -EINVAL; + } if (tb[TCA_SKBEDIT_PRIORITY] != NULL) { flags |= SKBEDIT_F_PRIORITY; @@ -161,8 +165,10 @@ static int tcf_skbedit_init(struct net *net, struct nlattr *nla, if (tb[TCA_SKBEDIT_PTYPE] != NULL) { ptype = nla_data(tb[TCA_SKBEDIT_PTYPE]); - if (!skb_pkt_type_ok(*ptype)) + if (!skb_pkt_type_ok(*ptype)) { + NL_SET_ERR_MSG_MOD(extack, "ptype is not a valid"); return -EINVAL; + } flags |= SKBEDIT_F_PTYPE; } @@ -212,6 +218,7 @@ static int tcf_skbedit_init(struct net *net, struct nlattr *nla, return ACT_P_BOUND; if (!flags) { + NL_SET_ERR_MSG_MOD(extack, "No skbedit action flag"); if (exists) tcf_idr_release(*a, bind); else diff --git a/net/sched/act_skbmod.c b/net/sched/act_skbmod.c index bcb673ab0008..c80828cdeb69 100644 --- a/net/sched/act_skbmod.c +++ b/net/sched/act_skbmod.c @@ -119,16 +119,20 @@ static int tcf_skbmod_init(struct net *net, struct nlattr *nla, u16 eth_type = 0; int ret = 0, err; - if (!nla) + if (!nla) { + NL_SET_ERR_MSG_MOD(extack, "Skbmod requires attributes to be passed"); return -EINVAL; + } err = nla_parse_nested_deprecated(tb, TCA_SKBMOD_MAX, nla, skbmod_policy, NULL); if (err < 0) return err; - if (!tb[TCA_SKBMOD_PARMS]) + if (!tb[TCA_SKBMOD_PARMS]) { + NL_SET_ERR_MSG_MOD(extack, "Missing required skbmod parameters"); return -EINVAL; + } if (tb[TCA_SKBMOD_DMAC]) { daddr = nla_data(tb[TCA_SKBMOD_DMAC]); @@ -160,6 +164,7 @@ static int tcf_skbmod_init(struct net *net, struct nlattr *nla, return ACT_P_BOUND; if (!lflags) { + NL_SET_ERR_MSG_MOD(extack, "No skbmod action flag"); if (exists) tcf_idr_release(*a, bind); else diff --git a/net/sched/act_vlan.c b/net/sched/act_vlan.c index 836183011a7c..b468a4c8a904 100644 --- a/net/sched/act_vlan.c +++ b/net/sched/act_vlan.c @@ -134,16 +134,20 @@ static int tcf_vlan_init(struct net *net, struct nlattr *nla, int ret = 0, err; u32 index; - if (!nla) + if (!nla) { + NL_SET_ERR_MSG_MOD(extack, "Vlan requires attributes to be passed"); return -EINVAL; + } err = nla_parse_nested_deprecated(tb, TCA_VLAN_MAX, nla, vlan_policy, NULL); if (err < 0) return err; - if (!tb[TCA_VLAN_PARMS]) + if (!tb[TCA_VLAN_PARMS]) { + NL_SET_ERR_MSG_MOD(extack, "Missing required vlan action parameters"); return -EINVAL; + } parm = nla_data(tb[TCA_VLAN_PARMS]); index = parm->index; err = tcf_idr_check_alloc(tn, &index, a, bind);
While working a BPF action, found that the error handling was limited. The support of external ack was only added to some but not all actions. When an action detects invalid parameters, it should be adding an external ack to netlink so that the user is able to diagnose the issue. Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> --- net/sched/act_bpf.c | 31 ++++++++++++++++++++++--------- net/sched/act_connmark.c | 8 ++++++-- net/sched/act_csum.c | 8 ++++++-- net/sched/act_gact.c | 14 +++++++++++--- net/sched/act_gate.c | 15 +++++++++++---- net/sched/act_ife.c | 8 ++++++-- net/sched/act_nat.c | 9 +++++++-- net/sched/act_police.c | 13 ++++++++++--- net/sched/act_sample.c | 8 ++++++-- net/sched/act_simple.c | 9 +++++++-- net/sched/act_skbedit.c | 13 ++++++++++--- net/sched/act_skbmod.c | 9 +++++++-- net/sched/act_vlan.c | 8 ++++++-- 13 files changed, 115 insertions(+), 38 deletions(-)