On Fri, May 14, 2021 at 3:19 PM Numan Siddique <[email protected]> wrote: > > On Fri, May 7, 2021 at 9:03 AM Mark Michelson <[email protected]> wrote: > > > > On 5/7/21 8:03 AM, Dumitru Ceara wrote: > > > On 5/6/21 10:49 PM, Mark Michelson wrote: > > >> ACLs and logical router policies leave the user open to crafting > > >> creative matches, such as this one: > > >> > > >> (ip4.dst == 172.27.0.65/32) && ip4.src == $as && ip4.dst != 10.128.0.0/11 > > >> > > >> Ideally, that final part of the match would just be omitted, but in some > > >> CMSes this kind of thing gets generated. If we zero in on the ip4.dst > > >> part of the match, then initially, this gets parsed into the expression: > > >> > > >> ip4.dst == 0xac1b0041 && ip4.dst[21..31] != 0x54 > > >> > > >> After annotation, simplification, and evaluating conditions, this > > >> becomes: > > >> > > >> ip4.dst == 0xac1b0041 && (ip4.dst[21] || ip4.dst[22] || !ip4.dst[23] || > > >> ip4.dst[24] || !ip4.dst[25] || ip4.dst[26] || !ip4.dst[27] || > > >> ip4.dst[28] || ip4.dst[29] || ip4.dst[30] || ip4.dst[31]) > > >> > > >> At this point, we call expr_normalize(), which then calls expr_sort(). > > >> expr_sort() attempts to turn expressions of the type > > >> > > >> a && (b || c || d) > > >> > > >> into > > >> > > >> ab && ac && ad > > > > > > Hmm, I guess you meant "ab || ac || ad" here, right? > > > > Whoops, yes that's correct. I'll amend the commit log when I merge if > > this gets approved. If a v3 is required for some other reason, then I'll > > amend the commit log in that version. > > > > > > > >> > > >> In this particular case, it turns the expression into > > >> > > >> (ip4.dst == 0xac1b0041 || ip4.dst == 0xac1b0041 || ip4.dst == 0xac1b0041 > > >> || ip4.dst == 0xac1b0041 || ip4.dst == 0xac1b0041) > > >> > > >> In other words, the same expression repeated 5 times. > > >> > > >> Because the address set in the original match expands to multiple > > >> addresses, and it is ANDed with the above disjunction, a conjunctive > > >> match is created. This results in the following OF flow being created: > > >> > > >> nw_dst=172.27.0.65,action=conjunction(2,1/2),conjunction(2,1/2), > > >> conjunction(2,1/2),conjunction(2,1/2), > > >> conjunction(2,1/2) > > >> > > >> If multiple ACLs or logical router policies perform similar matches, > > >> this can cause the number of conjunction actions on the flow to balloon, > > >> possibly even reaching the point where the flow size is larger than 64K. > > >> > > >> This patch seeks to fix the issue by crushing the resulting OR that is > > >> created from expr_sort(). In the example match, this changes the ip4.dst > > >> match to just: > > >> > > >> ip4.dst == 0xac1b0041 > > >> > > >> Because it is now a single comparison, there's no conjunctive match > > >> needed, and the generated OF is as expected. > > > > > > Thanks for the very detailed commit log, it really makes the problem > > > you're trying to solve very clear! > > > > > >> > > >> Reported at: https://bugzilla.redhat.com/show_bug.cgi?id=1955161 > > >> Signed-off-by: Mark Michelson <[email protected]> > > >> --- > > > > > > I think the change is OK, but it might be good for other people to > > > review this too before pushing this patch. With that in mind: > > > > > > Acked-by: Dumitru Ceara <[email protected]> > > Can you please add a test case for this ? > > Either you can enhance this test case here - > https://github.com/ovn-org/ovn/blob/master/tests/ovn.at#L716 > or add a new one. > > With a test case added: > Acked-by: Numan Siddique <[email protected]>
Oops. My bad. There's already a test case. Please ignore my comment. Thanks Numan > > Thanks > Numan > > > > > > > Regards, > > > Dumitru > > > > > > > _______________________________________________ > > dev mailing list > > [email protected] > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
