[ovs-dev] dpif-netdev: Fix integer handling issue.
diff mbox series

Message ID 1584487915-19859-1-git-send-email-u9012063@gmail.com
State New
Headers show
Series
  • [ovs-dev] dpif-netdev: Fix integer handling issue.
Related show

Commit Message

William Tu March 17, 2020, 11:31 p.m. UTC
Coverity CID 279497 reports "Operands don't affect result".
Because flow->ct_state is uint8_t and DP_NETDEV_CS_UNSUPPORTED_MASK
is '0xffffff00'. So remove the statement.

Cc: Usman Ansari <uansari@vmware.com>
Signed-off-by: William Tu <u9012063@gmail.com>
---
 lib/dpif-netdev.c | 4 ----
 1 file changed, 4 deletions(-)

Comments

Gregory Rose March 18, 2020, 2:45 p.m. UTC | #1
On 3/17/2020 4:31 PM, William Tu wrote:
> Coverity CID 279497 reports "Operands don't affect result".
> Because flow->ct_state is uint8_t and DP_NETDEV_CS_UNSUPPORTED_MASK
> is '0xffffff00'. So remove the statement.
>
> Cc: Usman Ansari <uansari@vmware.com>
> Signed-off-by: William Tu <u9012063@gmail.com>
> ---
>   lib/dpif-netdev.c | 4 ----
>   1 file changed, 4 deletions(-)
>
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index a798db45d9cb..0e2678d002d5 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -3224,10 +3224,6 @@ dpif_netdev_flow_from_nlattrs(const struct nlattr *key, uint32_t key_len,
>           return EINVAL;
>       }
>   
> -    if (flow->ct_state & DP_NETDEV_CS_UNSUPPORTED_MASK) {
> -        return EINVAL;
> -    }
> -
>       return 0;
>   }
>   

Reviewed-by: Greg Rose <gvrose8192@gmail.com>
Ilya Maximets March 18, 2020, 2:59 p.m. UTC | #2
On 3/18/20 12:31 AM, William Tu wrote:
> Coverity CID 279497 reports "Operands don't affect result".
> Because flow->ct_state is uint8_t and DP_NETDEV_CS_UNSUPPORTED_MASK
> is '0xffffff00'. So remove the statement.
> 
> Cc: Usman Ansari <uansari@vmware.com>
> Signed-off-by: William Tu <u9012063@gmail.com>
> ---
>  lib/dpif-netdev.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index a798db45d9cb..0e2678d002d5 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -3224,10 +3224,6 @@ dpif_netdev_flow_from_nlattrs(const struct nlattr *key, uint32_t key_len,
>          return EINVAL;
>      }
>  
> -    if (flow->ct_state & DP_NETDEV_CS_UNSUPPORTED_MASK) {
> -        return EINVAL;
> -    }
> -

I'm not sure if we need to remove this.  This code doesn't make any harm
and most likely compiled out.  I agree that it doesn't change any logic
in this function, but in case someone will try to add new flags or change
the type of ct_state we will be safe and will reject all the unknown flags.
Without this code we'll have to catch this case somehow on code review and
re-introduce this check or implement missing functionality.

One more thing is that DP_NETDEV_CS_UNSUPPORTED_MASK definition becomes
unused and should be removed along with _SUPPORTED_MASK.

So, I'd rather not touch this and just mark this code as OK for coverity
scanner.  But if you want to remove, please, clean up other parts and
add a build assert for the ct_state size and flags, so any disruptive change
will be caught by the developer of this change.

Best regards, Ilya Maximets.
William Tu March 18, 2020, 3:34 p.m. UTC | #3
On Wed, Mar 18, 2020 at 7:59 AM Ilya Maximets <i.maximets@ovn.org> wrote:
>
> On 3/18/20 12:31 AM, William Tu wrote:
> > Coverity CID 279497 reports "Operands don't affect result".
> > Because flow->ct_state is uint8_t and DP_NETDEV_CS_UNSUPPORTED_MASK
> > is '0xffffff00'. So remove the statement.
> >
> > Cc: Usman Ansari <uansari@vmware.com>
> > Signed-off-by: William Tu <u9012063@gmail.com>
> > ---
> >  lib/dpif-netdev.c | 4 ----
> >  1 file changed, 4 deletions(-)
> >
> > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> > index a798db45d9cb..0e2678d002d5 100644
> > --- a/lib/dpif-netdev.c
> > +++ b/lib/dpif-netdev.c
> > @@ -3224,10 +3224,6 @@ dpif_netdev_flow_from_nlattrs(const struct nlattr *key, uint32_t key_len,
> >          return EINVAL;
> >      }
> >
> > -    if (flow->ct_state & DP_NETDEV_CS_UNSUPPORTED_MASK) {
> > -        return EINVAL;
> > -    }
> > -
>
> I'm not sure if we need to remove this.  This code doesn't make any harm
> and most likely compiled out.  I agree that it doesn't change any logic
> in this function, but in case someone will try to add new flags or change
> the type of ct_state we will be safe and will reject all the unknown flags.
> Without this code we'll have to catch this case somehow on code review and
> re-introduce this check or implement missing functionality.
>
> One more thing is that DP_NETDEV_CS_UNSUPPORTED_MASK definition becomes
> unused and should be removed along with _SUPPORTED_MASK.

