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 <[email protected]>
---
v2 - use NL_REQ_ATTR_CHECK()
net/sched/act_bpf.c | 32 +++++++++++++++++++++++---------
net/sched/act_connmark.c | 8 ++++++--
net/sched/act_csum.c | 9 +++++++--
net/sched/act_ct.c | 5 +++--
net/sched/act_ctinfo.c | 6 +++---
net/sched/act_gact.c | 14 +++++++++++---
net/sched/act_gate.c | 15 +++++++++++----
net/sched/act_ife.c | 8 ++++++--
net/sched/act_mirred.c | 6 ++++--
net/sched/act_nat.c | 9 +++++++--
net/sched/act_police.c | 13 ++++++++++---
net/sched/act_sample.c | 8 ++++++--
net/sched/act_simple.c | 11 ++++++++---
net/sched/act_skbedit.c | 17 ++++++++++++-----
net/sched/act_skbmod.c | 9 +++++++--
net/sched/act_vlan.c | 8 ++++++--
16 files changed, 130 insertions(+), 48 deletions(-)
diff --git a/net/sched/act_bpf.c b/net/sched/act_bpf.c
index 0e3cf11ae5fc..4dc6f27a4809 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,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);
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);
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");
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");
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");
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 0fce631e7c91..00c7e52d91ca 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 (NL_REQ_ATTR_CHECK(extack, nla, tb, TCA_CONNMARK_PARMS)) {
+ NL_SET_ERR_MSG(extack, "Missing required attribute");
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 5cc8e407e791..b83e6d5f10ee 100644
--- a/net/sched/act_csum.c
+++ b/net/sched/act_csum.c
@@ -55,16 +55,21 @@ 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 (NL_REQ_ATTR_CHECK(extack, nla, tb, TCA_CSUM_PARMS)) {
+ NL_SET_ERR_MSG(extack, "Missing required attribute");
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_ct.c b/net/sched/act_ct.c
index baac083fd8f1..7984f9f6ea2c 100644
--- a/net/sched/act_ct.c
+++ b/net/sched/act_ct.c
@@ -1329,10 +1329,11 @@ static int tcf_ct_init(struct net *net, struct nlattr *nla,
if (err < 0)
return err;
- if (!tb[TCA_CT_PARMS]) {
- NL_SET_ERR_MSG_MOD(extack, "Missing required ct parameters");
+ if (NL_REQ_ATTR_CHECK(extack, nla, tb, TCA_CT_PARMS)) {
+ NL_SET_ERR_MSG(extack, "Missing required attribute");
return -EINVAL;
}
+
parm = nla_data(tb[TCA_CT_PARMS]);
index = parm->index;
err = tcf_idr_check_alloc(tn, &index, a, bind);
diff --git a/net/sched/act_ctinfo.c b/net/sched/act_ctinfo.c
index 5dd41a012110..dde047b6b839 100644
--- a/net/sched/act_ctinfo.c
+++ b/net/sched/act_ctinfo.c
@@ -178,11 +178,11 @@ static int tcf_ctinfo_init(struct net *net, struct nlattr *nla,
if (err < 0)
return err;
- if (!tb[TCA_CTINFO_ACT]) {
- NL_SET_ERR_MSG_MOD(extack,
- "Missing required TCA_CTINFO_ACT attribute");
+ if (NL_REQ_ATTR_CHECK(extack, nla, tb, TCA_CTINFO_ACT)) {
+ NL_SET_ERR_MSG(extack, "Missing required attribute");
return -EINVAL;
}
+
actparm = nla_data(tb[TCA_CTINFO_ACT]);
/* do some basic validation here before dynamically allocating things */
diff --git a/net/sched/act_gact.c b/net/sched/act_gact.c
index e949280eb800..42c6b8d9002d 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 (NL_REQ_ATTR_CHECK(extack, nla, tb, TCA_GACT_PARMS)) {
+ NL_SET_ERR_MSG(extack, "Missing required attribute");
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 1dd74125398a..3e8056a2c304 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 (NL_REQ_ATTR_CHECK(extack, nla, tb, TCA_GATE_PARMS)) {
+ NL_SET_ERR_MSG(extack, "Missing required attribute");
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 107c6d83dc5c..b22881363029 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 (NL_REQ_ATTR_CHECK(extack, nla, tb, TCA_IFE_PARMS)) {
+ NL_SET_ERR_MSG(extack, "Missing required attribute");
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_mirred.c b/net/sched/act_mirred.c
index 93a96e9d8d90..f1bdd19e0bbb 100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -124,10 +124,12 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
mirred_policy, extack);
if (ret < 0)
return ret;
- if (!tb[TCA_MIRRED_PARMS]) {
- NL_SET_ERR_MSG_MOD(extack, "Missing required mirred parameters");
+
+ if (NL_REQ_ATTR_CHECK(extack, nla, tb, TCA_MIRRED_PARMS)) {
+ NL_SET_ERR_MSG(extack, "Missing required attribute");
return -EINVAL;
}
+
parm = nla_data(tb[TCA_MIRRED_PARMS]);
index = parm->index;
err = tcf_idr_check_alloc(tn, &index, a, bind);
diff --git a/net/sched/act_nat.c b/net/sched/act_nat.c
index d541f553805f..42019977514e 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 (NL_REQ_ATTR_CHECK(extack, nla, tb, TCA_NAT_PARMS)) {
+ NL_SET_ERR_MSG_MOD(extack, "Missing required NAT parameters");
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 8555125ed34d..17708fe32ad1 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 (NL_REQ_ATTR_CHECK(extack, nla, 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 a69b53d54039..0492df144b39 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 (NL_REQ_ATTR_CHECK(extack, nla, 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 f3abe0545989..0c56c8c9ef44 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 (NL_REQ_ATTR_CHECK(extack, nla, 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;
@@ -120,7 +124,8 @@ static int tcf_simp_init(struct net *net, struct nlattr *nla,
if (exists && bind)
return ACT_P_BOUND;
- if (tb[TCA_DEF_DATA] == NULL) {
+ if (NL_REQ_ATTR_CHECK(extack, NULL, tb, TCA_DEF_DATA)) {
+ 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 1f1d9ce3e968..e9c4f2befb8b 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 (NL_REQ_ATTR_CHECK(extack, nla, 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;
}
@@ -182,8 +188,8 @@ static int tcf_skbedit_init(struct net *net, struct nlattr *nla,
if (*pure_flags & SKBEDIT_F_TXQ_SKBHASH) {
u16 *queue_mapping_max;
- if (!tb[TCA_SKBEDIT_QUEUE_MAPPING] ||
- !tb[TCA_SKBEDIT_QUEUE_MAPPING_MAX]) {
+ if (NL_REQ_ATTR_CHECK(extack, NULL, tb, TCA_SKBEDIT_QUEUE_MAPPING) ||
+ NL_REQ_ATTR_CHECK(extack, NULL, tb, TCA_SKBEDIT_QUEUE_MAPPING_MAX)) {
NL_SET_ERR_MSG_MOD(extack, "Missing required range of queue_mapping.");
return -EINVAL;
}
@@ -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 39945b139c48..19b35666f357 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 (NL_REQ_ATTR_CHECK(extack, nla, 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 22f4b1e8ade9..414129539c4a 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 (NL_REQ_ATTR_CHECK(extack, nla, 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
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;
On Thu, 8 Feb 2024 18:27:31 -0800
Jakub Kicinski <[email protected]> wrote:
> > - 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.
I was just addressing the error handling. This keeps the same impact as
before, i.e no userspace API change.
On Fri, 9 Feb 2024 13:11:19 -0800 Stephen Hemminger wrote:
> On Thu, 8 Feb 2024 18:27:31 -0800
> Jakub Kicinski <[email protected]> wrote:
>
> > > - 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.
>
> I was just addressing the error handling. This keeps the same impact as
> before, i.e no userspace API change.
I mean that NL_REQ_ATTR_CHECK() should be more than enough by itself.
We have full TC specs in YAML now, we can hack up a script to generate
reverse parsing tables for iproute2 even if you don't want to go full
YNL.
On Fri, 9 Feb 2024 13:41:12 -0800
Jakub Kicinski <[email protected]> wrote:
> On Fri, 9 Feb 2024 13:11:19 -0800 Stephen Hemminger wrote:
> > On Thu, 8 Feb 2024 18:27:31 -0800
> > Jakub Kicinski <[email protected]> wrote:
> >
> > > > - 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.
> >
> > I was just addressing the error handling. This keeps the same impact as
> > before, i.e no userspace API change.
>
> I mean that NL_REQ_ATTR_CHECK() should be more than enough by itself.
> We have full TC specs in YAML now, we can hack up a script to generate
> reverse parsing tables for iproute2 even if you don't want to go full
> YNL.
Ok, then will take the err msg across all places using NL_REQ_ATTR_CHECK?
Would prefer not to add the complexity of reverse parsing tables, that gets ugly fast.
On Fri, 9 Feb 2024 15:58:30 -0800 Stephen Hemminger wrote:
> > I mean that NL_REQ_ATTR_CHECK() should be more than enough by itself.
> > We have full TC specs in YAML now, we can hack up a script to generate
> > reverse parsing tables for iproute2 even if you don't want to go full
> > YNL.
>
> Ok, then will take the err msg across all places using NL_REQ_ATTR_CHECK?
Take _out_ ? That'd be great, yup.
> Would prefer not to add the complexity of reverse parsing tables, that gets ugly fast.
"reverse" is probably to strong. It's just a parse which parses
the request instead of the response. ethtool CLI does it, too.
On Fri, 9 Feb 2024 18:31:33 -0800
Jakub Kicinski <[email protected]> wrote:
> On Fri, 9 Feb 2024 15:58:30 -0800 Stephen Hemminger wrote:
> > > I mean that NL_REQ_ATTR_CHECK() should be more than enough by itself.
> > > We have full TC specs in YAML now, we can hack up a script to generate
> > > reverse parsing tables for iproute2 even if you don't want to go full
> > > YNL.
> >
> > Ok, then will take the err msg across all places using NL_REQ_ATTR_CHECK?
>
> Take _out_ ? That'd be great, yup.
I don't think that is actually a good idea because new kernel needs to still
report errors to older userspace iproute2.
This is kind of academic hair splitting, iproute2 doesn't send these kind
of missing part messages. It is more from test suites, or from custom
user programs; found this when looking at DPDK device driver that was
sending its own netlink.