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 <[email protected]> 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? > Best regards, Ilya Maximets. > Regards, Dumitru _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
