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.
>> + /* 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