Good point.

>
> So, I'd rather not touch this and just mark this code as OK for coverity
> scanner.  But if you want to remove, please, clean up other parts and
> add a build assert for the ct_state size and flags, so any disruptive change
> will be caught by the developer of this change.
>
OK thanks!
Let's keep this code block as it is now.
William
Ben Pfaff March 18, 2020, 3:36 p.m. UTC | #4
On Wed, Mar 18, 2020 at 08:34:16AM -0700, William Tu wrote:
> On Wed, Mar 18, 2020 at 7:59 AM Ilya Maximets <i.maximets@ovn.org> wrote:
> >
> > On 3/18/20 12:31 AM, William Tu wrote:
> > > Coverity CID 279497 reports "Operands don't affect result".
> > > Because flow->ct_state is uint8_t and DP_NETDEV_CS_UNSUPPORTED_MASK
> > > is '0xffffff00'. So remove the statement.
> > >
> > > Cc: Usman Ansari <uansari@vmware.com>
> > > Signed-off-by: William Tu <u9012063@gmail.com>
> > > ---
> > >  lib/dpif-netdev.c | 4 ----
> > >  1 file changed, 4 deletions(-)
> > >
> > > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> > > index a798db45d9cb..0e2678d002d5 100644
> > > --- a/lib/dpif-netdev.c
> > > +++ b/lib/dpif-netdev.c
> > > @@ -3224,10 +3224,6 @@ dpif_netdev_flow_from_nlattrs(const struct nlattr *key, uint32_t key_len,
> > >          return EINVAL;
> > >      }
> > >
> > > -    if (flow->ct_state & DP_NETDEV_CS_UNSUPPORTED_MASK) {
> > > -        return EINVAL;
> > > -    }
> > > -
> >
> > I'm not sure if we need to remove this.  This code doesn't make any harm
> > and most likely compiled out.  I agree that it doesn't change any logic
> > in this function, but in case someone will try to add new flags or change
> > the type of ct_state we will be safe and will reject all the unknown flags.
> > Without this code we'll have to catch this case somehow on code review and
> > re-introduce this check or implement missing functionality.
> >
> > One more thing is that DP_NETDEV_CS_UNSUPPORTED_MASK definition becomes
> > unused and should be removed along with _SUPPORTED_MASK.
> 
> Good point.
> 
> >
> > So, I'd rather not touch this and just mark this code as OK for coverity
> > scanner.  But if you want to remove, please, clean up other parts and
> > add a build assert for the ct_state size and flags, so any disruptive change
> > will be caught by the developer of this change.
> >
> OK thanks!
> Let's keep this code block as it is now.

