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.
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?
> + /* 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])
Why above 2 lines are commented out?
> +
> +OVS_VSWITCHD_STOP(["/|WARN|/d"])
Should be better to strip the specific warning instead of all of them.
> +AT_CLEANUP
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
