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. I still have a couple of ofp patches that I didn't look close enough yet. Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
