On 5 Jan 2022, at 10:43, Eelco Chaudron wrote:
> On 4 Jan 2022, at 17:52, Ilya Maximets wrote: > >> On 12/6/21 17:57, Eelco Chaudron wrote: >>> Currently, if a flow reply results in a message which exceeds >>> the maximum reply size, it will assert OVS. This would happen >>> when OVN uses OpenFlow15 to add large flows, and they get read >>> using OpenFlow10 with ovs-ofctl. >>> >>> This patch prevents this and adds a test case to make sure the >>> code behaves as expected. >>> >>> Signed-off-by: Eelco Chaudron <[email protected]> >> >> Hi, Eelco. Thanks for the fix! Couple of comments inline. > > See below, and will send a v2. > >> Best regards, Ilya Maximets. >> >>> --- >>> lib/ofp-flow.c | 12 +++++++++++- >>> tests/ovs-ofctl.at | 19 +++++++++++++++++++ >>> 2 files changed, 30 insertions(+), 1 deletion(-) >>> >>> diff --git a/lib/ofp-flow.c b/lib/ofp-flow.c >>> index ff0396845..796699dba 100644 >>> --- a/lib/ofp-flow.c >>> +++ b/lib/ofp-flow.c >>> @@ -1254,7 +1254,17 @@ ofputil_append_flow_stats_reply(const struct >>> ofputil_flow_stats *fs, >>> OVS_NOT_REACHED(); >>> } >>> >>> - ofpmp_postappend(replies, start_ofs); >>> + if ((reply->size - start_ofs) > >>> + (UINT16_MAX - ((char *) reply->msg - (char *) reply->header))) { >> >> Is this check equal to ofpbuf_oversized(reply) ? >> Or we can't use this function here for some reason? > > The difference was that this checks if the current addition was causing the > packet buffer to become oversized. > But looking at it again, using this function is more clear, and if the reply > was already oversized for another reason (bug), the ofpmp_postappend() below > will catch it. I was wrong, so ignore my v2, will send a v3 :( I need this specific check as I’m only interested to see if my message is above 64K, not the whole size, as we will split it up in ofpmp_postappend(). >>> + /* When this happens, the reply will not fit in a single OFP >>> message, >>> + * and we should not append it to the queue. We will log a warning >>> + * and continue with the next flow stat entry. */ >>> + reply->size = start_ofs; >>> + VLOG_WARN_RL(&rl, "Flow exceeded the maximum flow statistics reply >>> " >>> + "size and was excluded from the response set"); >>> + } else { >>> + v(replies, start_ofs); >>> + } >>> fs_->match.flow.tunnel.metadata.tab = orig_tun_table; >>> } >>> >>> diff --git a/tests/ovs-ofctl.at b/tests/ovs-ofctl.at >>> index 586e55806..9772094fc 100644 >>> --- a/tests/ovs-ofctl.at >>> +++ b/tests/ovs-ofctl.at >>> @@ -3252,3 +3252,22 @@ dnl because we need ovs-vswitchd to have the >>> controller config before starting >>> dnl the controller to 'snoop' the OpenFlow messages from beginning >>> OVS_VSWITCHD_STOP(["/connection failed (No such file or directory)/d"]) >>> AT_CLEANUP >>> + >>> + >>> +AT_SETUP([ovs-ofctl show-flows - Oversized flow]) >>> +OVS_VSWITCHD_START >>> + >>> +printf "add >>> priority=90,icmp,metadata=0x3,nw_dst=11.0.0.1,icmp_type=8,icmp_code=0,reg15=0x8005,metadata=0x1,actions=" >>> > flow.txt >>> +for i in `seq 0 1021`; do printf >>> "set_field:0x399->reg13,set_field:0x$i->reg15,resubmit(,39),"; done >> >>> flow.txt >>> +echo "resubmit(,39)" >> flow.txt >>> + >>> +AT_CHECK([ovs-ofctl -O OpenFlow15 add-flows br0 flow.txt]) >>> + >>> +AT_CHECK([ovs-ofctl -O OpenFlow10 dump-flows br0 | ofctl_strip | sed >>> '/NXST_FLOW/d' | sort], [0], []) >>> +OVS_WAIT_UNTIL([grep -q "ofp_flow|WARN|Flow exceeded the maximum flow >>> statistics reply size and was excluded from the response set" >>> ovs-vswitchd.log]) >>> + >>> +dnl cat flow.txt > expout >>> +dnl AT_CHECK([ovs-ofctl -O OpenFlow15 dump-flows br0 | ofctl_strip | sed >>> '/OFPST_FLOW/d' | sort], [0], [expout]) >> >> Why above 2 lines are commented out? >> > > Oops, and I was thinking the test passes :( Fixed this in V2, plus some other > changes in the flow definition to make it actually work! > >>> + >>> +OVS_VSWITCHD_STOP(["/|WARN|/d"]) >> >> Should be better to strip the specific warning instead of all of them. > > Done in v2. > >>> +AT_CLEANUP _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
