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)?
> 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
