diff mbox series

[net,v4] net/sched: cls_flower: Reject invalid ct_state flags rules

Message ID 1612674803-7912-1-git-send-email-wenxu@ucloud.cn (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series [net,v4] net/sched: cls_flower: Reject invalid ct_state flags rules | expand

Checks

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 warning 6 maintainers not CCed: jiri@resnulli.us xiyou.wangcong@gmail.com paulb@mellanox.com marcelo.leitner@gmail.com yossiku@mellanox.com davem@davemloft.net
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, 66 lines checked
netdev/build_allmodconfig_warn success Errors and warnings before: 88 this patch: 88
netdev/header_inline success Link
netdev/stable success Stable not CCed

Commit Message

wenxu Feb. 7, 2021, 5:13 a.m. UTC 
From: wenxu <wenxu@ucloud.cn>

Reject the unsupported and invalid ct_state flags of cls flower rules.

Fixes: e0ace68af2ac ("net/sched: cls_flower: Add matching on conntrack info")
Signed-off-by: wenxu <wenxu@ucloud.cn>
---
 net/sched/cls_flower.c | 36 +++++++++++++++++++++++++++++++++++-
 1 file changed, 35 insertions(+), 1 deletion(-)

Comments

Cong Wang Feb. 8, 2021, 6:41 p.m. UTC | #1 
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.
Jakub Kicinski Feb. 8, 2021, 6:47 p.m. UTC | #2 
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.
Marcelo Ricardo Leitner Feb. 8, 2021, 6:57 p.m. UTC | #3 
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)

>  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 },

I wonder if this one should be protected by the flags mask as well.
It won't take action on unknown bits because of the mask below, but
still, it is accepting data that it doesn't know its meaning.

> -	[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 },
Marcelo Ricardo Leitner Feb. 8, 2021, 7:03 p.m. UTC | #4 
On Mon, Feb 08, 2021 at 10:47:59AM -0800, Jakub Kicinski 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");

This one was imported from OvS but it's not accurate.
Should be more like: no trk, so no other flag can be set
or something like that.

Seems it doesn't need to explicitly mention "ct_state" in the msg,
btw. I can't check it right now but all other uses of
NL_SET_ERR_MSG_ATTR are not doing it, at least in cls_flower.c.

> > > +               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:

I also think it makes sense as is.

> 
>         { "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.
>
Jakub Kicinski Feb. 8, 2021, 7:21 p.m. UTC | #5 
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;
Cong Wang Feb. 9, 2021, 5:36 a.m. UTC | #6 
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 mbox series

Patch

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)) {