diff mbox series

[ovs-dev,v3] netdev-offload-tc: Reject install rules for tc flower unsupported ct_state flags

Message ID 1612407014-25237-1-git-send-email-wenxu@ucloud.cn
State Accepted
Headers show
Series [ovs-dev,v3] netdev-offload-tc: Reject install rules for tc flower unsupported ct_state flags | expand

Commit Message

wenxu Feb. 4, 2021, 2:50 a.m. UTC 
From: wenxu <wenxu@ucloud.cn>

TC flower doesn't support some ct state flags such as
INVALID/SNAT/DNAT/REPLY. So it is better to reject this rule.

Signed-off-by: wenxu <wenxu@ucloud.cn>
---
 lib/netdev-offload-tc.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

Ilya Maximets Feb. 4, 2021, 2:34 p.m. UTC | #1 
On 2/4/21 3:50 AM, wenxu@ucloud.cn wrote:
> From: wenxu <wenxu@ucloud.cn>
> 
> TC flower doesn't support some ct state flags such as
> INVALID/SNAT/DNAT/REPLY. So it is better to reject this rule.
> 

Fixes: 576126a931cd ("netdev-offload-tc: Add conntrack support")

> Signed-off-by: wenxu <wenxu@ucloud.cn>
> ---
>  lib/netdev-offload-tc.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)

This version loogs good to me.
Marcelo, Paul, what do you think?

> 
> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
> index 586d99d..7cdd849 100644
> --- a/lib/netdev-offload-tc.c
> +++ b/lib/netdev-offload-tc.c
> @@ -1646,6 +1646,7 @@ netdev_tc_flow_put(struct netdev *netdev, struct match *match,
>                  flower.key.ct_state |= TCA_FLOWER_KEY_CT_FLAGS_NEW;
>              }
>              flower.mask.ct_state |= TCA_FLOWER_KEY_CT_FLAGS_NEW;
> +            mask->ct_state &= ~OVS_CS_F_NEW;
>          }
>  
>          if (mask->ct_state & OVS_CS_F_ESTABLISHED) {
> @@ -1653,6 +1654,7 @@ netdev_tc_flow_put(struct netdev *netdev, struct match *match,
>                  flower.key.ct_state |= TCA_FLOWER_KEY_CT_FLAGS_ESTABLISHED;
>              }
>              flower.mask.ct_state |= TCA_FLOWER_KEY_CT_FLAGS_ESTABLISHED;
> +            mask->ct_state &= ~OVS_CS_F_ESTABLISHED;
>          }
>  
>          if (mask->ct_state & OVS_CS_F_TRACKED) {
> @@ -1660,14 +1662,13 @@ netdev_tc_flow_put(struct netdev *netdev, struct match *match,
>                  flower.key.ct_state |= TCA_FLOWER_KEY_CT_FLAGS_TRACKED;
>              }
>              flower.mask.ct_state |= TCA_FLOWER_KEY_CT_FLAGS_TRACKED;
> +            mask->ct_state &= ~OVS_CS_F_TRACKED;
>          }
>  
>          if (flower.key.ct_state & TCA_FLOWER_KEY_CT_FLAGS_ESTABLISHED) {
>              flower.key.ct_state &= ~(TCA_FLOWER_KEY_CT_FLAGS_NEW);
>              flower.mask.ct_state &= ~(TCA_FLOWER_KEY_CT_FLAGS_NEW);
>          }
> -
> -        mask->ct_state = 0;
>      }
>  
>      if (mask->ct_zone) {
>
Marcelo Leitner Feb. 4, 2021, 2:42 p.m. UTC | #2 
On Thu, Feb 04, 2021 at 03:34:41PM +0100, Ilya Maximets wrote:
> On 2/4/21 3:50 AM, wenxu@ucloud.cn wrote:
> > From: wenxu <wenxu@ucloud.cn>
> > 
> > TC flower doesn't support some ct state flags such as
> > INVALID/SNAT/DNAT/REPLY. So it is better to reject this rule.
> > 
> 
> Fixes: 576126a931cd ("netdev-offload-tc: Add conntrack support")
> 
> > Signed-off-by: wenxu <wenxu@ucloud.cn>
> > ---
> >  lib/netdev-offload-tc.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> This version loogs good to me.
> Marcelo, Paul, what do you think?

+1
Reviewed-by: Marcelo Ricardo Leitner <mleitner@redhat.com>

...
> > @@ -1660,14 +1662,13 @@ netdev_tc_flow_put(struct netdev *netdev, struct match *match,
> >                  flower.key.ct_state |= TCA_FLOWER_KEY_CT_FLAGS_TRACKED;
> >              }
> >              flower.mask.ct_state |= TCA_FLOWER_KEY_CT_FLAGS_TRACKED;
> > +            mask->ct_state &= ~OVS_CS_F_TRACKED;
> >          }
> >  
> >          if (flower.key.ct_state & TCA_FLOWER_KEY_CT_FLAGS_ESTABLISHED) {
> >              flower.key.ct_state &= ~(TCA_FLOWER_KEY_CT_FLAGS_NEW);
> >              flower.mask.ct_state &= ~(TCA_FLOWER_KEY_CT_FLAGS_NEW);
> >          }

