| Message ID | 1612674803-7912-1-git-send-email-wenxu@ucloud.cn |
|---|---|
| State | New |
| Headers | show |
| Series | [net,v4] net/sched: cls_flower: Reject invalid ct_state flags rules | expand |
On Mon, 8 Feb 2021 15:57:05 -0300 Marcelo Ricardo Leitner wrote: > On Sun, Feb 07, 2021 at 01:13:23PM +0800, wenxu@ucloud.cn wrote: > > --- a/net/sched/cls_flower.c > > +++ b/net/sched/cls_flower.c > > @@ -30,6 +30,11 @@ > > > > #include <uapi/linux/netfilter/nf_conntrack_common.h> > > > > +#define TCA_FLOWER_KEY_CT_FLAGS_MASK (TCA_FLOWER_KEY_CT_FLAGS_NEW | \ > > + TCA_FLOWER_KEY_CT_FLAGS_ESTABLISHED | \ > > + TCA_FLOWER_KEY_CT_FLAGS_RELATED | \ > > + TCA_FLOWER_KEY_CT_FLAGS_TRACKED) > > + > > I know Jakub had said the calculations for _MASK were complicated, but > seeing this, they seem worth, otherwise we have to manually maintain > this duplicated list of entries here. > > Maybe add just the __TCA_FLOWER_KEY_CT_FLAGS_MAX to the enum, and do > the calcs here? (to avoid having them in uapi) IDK, MAX feels rather weird for a bitfield. Someone would have to do no testing at all to miss extending the mask. If you strongly prefer to keep the MAX definition let's at least move the mask definition out of the uAPI. > > struct fl_flow_key { > > struct flow_dissector_key_meta meta; > > struct flow_dissector_key_control control;
On Mon, Feb 8, 2021 at 10:48 AM Jakub Kicinski <kuba@kernel.org> wrote: > > On Mon, 8 Feb 2021 10:41:35 -0800 Cong Wang wrote: > > On Sat, Feb 6, 2021 at 9:26 PM <wenxu@ucloud.cn> wrote: > > > + if (state && !(state & TCA_FLOWER_KEY_CT_FLAGS_TRACKED)) { > > > + NL_SET_ERR_MSG_ATTR(extack, tb, > > > + "ct_state no trk, no other flag are set"); > > > + return -EINVAL; > > > + } > > > + > > > + if (state & TCA_FLOWER_KEY_CT_FLAGS_NEW && > > > + state & TCA_FLOWER_KEY_CT_FLAGS_ESTABLISHED) { > > > + NL_SET_ERR_MSG_ATTR(extack, tb, > > > + "ct_state new and est are exclusive"); > > > > Please spell out the full words, "trk" and "est" are not good abbreviations. > > It does match user space naming in OvS as well as iproute2: > > { "trk", TCA_FLOWER_KEY_CT_FLAGS_TRACKED }, > { "new", TCA_FLOWER_KEY_CT_FLAGS_NEW }, > { "est", TCA_FLOWER_KEY_CT_FLAGS_ESTABLISHED }, > { "inv", TCA_FLOWER_KEY_CT_FLAGS_INVALID }, > { "rpl", TCA_FLOWER_KEY_CT_FLAGS_REPLY }, > > IDK about netfilter itself. It makes sense now, but still they are bad abbreviations, especially "est" is usually short for "estimated" not "established". More importantly, we do not have to use abbreviations in ext ack, we have enough room there. Thanks.
diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c index 84f9325..49ae67b 100644 --- a/net/sched/cls_flower.c +++ b/net/sched/cls_flower.c @@ -30,6 +30,11 @@ #include <uapi/linux/netfilter/nf_conntrack_common.h> +#define TCA_FLOWER_KEY_CT_FLAGS_MASK (TCA_FLOWER_KEY_CT_FLAGS_NEW | \ + TCA_FLOWER_KEY_CT_FLAGS_ESTABLISHED | \ + TCA_FLOWER_KEY_CT_FLAGS_RELATED | \ + TCA_FLOWER_KEY_CT_FLAGS_TRACKED) + struct fl_flow_key { struct flow_dissector_key_meta meta; struct flow_dissector_key_control control; @@ -687,7 +692,8 @@ static void *fl_get(struct tcf_proto *tp, u32 handle) [TCA_FLOWER_KEY_ENC_OPTS] = { .type = NLA_NESTED }, [TCA_FLOWER_KEY_ENC_OPTS_MASK] = { .type = NLA_NESTED }, [TCA_FLOWER_KEY_CT_STATE] = { .type = NLA_U16 }, - [TCA_FLOWER_KEY_CT_STATE_MASK] = { .type = NLA_U16 }, + [TCA_FLOWER_KEY_CT_STATE_MASK] = + NLA_POLICY_MASK(NLA_U16, TCA_FLOWER_KEY_CT_FLAGS_MASK), [TCA_FLOWER_KEY_CT_ZONE] = { .type = NLA_U16 }, [TCA_FLOWER_KEY_CT_ZONE_MASK] = { .type = NLA_U16 }, [TCA_FLOWER_KEY_CT_MARK] = { .type = NLA_U32 }, @@ -1390,12 +1396,33 @@ static int fl_set_enc_opt(struct nlattr **tb, struct fl_flow_key *key, return 0; } +static int fl_validate_ct_state(u16 state, struct nlattr *tb, + struct netlink_ext_ack *extack) +{ + if (state && !(state & TCA_FLOWER_KEY_CT_FLAGS_TRACKED)) { + NL_SET_ERR_MSG_ATTR(extack, tb, + "ct_state no trk, no other flag are set"); + return -EINVAL; + } + + if (state & TCA_FLOWER_KEY_CT_FLAGS_NEW && + state & TCA_FLOWER_KEY_CT_FLAGS_ESTABLISHED) { + NL_SET_ERR_MSG_ATTR(extack, tb, + "ct_state new and est are exclusive"); + return -EINVAL; + } + + return 0; +} + static int fl_set_key_ct(struct nlattr **tb, struct flow_dissector_key_ct *key, struct flow_dissector_key_ct *mask, struct netlink_ext_ack *extack) { if (tb[TCA_FLOWER_KEY_CT_STATE]) { + int err; + if (!IS_ENABLED(CONFIG_NF_CONNTRACK)) { NL_SET_ERR_MSG(extack, "Conntrack isn't enabled"); return -EOPNOTSUPP; @@ -1403,6 +1430,13 @@ static int fl_set_key_ct(struct nlattr **tb, fl_set_key_val(tb, &key->ct_state, TCA_FLOWER_KEY_CT_STATE, &mask->ct_state, TCA_FLOWER_KEY_CT_STATE_MASK, sizeof(key->ct_state)); + + err = fl_validate_ct_state(mask->ct_state, + tb[TCA_FLOWER_KEY_CT_STATE_MASK], + extack); + if (err) + return err; + } if (tb[TCA_FLOWER_KEY_CT_ZONE]) { if (!IS_ENABLED(CONFIG_NF_CONNTRACK_ZONES)) {