I was surprised to hear that it doesn't have any effect.  Adding a
comment might be helpful.
Ilya Maximets March 18, 2020, 3:53 p.m. UTC | #5
On 3/18/20 4:36 PM, Ben Pfaff wrote:
> On Wed, Mar 18, 2020 at 08:34:16AM -0700, William Tu wrote:
>> On Wed, Mar 18, 2020 at 7:59 AM Ilya Maximets <i.maximets@ovn.org> wrote:
>>>
>>> On 3/18/20 12:31 AM, William Tu wrote:
>>>> Coverity CID 279497 reports "Operands don't affect result".
>>>> Because flow->ct_state is uint8_t and DP_NETDEV_CS_UNSUPPORTED_MASK
>>>> is '0xffffff00'. So remove the statement.
>>>>
>>>> Cc: Usman Ansari <uansari@vmware.com>
>>>> Signed-off-by: William Tu <u9012063@gmail.com>
>>>> ---
>>>>  lib/dpif-netdev.c | 4 ----
>>>>  1 file changed, 4 deletions(-)
>>>>
>>>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
>>>> index a798db45d9cb..0e2678d002d5 100644
>>>> --- a/lib/dpif-netdev.c
>>>> +++ b/lib/dpif-netdev.c
>>>> @@ -3224,10 +3224,6 @@ dpif_netdev_flow_from_nlattrs(const struct nlattr *key, uint32_t key_len,
>>>>          return EINVAL;
>>>>      }
>>>>
>>>> -    if (flow->ct_state & DP_NETDEV_CS_UNSUPPORTED_MASK) {
>>>> -        return EINVAL;
>>>> -    }
>>>> -
>>>
>>> I'm not sure if we need to remove this.  This code doesn't make any harm
>>> and most likely compiled out.  I agree that it doesn't change any logic
>>> in this function, but in case someone will try to add new flags or change
>>> the type of ct_state we will be safe and will reject all the unknown flags.
>>> Without this code we'll have to catch this case somehow on code review and
>>> re-introduce this check or implement missing functionality.
>>>
>>> One more thing is that DP_NETDEV_CS_UNSUPPORTED_MASK definition becomes
>>> unused and should be removed along with _SUPPORTED_MASK.
>>
>> Good point.
>>
>>>
>>> So, I'd rather not touch this and just mark this code as OK for coverity
>>> scanner.  But if you want to remove, please, clean up other parts and
>>> add a build assert for the ct_state size and flags, so any disruptive change
>>> will be caught by the developer of this change.
>>>
>> OK thanks!
>> Let's keep this code block as it is now.
> 
> I was surprised to hear that it doesn't have any effect.  Adding a
> comment might be helpful.
> 

DP_NETDEV_CS_UNSUPPORTED_MASK was introduced at the time when dpif-netdev didn't
support NAT.  After the NAT support implementation in commit
4cddb1f0d837 ("dpdk: Parse NAT netlink for userspace datapath.") this mask is just
a zero in the lowest 8 bits, i.e. all current flags are supported.

I'm not sure why it's casted to uint32_t, though, or why flow->ct_state is 8 bits
only.  packets.h has similar mask and it's casted to uint32_t too.  The main concern
here is that it seems like ct_state is 32bit in netlink.  That produces misunderstanding
and makes me nervous about potential issues.

flow->ct_state is 8 bits long and mask is zero there, so this 'if' statement is
always false.
Ben Pfaff March 18, 2020, 3:58 p.m. UTC | #6
On Wed, Mar 18, 2020 at 04:53:27PM +0100, Ilya Maximets wrote:
> On 3/18/20 4:36 PM, Ben Pfaff wrote:
> > On Wed, Mar 18, 2020 at 08:34:16AM -0700, William Tu wrote:
> >> On Wed, Mar 18, 2020 at 7:59 AM Ilya Maximets <i.maximets@ovn.org> wrote:
> >>>
> >>> On 3/18/20 12:31 AM, William Tu wrote:
> >>>> Coverity CID 279497 reports "Operands don't affect result".
> >>>> Because flow->ct_state is uint8_t and DP_NETDEV_CS_UNSUPPORTED_MASK
> >>>> is '0xffffff00'. So remove the statement.
> >>>>
> >>>> Cc: Usman Ansari <uansari@vmware.com>
> >>>> Signed-off-by: William Tu <u9012063@gmail.com>
> >>>> ---
> >>>>  lib/dpif-netdev.c | 4 ----
> >>>>  1 file changed, 4 deletions(-)
> >>>>
> >>>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> >>>> index a798db45d9cb..0e2678d002d5 100644
> >>>> --- a/lib/dpif-netdev.c
> >>>> +++ b/lib/dpif-netdev.c
> >>>> @@ -3224,10 +3224,6 @@ dpif_netdev_flow_from_nlattrs(const struct nlattr *key, uint32_t key_len,
> >>>>          return EINVAL;
> >>>>      }
> >>>>
> >>>> -    if (flow->ct_state & DP_NETDEV_CS_UNSUPPORTED_MASK) {
> >>>> -        return EINVAL;
> >>>> -    }
> >>>> -
> >>>
> >>> I'm not sure if we need to remove this.  This code doesn't make any harm
> >>> and most likely compiled out.  I agree that it doesn't change any logic
> >>> in this function, but in case someone will try to add new flags or change
> >>> the type of ct_state we will be safe and will reject all the unknown flags.
> >>> Without this code we'll have to catch this case somehow on code review and
> >>> re-introduce this check or implement missing functionality.
> >>>
> >>> One more thing is that DP_NETDEV_CS_UNSUPPORTED_MASK definition becomes
> >>> unused and should be removed along with _SUPPORTED_MASK.
> >>
> >> Good point.
> >>
> >>>
> >>> So, I'd rather not touch this and just mark this code as OK for coverity
> >>> scanner.  But if you want to remove, please, clean up other parts and
> >>> add a build assert for the ct_state size and flags, so any disruptive change
> >>> will be caught by the developer of this change.
> >>>
> >> OK thanks!
> >> Let's keep this code block as it is now.
> > 
> > I was surprised to hear that it doesn't have any effect.  Adding a
> > comment might be helpful.
> > 
> 
> DP_NETDEV_CS_UNSUPPORTED_MASK was introduced at the time when dpif-netdev didn't
> support NAT.  After the NAT support implementation in commit
> 4cddb1f0d837 ("dpdk: Parse NAT netlink for userspace datapath.") this mask is just
> a zero in the lowest 8 bits, i.e. all current flags are supported.
> 
> I'm not sure why it's casted to uint32_t, though, or why flow->ct_state is 8 bits
> only.  packets.h has similar mask and it's casted to uint32_t too.  The main concern
> here is that it seems like ct_state is 32bit in netlink.  That produces misunderstanding
> and makes me nervous about potential issues.
> 
> flow->ct_state is 8 bits long and mask is zero there, so this 'if' statement is
> always false.

