On 1/7/22 13:43, Eelco Chaudron wrote: > > > On 7 Jan 2022, at 13:34, Maxime Coquelin wrote: > >> Hi Eelco, >> >> On 1/5/22 10:48, 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]> >>> --- >>> v2: >>> - Fixed test to actually verify the output. >>> - Used ofpbuf_oversized() instead of manual calculation. >>> >>> lib/ofp-flow.c | 11 ++++++++++- >>> tests/ovs-ofctl.at | 19 +++++++++++++++++++ >>> 2 files changed, 29 insertions(+), 1 deletion(-) >>> >>> diff --git a/lib/ofp-flow.c b/lib/ofp-flow.c >>> index ff0396845..97b342c03 100644 >>> --- a/lib/ofp-flow.c >>> +++ b/lib/ofp-flow.c >>> @@ -1254,7 +1254,16 @@ ofputil_append_flow_stats_reply(const struct >>> ofputil_flow_stats *fs, >>> OVS_NOT_REACHED(); >>> } >>> - ofpmp_postappend(replies, start_ofs); >>> + if (ofpbuf_oversized(reply)) { >>> + /* 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 { >>> + ofpmp_postappend(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..090b5f0ad 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 " >>> priority=90,icmp,reg15=0x8005,metadata=0x1,nw_dst=11.0.0.1,icmp_type=8,icmp_code=0 >>> actions=" > flow.txt >>> +for i in `seq 1 1022`; do printf >>> "set_field:0x399->reg13,set_field:0x$i->reg15,resubmit(,39),"; done >> >>> flow.txt >>> +echo "resubmit(,39)" >> flow.txt >> >> Minor comment, but maybe using printf everywhere would be more >> consistent. > > Thanks for the review Maxime! > Ilya do you want me to send another revision with this change, or are you ok > (or fix it on commit)?
If you mean just s/echo/printf/ in the line above, this can be fixed on commit, I think. But I'd prefer Maxime to review v3 of the patch. :) OTOH, I'd prefer s/(char *) reply->msg - (char *) reply->header/ofpbuf_headersize(reply)/ in the v3. So, maybe you can change both things and post v4 to avoid confusion about different versions? > >> Other than that, it looks good to me: >> Reviewed-by: Maxime Coquelin <[email protected]> >> >> Thanks, >> Maxime > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
