| Message ID | 1584487915-19859-1-git-send-email-u9012063@gmail.com |
|---|---|
| State | Rejected |
| Headers | show |
| Series | [ovs-dev] dpif-netdev: Fix integer handling issue. | expand |
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>
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.
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
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.
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.
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.
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
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; }
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(-)