| Message ID | 1612412244-26434-1-git-send-email-wenxu@ucloud.cn (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net/sched: cls_flower: Return invalid for unknown ct_state flags rules | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | fail | Series targets non-next tree, but doesn't contain any Fixes tags |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | fail | 5 maintainers not CCed: jhs@mojatatu.com kuba@kernel.org jiri@resnulli.us davem@davemloft.net xiyou.wangcong@gmail.com |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 7176 this patch: 7176 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 27 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 7581 this patch: 7581 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
Hi, On Thu, Feb 04, 2021 at 12:17:24PM +0800, wenxu@ucloud.cn wrote: > From: wenxu <wenxu@ucloud.cn> > > Reject the unknown ct_state flags of cls flower rules. This also make > the userspace like ovs to probe the ct_state flags support in the > kernel. That's a good start but it could also do some combination sanity checks, like ovs does in validate_ct_state(). For example, it does: if (state && !(state & CS_TRACKED)) { ds_put_format(ds, "%s: invalid connection state: " "If \"trk\" is unset, no other flags are set\n", ... > --- a/net/sched/cls_flower.c > +++ b/net/sched/cls_flower.c > @@ -1403,6 +1403,10 @@ 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)); > + if (TCA_FLOWER_KEY_CT_FLAGS_UNKNOWN(mask->ct_state)) { > + NL_SET_ERR_MSG(extack, "invalid ct_state flags"); cls_flower is inconsistent on this but please use NL_SET_ERR_MSG_MOD instead. > + return -EINVAL; > + } > } > if (tb[TCA_FLOWER_KEY_CT_ZONE]) { > if (!IS_ENABLED(CONFIG_NF_CONNTRACK_ZONES)) { > -- > 1.8.3.1 >
在 2021/2/4 21:38, Marcelo Ricardo Leitner 写道: > Hi, > > On Thu, Feb 04, 2021 at 12:17:24PM +0800, wenxu@ucloud.cn wrote: >> From: wenxu <wenxu@ucloud.cn> >> >> Reject the unknown ct_state flags of cls flower rules. This also make >> the userspace like ovs to probe the ct_state flags support in the >> kernel. > That's a good start but it could also do some combination sanity > checks, like ovs does in validate_ct_state(). For example, it does: > > if (state && !(state & CS_TRACKED)) { > ds_put_format(ds, "%s: invalid connection state: " > "If \"trk\" is unset, no other flags are set\n", > So this sanity checks maybe also need to be added in the ovs kernel modules? The kernel datapath can work without ovs-vswitchd.
On Thu, Feb 04, 2021 at 11:50:53PM +0800, wenxu wrote: > > 在 2021/2/4 21:38, Marcelo Ricardo Leitner 写道: > > Hi, > > > > On Thu, Feb 04, 2021 at 12:17:24PM +0800, wenxu@ucloud.cn wrote: > >> From: wenxu <wenxu@ucloud.cn> > >> > >> Reject the unknown ct_state flags of cls flower rules. This also make > >> the userspace like ovs to probe the ct_state flags support in the > >> kernel. > > That's a good start but it could also do some combination sanity > > checks, like ovs does in validate_ct_state(). For example, it does: > > > > if (state && !(state & CS_TRACKED)) { > > ds_put_format(ds, "%s: invalid connection state: " > > "If \"trk\" is unset, no other flags are set\n", > > > So this sanity checks maybe also need to be added in the ovs kernel modules? > > The kernel datapath can work without ovs-vswitchd. IMHO that would be nice, yes. Marcelo
diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h index ee95f42..1ac8ff8 100644 --- a/include/uapi/linux/pkt_cls.h +++ b/include/uapi/linux/pkt_cls.h @@ -591,8 +591,17 @@ enum { TCA_FLOWER_KEY_CT_FLAGS_ESTABLISHED = 1 << 1, /* Part of an existing connection. */ TCA_FLOWER_KEY_CT_FLAGS_RELATED = 1 << 2, /* Related to an established connection. */ TCA_FLOWER_KEY_CT_FLAGS_TRACKED = 1 << 3, /* Conntrack has occurred. */ + + __TCA_FLOWER_KEY_CT_FLAGS_MAX, }; +#define TCA_FLOWER_KEY_CT_FLAGS_MAX \ + ((__TCA_FLOWER_KEY_CT_FLAGS_MAX - 1) << 1) +#define TCA_FLOWER_KEY_CT_FLAGS_MASK \ + (TCA_FLOWER_KEY_CT_FLAGS_MAX - 1) +#define TCA_FLOWER_KEY_CT_FLAGS_UNKNOWN(v) \ + ((v) & (~TCA_FLOWER_KEY_CT_FLAGS_MASK)) + enum { TCA_FLOWER_KEY_ENC_OPTS_UNSPEC, TCA_FLOWER_KEY_ENC_OPTS_GENEVE, /* Nested diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c index 84f9325..ebee70f 100644 --- a/net/sched/cls_flower.c +++ b/net/sched/cls_flower.c @@ -1403,6 +1403,10 @@ 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)); + if (TCA_FLOWER_KEY_CT_FLAGS_UNKNOWN(mask->ct_state)) { + NL_SET_ERR_MSG(extack, "invalid ct_state flags"); + return -EINVAL; + } } if (tb[TCA_FLOWER_KEY_CT_ZONE]) { if (!IS_ENABLED(CONFIG_NF_CONNTRACK_ZONES)) {