[ovs-dev] [PATCH] dpif-netdev: Fix integer handling issue.

[Ilya Maximets]

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 at 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 at vmware.com>
>>>> Signed-off-by: William Tu <u9012063 at 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.