On 20 Dec 2021, at 16:33, Adrian Moreno 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]> >> --- >> 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))) { >> + /* 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..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]) >> + >> +OVS_VSWITCHD_STOP(["/|WARN|/d"]) >> +AT_CLEANUP >> >> _______________________________________________ >> dev mailing list >> [email protected] >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> > > Reviewed and manually tested this patch. Verified excessively big flows are > skipped while the rest are sent properly. Although it's not ideal to skip a > flow, I don't see any better way to respond to this corner-case. Thanks for the review, and yes, I agree, but it’s better than a crash. > Acked-by: Adrian Moreno <[email protected]> > > > -- > Adrián Moreno _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