Oh, I understand the reason, but from glancing at the code it's not
obvious.
William Tu March 18, 2020, 7:58 p.m. UTC | #7
On Wed, Mar 18, 2020 at 8:36 AM Ben Pfaff <blp@ovn.org> wrote:
>
> On Wed, Mar 18, 2020 at 08:34:16AM -0700, William Tu wrote:
> > On Wed, Mar 18, 2020 at 7:59 AM Ilya Maximets <i.maximets@ovn.org> wrote:
> > >
> > > On 3/18/20 12:31 AM, William Tu wrote:
> > > > Coverity CID 279497 reports "Operands don't affect result".
> > > > Because flow->ct_state is uint8_t and DP_NETDEV_CS_UNSUPPORTED_MASK
> > > > is '0xffffff00'. So remove the statement.
> > > >
> > > > Cc: Usman Ansari <uansari@vmware.com>
> > > > Signed-off-by: William Tu <u9012063@gmail.com>
> > > > ---
> > > >  lib/dpif-netdev.c | 4 ----
> > > >  1 file changed, 4 deletions(-)
> > > >
> > > > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> > > > index a798db45d9cb..0e2678d002d5 100644
> > > > --- a/lib/dpif-netdev.c
> > > > +++ b/lib/dpif-netdev.c
> > > > @@ -3224,10 +3224,6 @@ dpif_netdev_flow_from_nlattrs(const struct nlattr *key, uint32_t key_len,
> > > >          return EINVAL;
> > > >      }
> > > >
> > > > -    if (flow->ct_state & DP_NETDEV_CS_UNSUPPORTED_MASK) {
> > > > -        return EINVAL;
> > > > -    }
> > > > -
> > >
> > > I'm not sure if we need to remove this.  This code doesn't make any harm
> > > and most likely compiled out.  I agree that it doesn't change any logic
> > > in this function, but in case someone will try to add new flags or change
> > > the type of ct_state we will be safe and will reject all the unknown flags.
> > > Without this code we'll have to catch this case somehow on code review and
> > > re-introduce this check or implement missing functionality.
> > >
> > > One more thing is that DP_NETDEV_CS_UNSUPPORTED_MASK definition becomes
> > > unused and should be removed along with _SUPPORTED_MASK.
> >
> > Good point.
> >
> > >
> > > So, I'd rather not touch this and just mark this code as OK for coverity
> > > scanner.  But if you want to remove, please, clean up other parts and
> > > add a build assert for the ct_state size and flags, so any disruptive change
> > > will be caught by the developer of this change.
> > >
> > OK thanks!
> > Let's keep this code block as it is now.
>
> I was surprised to hear that it doesn't have any effect.  Adding a
> comment might be helpful.

OK, then let me just add a comment.
In case in the future people run Coverity and see this again.
William

Patch
diff mbox series

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index a798db45d9cb..0e2678d002d5 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -3224,10 +3224,6 @@  dpif_netdev_flow_from_nlattrs(const struct nlattr *key, uint32_t key_len,
         return EINVAL;
     }
 
-    if (flow->ct_state & DP_NETDEV_CS_UNSUPPORTED_MASK) {
-        return EINVAL;
-    }
-
     return 0;
 }