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

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

[wenxu]

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

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

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

[Cong Wang]

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.

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

[Jakub Kicinski]

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.

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

[Marcelo Ricardo Leitner]

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

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

[Marcelo Ricardo Leitner]

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.
> 

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

[Jakub Kicinski]

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;

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

[Cong Wang]

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.