| Message ID | 1615880657-10364-1-git-send-email-wenxu@ucloud.cn (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net/sched: cls_flower: fix only mask bit check in the validate_ct_state | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | fail | 1 blamed authors not CCed: marcelo.leitner@gmail.com; 3 maintainers not CCed: xiyou.wangcong@gmail.com jiri@resnulli.us marcelo.leitner@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: 88 this patch: 88 |
| 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, 51 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 88 this patch: 88 |
| netdev/header_inline | success | Link |
On Tue, 16 Mar 2021 15:44:17 +0800 wenxu@ucloud.cn wrote: > From: wenxu <wenxu@ucloud.cn> > > The ct_state validate should not only check the mask bit and also > check the state bit. > For the +new+est case example, The 'new' and 'est' bits should be > set in both state_mask and state flags. Or the -new-est case also > will be reject by kernel. > > Fixes: 1bcc51ac0731 ("net/sched: cls_flower: Reject invalid ct_state flags rules") > Fixes: 3aed8b63336c ("net/sched: cls_flower: validate ct_state for invalid and reply flags") > Signed-off-by: wenxu <wenxu@ucloud.cn> > --- > net/sched/cls_flower.c | 20 +++++++++++++------- > 1 file changed, 13 insertions(+), 7 deletions(-) > > diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c > index d097b5c..92659e1 100644 > --- a/net/sched/cls_flower.c > +++ b/net/sched/cls_flower.c > @@ -1401,31 +1401,37 @@ 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, > +static int fl_validate_ct_state(u16 state_mask, u16 state, > + struct nlattr *tb, > struct netlink_ext_ack *extack) > { > - if (state && !(state & TCA_FLOWER_KEY_CT_FLAGS_TRACKED)) { > + if (state_mask && !(state_mask & TCA_FLOWER_KEY_CT_FLAGS_TRACKED)) { > NL_SET_ERR_MSG_ATTR(extack, tb, > "no trk, so no other flag can be set"); > return -EINVAL; > } > > - if (state & TCA_FLOWER_KEY_CT_FLAGS_NEW && > + if (state_mask & TCA_FLOWER_KEY_CT_FLAGS_NEW && > + state & TCA_FLOWER_KEY_CT_FLAGS_NEW && bitwise and operator chains well, BTW, so this could be written as: if (state & state_mask & TCA_FLOWER_KEY_CT_FLAGS_NEW && state & state_mask & TCA_FLOWER_KEY_CT_FLAGS_ESTABLISHED) { > + state_mask & TCA_FLOWER_KEY_CT_FLAGS_ESTABLISHED && > state & TCA_FLOWER_KEY_CT_FLAGS_ESTABLISHED) { > NL_SET_ERR_MSG_ATTR(extack, tb, > "new and est are mutually exclusive"); > return -EINVAL; > } > > - if (state & TCA_FLOWER_KEY_CT_FLAGS_INVALID && > - state & ~(TCA_FLOWER_KEY_CT_FLAGS_TRACKED | > + if (state_mask & TCA_FLOWER_KEY_CT_FLAGS_INVALID && > + state & TCA_FLOWER_KEY_CT_FLAGS_INVALID && > + state_mask & ~(TCA_FLOWER_KEY_CT_FLAGS_TRACKED | > TCA_FLOWER_KEY_CT_FLAGS_INVALID)) { nit: this needs to be realigned after opening bracket has moved > NL_SET_ERR_MSG_ATTR(extack, tb, > "when inv is set, only trk may be set"); > return -EINVAL; > } > > - if (state & TCA_FLOWER_KEY_CT_FLAGS_NEW && > + if (state_mask & TCA_FLOWER_KEY_CT_FLAGS_NEW && > + state & TCA_FLOWER_KEY_CT_FLAGS_NEW && > + state_mask & TCA_FLOWER_KEY_CT_FLAGS_REPLY && > state & TCA_FLOWER_KEY_CT_FLAGS_REPLY) { > NL_SET_ERR_MSG_ATTR(extack, tb, > "new and rpl are mutually exclusive"); > @@ -1451,7 +1457,7 @@ static int fl_set_key_ct(struct nlattr **tb, > &mask->ct_state, TCA_FLOWER_KEY_CT_STATE_MASK, > sizeof(key->ct_state)); > > - err = fl_validate_ct_state(mask->ct_state, > + err = fl_validate_ct_state(mask->ct_state, key->ct_state, > tb[TCA_FLOWER_KEY_CT_STATE_MASK], > extack); > if (err)
Hi, On Tue, Mar 16, 2021 at 03:44:17PM +0800, wenxu@ucloud.cn wrote: > From: wenxu <wenxu@ucloud.cn> > > The ct_state validate should not only check the mask bit and also > check the state bit. > For the +new+est case example, The 'new' and 'est' bits should be > set in both state_mask and state flags. Or the -new-est case also > will be reject by kernel. Please mention why +trk-new-est is expected. > > Fixes: 1bcc51ac0731 ("net/sched: cls_flower: Reject invalid ct_state flags rules") > Fixes: 3aed8b63336c ("net/sched: cls_flower: validate ct_state for invalid and reply flags") checkpatch.pl doesn't complain but I'm not sure if a tab is allowed here, btw. > Signed-off-by: wenxu <wenxu@ucloud.cn> > --- > net/sched/cls_flower.c | 20 +++++++++++++------- > 1 file changed, 13 insertions(+), 7 deletions(-) > > diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c > index d097b5c..92659e1 100644 > --- a/net/sched/cls_flower.c > +++ b/net/sched/cls_flower.c > @@ -1401,31 +1401,37 @@ 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, > +static int fl_validate_ct_state(u16 state_mask, u16 state, > + struct nlattr *tb, The key/mask ordering is becoming messy in flower. As this function gets called from fl_set_key_ct, please lets keep what was used there: key, mask. Seems it's still the dominant one. static int fl_set_key_ct(struct nlattr **tb, struct flow_dissector_key_ct *key, struct flow_dissector_key_ct *mask, On a similar note, I'm wondering if it worth just doing: u16 effective = state & state_mask; To avoid this many checks below against key and mask simultaneously. > struct netlink_ext_ack *extack) > { > - if (state && !(state & TCA_FLOWER_KEY_CT_FLAGS_TRACKED)) { > + if (state_mask && !(state_mask & TCA_FLOWER_KEY_CT_FLAGS_TRACKED)) { > NL_SET_ERR_MSG_ATTR(extack, tb, > "no trk, so no other flag can be set"); > return -EINVAL; > } > > - if (state & TCA_FLOWER_KEY_CT_FLAGS_NEW && > + if (state_mask & TCA_FLOWER_KEY_CT_FLAGS_NEW && > + state & TCA_FLOWER_KEY_CT_FLAGS_NEW && > + state_mask & TCA_FLOWER_KEY_CT_FLAGS_ESTABLISHED && > state & TCA_FLOWER_KEY_CT_FLAGS_ESTABLISHED) { > NL_SET_ERR_MSG_ATTR(extack, tb, > "new and est are mutually exclusive"); > return -EINVAL; > } > > - if (state & TCA_FLOWER_KEY_CT_FLAGS_INVALID && > - state & ~(TCA_FLOWER_KEY_CT_FLAGS_TRACKED | > + if (state_mask & TCA_FLOWER_KEY_CT_FLAGS_INVALID && > + state & TCA_FLOWER_KEY_CT_FLAGS_INVALID && > + state_mask & ~(TCA_FLOWER_KEY_CT_FLAGS_TRACKED | > TCA_FLOWER_KEY_CT_FLAGS_INVALID)) { An indent adjust here is welcomed. Thanks, Marcelo > NL_SET_ERR_MSG_ATTR(extack, tb, > "when inv is set, only trk may be set"); > return -EINVAL; > } > > - if (state & TCA_FLOWER_KEY_CT_FLAGS_NEW && > + if (state_mask & TCA_FLOWER_KEY_CT_FLAGS_NEW && > + state & TCA_FLOWER_KEY_CT_FLAGS_NEW && > + state_mask & TCA_FLOWER_KEY_CT_FLAGS_REPLY && > state & TCA_FLOWER_KEY_CT_FLAGS_REPLY) { > NL_SET_ERR_MSG_ATTR(extack, tb, > "new and rpl are mutually exclusive"); > @@ -1451,7 +1457,7 @@ static int fl_set_key_ct(struct nlattr **tb, > &mask->ct_state, TCA_FLOWER_KEY_CT_STATE_MASK, > sizeof(key->ct_state)); > > - err = fl_validate_ct_state(mask->ct_state, > + err = fl_validate_ct_state(mask->ct_state, key->ct_state, > tb[TCA_FLOWER_KEY_CT_STATE_MASK], > extack); > if (err) > -- > 1.8.3.1 >
diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c index d097b5c..92659e1 100644 --- a/net/sched/cls_flower.c +++ b/net/sched/cls_flower.c @@ -1401,31 +1401,37 @@ 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, +static int fl_validate_ct_state(u16 state_mask, u16 state, + struct nlattr *tb, struct netlink_ext_ack *extack) { - if (state && !(state & TCA_FLOWER_KEY_CT_FLAGS_TRACKED)) { + if (state_mask && !(state_mask & TCA_FLOWER_KEY_CT_FLAGS_TRACKED)) { NL_SET_ERR_MSG_ATTR(extack, tb, "no trk, so no other flag can be set"); return -EINVAL; } - if (state & TCA_FLOWER_KEY_CT_FLAGS_NEW && + if (state_mask & TCA_FLOWER_KEY_CT_FLAGS_NEW && + state & TCA_FLOWER_KEY_CT_FLAGS_NEW && + state_mask & TCA_FLOWER_KEY_CT_FLAGS_ESTABLISHED && state & TCA_FLOWER_KEY_CT_FLAGS_ESTABLISHED) { NL_SET_ERR_MSG_ATTR(extack, tb, "new and est are mutually exclusive"); return -EINVAL; } - if (state & TCA_FLOWER_KEY_CT_FLAGS_INVALID && - state & ~(TCA_FLOWER_KEY_CT_FLAGS_TRACKED | + if (state_mask & TCA_FLOWER_KEY_CT_FLAGS_INVALID && + state & TCA_FLOWER_KEY_CT_FLAGS_INVALID && + state_mask & ~(TCA_FLOWER_KEY_CT_FLAGS_TRACKED | TCA_FLOWER_KEY_CT_FLAGS_INVALID)) { NL_SET_ERR_MSG_ATTR(extack, tb, "when inv is set, only trk may be set"); return -EINVAL; } - if (state & TCA_FLOWER_KEY_CT_FLAGS_NEW && + if (state_mask & TCA_FLOWER_KEY_CT_FLAGS_NEW && + state & TCA_FLOWER_KEY_CT_FLAGS_NEW && + state_mask & TCA_FLOWER_KEY_CT_FLAGS_REPLY && state & TCA_FLOWER_KEY_CT_FLAGS_REPLY) { NL_SET_ERR_MSG_ATTR(extack, tb, "new and rpl are mutually exclusive"); @@ -1451,7 +1457,7 @@ static int fl_set_key_ct(struct nlattr **tb, &mask->ct_state, TCA_FLOWER_KEY_CT_STATE_MASK, sizeof(key->ct_state)); - err = fl_validate_ct_state(mask->ct_state, + err = fl_validate_ct_state(mask->ct_state, key->ct_state, tb[TCA_FLOWER_KEY_CT_STATE_MASK], extack); if (err)