Btw, this check is probably useless as validate_ct_state() already does:

      if (state & CS_NEW && state & CS_ESTABLISHED) {
          ds_put_format(ds, "%s: invalid connection state: "
                        "\"new\" and \"est\" are mutually exclusive\n",

And it would be wrong if it was effective. The translation shouldn't
be saying which flag has preference.

> > -
> > -        mask->ct_state = 0;
> >      }
> >  
> >      if (mask->ct_zone) {
> > 
>
Ilya Maximets Feb. 4, 2021, 7:17 p.m. UTC | #3 
On 2/4/21 3:42 PM, Marcelo Ricardo Leitner wrote:
> On Thu, Feb 04, 2021 at 03:34:41PM +0100, Ilya Maximets wrote:
>> On 2/4/21 3:50 AM, wenxu@ucloud.cn wrote:
>>> From: wenxu <wenxu@ucloud.cn>
>>>
>>> TC flower doesn't support some ct state flags such as
>>> INVALID/SNAT/DNAT/REPLY. So it is better to reject this rule.
>>>
>>
>> Fixes: 576126a931cd ("netdev-offload-tc: Add conntrack support")
>>
>>> Signed-off-by: wenxu <wenxu@ucloud.cn>
>>> ---
>>>  lib/netdev-offload-tc.c | 5 +++--
>>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> This version loogs good to me.
>> Marcelo, Paul, what do you think?
> 
> +1
> Reviewed-by: Marcelo Ricardo Leitner <mleitner@redhat.com>

Thanks.
Applied to master and backported down to 2.13.

> 
> ...
>>> @@ -1660,14 +1662,13 @@ netdev_tc_flow_put(struct netdev *netdev, struct match *match,
>>>                  flower.key.ct_state |= TCA_FLOWER_KEY_CT_FLAGS_TRACKED;
>>>              }
>>>              flower.mask.ct_state |= TCA_FLOWER_KEY_CT_FLAGS_TRACKED;
>>> +            mask->ct_state &= ~OVS_CS_F_TRACKED;
>>>          }
>>>  
>>>          if (flower.key.ct_state & TCA_FLOWER_KEY_CT_FLAGS_ESTABLISHED) {
>>>              flower.key.ct_state &= ~(TCA_FLOWER_KEY_CT_FLAGS_NEW);
>>>              flower.mask.ct_state &= ~(TCA_FLOWER_KEY_CT_FLAGS_NEW);
>>>          }
> 
> Btw, this check is probably useless as validate_ct_state() already does:
> 
>       if (state & CS_NEW && state & CS_ESTABLISHED) {
>           ds_put_format(ds, "%s: invalid connection state: "
>                         "\"new\" and \"est\" are mutually exclusive\n",
> 
> And it would be wrong if it was effective. The translation shouldn't
> be saying which flag has preference.
> 

IIUC, this part of the code is from validate_ct_state() function, but
it only used during ofproto/trace.  But I agree that we should not
silently clear one of the flags.  Some part of the code should fail
instead.

>>> -
>>> -        mask->ct_state = 0;
>>>      }
>>>  
>>>      if (mask->ct_zone) {
>>>
>>
>
Paul Blakey Feb. 7, 2021, 3:47 p.m. UTC | #4 
On Thu, 4 Feb 2021, Ilya Maximets wrote:

> On 2/4/21 3:42 PM, Marcelo Ricardo Leitner wrote:
> > On Thu, Feb 04, 2021 at 03:34:41PM +0100, Ilya Maximets wrote:
> >> On 2/4/21 3:50 AM, wenxu@ucloud.cn wrote:
> >>> From: wenxu <wenxu@ucloud.cn>
> >>>
> >>> TC flower doesn't support some ct state flags such as
> >>> INVALID/SNAT/DNAT/REPLY. So it is better to reject this rule.
> >>>
> >>
> >> Fixes: 576126a931cd ("netdev-offload-tc: Add conntrack support")
> >>
> >>> Signed-off-by: wenxu <wenxu@ucloud.cn>
> >>> ---
> >>>  lib/netdev-offload-tc.c | 5 +++--
> >>>  1 file changed, 3 insertions(+), 2 deletions(-)
> >>
> >> This version loogs good to me.
> >> Marcelo, Paul, what do you think?
> > 
> > +1
> > Reviewed-by: Marcelo Ricardo Leitner <mleitner@redhat.com>
> 
> Thanks.
> Applied to master and backported down to 2.13.

Sorry for late reply, Looks good (although you already merged :))


> 
> > 
> > ...
> >>> @@ -1660,14 +1662,13 @@ netdev_tc_flow_put(struct netdev *netdev, struct match *match,
> >>>                  flower.key.ct_state |= TCA_FLOWER_KEY_CT_FLAGS_TRACKED;
> >>>              }
> >>>              flower.mask.ct_state |= TCA_FLOWER_KEY_CT_FLAGS_TRACKED;
> >>> +            mask->ct_state &= ~OVS_CS_F_TRACKED;
> >>>          }
> >>>  
> >>>          if (flower.key.ct_state & TCA_FLOWER_KEY_CT_FLAGS_ESTABLISHED) {
> >>>              flower.key.ct_state &= ~(TCA_FLOWER_KEY_CT_FLAGS_NEW);
> >>>              flower.mask.ct_state &= ~(TCA_FLOWER_KEY_CT_FLAGS_NEW);
> >>>          }
> > 
> > Btw, this check is probably useless as validate_ct_state() already does:
> > 
> >       if (state & CS_NEW && state & CS_ESTABLISHED) {
> >           ds_put_format(ds, "%s: invalid connection state: "
> >                         "\"new\" and \"est\" are mutually exclusive\n",
> > 
> > And it would be wrong if it was effective. The translation shouldn't
> > be saying which flag has preference.
> > 
> 
> IIUC, this part of the code is from validate_ct_state() function, but
> it only used during ofproto/trace.  But I agree that we should not
> silently clear one of the flags.  Some part of the code should fail
> instead.
> 
> >>> -
> >>> -        mask->ct_state = 0;
> >>>      }
> >>>  
> >>>      if (mask->ct_zone) {
> >>>
> >>
> > 
> 
>
diff mbox series

Patch

diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
index 586d99d..7cdd849 100644
--- a/lib/netdev-offload-tc.c
+++ b/lib/netdev-offload-tc.c
@@ -1646,6 +1646,7 @@  netdev_tc_flow_put(struct netdev *netdev, struct match *match,
                 flower.key.ct_state |= TCA_FLOWER_KEY_CT_FLAGS_NEW;
             }
             flower.mask.ct_state |= TCA_FLOWER_KEY_CT_FLAGS_NEW;
+            mask->ct_state &= ~OVS_CS_F_NEW;
         }
 
         if (mask->ct_state & OVS_CS_F_ESTABLISHED) {
@@ -1653,6 +1654,7 @@  netdev_tc_flow_put(struct netdev *netdev, struct match *match,
                 flower.key.ct_state |= TCA_FLOWER_KEY_CT_FLAGS_ESTABLISHED;
             }
             flower.mask.ct_state |= TCA_FLOWER_KEY_CT_FLAGS_ESTABLISHED;
+            mask->ct_state &= ~OVS_CS_F_ESTABLISHED;
         }
 
         if (mask->ct_state & OVS_CS_F_TRACKED) {
@@ -1660,14 +1662,13 @@  netdev_tc_flow_put(struct netdev *netdev, struct match *match,
                 flower.key.ct_state |= TCA_FLOWER_KEY_CT_FLAGS_TRACKED;
             }
             flower.mask.ct_state |= TCA_FLOWER_KEY_CT_FLAGS_TRACKED;
+            mask->ct_state &= ~OVS_CS_F_TRACKED;
         }
 
         if (flower.key.ct_state & TCA_FLOWER_KEY_CT_FLAGS_ESTABLISHED) {
             flower.key.ct_state &= ~(TCA_FLOWER_KEY_CT_FLAGS_NEW);
             flower.mask.ct_state &= ~(TCA_FLOWER_KEY_CT_FLAGS_NEW);
         }
-
-        mask->ct_state = 0;
     }
 
     if (mask->ct_zone) {