On 2/15/22 18:06, Dumitru Ceara wrote:
> On 2/15/22 14:22, Ilya Maximets wrote:
>> On 1/24/22 21:10, Dumitru Ceara wrote:
>>> On 1/24/22 19:40, Adrian Moreno wrote:
>>>>
>>>>
>>>> On 1/24/22 18:49, Jeffrey Walton wrote:
>>>>> On Mon, Jan 24, 2022 at 9:17 AM Dumitru Ceara <dce...@redhat.com> wrote:
>>>>>>
>>>>>> As privately reported by Aaron Conole, and by Jeffrey Walton [0]
>>>>>> there's currently a number of undefined behavior instances in
>>>>>> the OVS code base. Running the OVS (and OVN) tests with UBSan [1]
>>>>>> enabled uncovers these.
>>>>>> ...
>>>>>
>>>>>> Note: depending on the order of reviews, if Adrian's "Fix undefined
>>>>>> behavior in loop macros" series [2] (or a follow up) is accepted first,
>>>>>> then patch 12/14 ("util: Avoid false positive UB when iterating
>>>>>> collections.") can be skipped. Adrian's series seems to be the more
>>>>>> correct way of fixing the issue.
>>>>>
>>>>> One small nit... UBsan (and Asan) do not produce false positives. They
>>>>> operate on real data, and when they produce a finding it is valid.
>>>>> That's also why a complete set of self tests are important. The
>>>>> complete set of tests are important because UBsan and Asan need real
>>>>> data.
>>>>>
>>>>
>>>> I agree, it's not a false positive. Furthermore, the patch that Dumitru
>>>> is referring to ("util: Avoid false positive UB when iterating
>>>> collections") reduces UBsan's sensitivity by changing some pointer
>>>> arithmetics to integer arithmetics. This silences the UBsan but
>>>> according to the discussion with the compiler folks [1], this can still
>>>> yield UB.
>>>>
>>>> Therefore, I think it would be safer to keep the pointer arithmetics
>>>> (and UBsan's high-sensitivity), fix the actual callers (i.e: the
>>>> iterator macros), and run UBsan in the CI to spot all future errors
>>>> (which Dumitru's series does).
>>>>
>>>> [1]https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103964
>>>>
>>>
>>> Yes, I probably should've rephrased the commit title of patch 12/14.
>>> It's not a false positive. I just kept it for now until Adrian's series
>>> [2] is merged. Otherwise jobs would've failed in CI, and it's
>>> technically not worse than before.
>>>
>>> But I completely agree, once Adrian's changes get accepted, the safest
>>> way is to keep the pointer arithmetic and rely on UBSan to complain if
>>> undefined behavior is detected.
>>>
>>> [2]
>>> https://patchwork.ozlabs.org/project/openvswitch/list/?series=282481&state=*
>>>
>>
>> For now I applied 7 out of 14 patches from this series and replied with
>> a comments to a few of the remaining patches.
>
> Thanks a lot for this!
>
>> I still have a couple of ofp patches that I didn't look close enough yet.
>>
>
> I'll send out a v4 addressing the current review comments and we can
> continue the review of the ofp part there. Does that sound OK to you?
Yep. Sure.
>
>> Best regards, Ilya Maximets.
>>
>
> Regards,
> Dumitru
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
