On 7 Jan 2022, at 14:15, Ilya Maximets wrote:
> 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?
ACK, will send a v4
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
