On Wed, Jan 31, 2024 at 12:00 PM Stephen Hemminger
<stephen@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx>
> ---
> 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
>
