Ilya Maximets <i.maxim...@ovn.org> 于2023年8月12日周六 02:18写道：
> On 8/11/23 19:37, Ilya Maximets wrote:
> > On 8/9/23 05:02, Peng He wrote:
> >> OVS allows overlapping megaflows, as long as the actions of these
> >> megaflows are equal. However, the current implementation of action
> >> modification relies on flow_lookup instead of ufid, this could result
> >> in looking up a wrong megaflow and make the ukeys and megaflows
> inconsistent
> >>
> >> Just like the test case in the patch, at first we have a rule with the
> >> prefix:
> >>
> >> 10.1.2.0/24
> >>
> >> and we will get a megaflow with prefixes 10.1.2.2/24 when a packet
> with IP
> >> 10.1.2.2 is received.
> >>
> >> Then suppose we change the rule into 10.1.0.0/16. OVS prefers to keep
> the
> >> 10.1.2.2/24 megaflow and just changes its action instead of extending
> >> the prefix into 10.1.2.2/16.
> >>
> >> then suppose we have a 10.1.0.2 packet, since it misses the megaflow,
> >> this time, we will have an overlapping megaflow with the right prefix:
> >> 10.1.0.2/16
> >>
> >> now we have two megaflows:
> >> 10.1.2.2/24
> >> 10.1.0.2/16
> >>
> >> last, suppose we have changed the ruleset again. The revalidator this
> >> time still decides to change the actions of both megaflows instead of
> >> deleting them.
> >>
> >> The dpif_netdev_flow_put will search the megaflow to modify with
> unmasked
> >> keys, however it might lookup the wrong megaflow as the key 10.1.2.2
> matches
> >> both 10.1.2.2/24 and 10.1.0.2/16!
> >>
> >> This patch changes the megaflow lookup code in modification path into
> >> relying the ufid to find the correct megaflow instead of key lookup.
> >>
> >> Signed-off-by: Peng He <hepeng.0...@bytedance.com>
> >> ---
> >> lib/dpif-netdev.c | 45 ++++++++++++++++++++++++++++++---------------
> >> tests/pmd.at | 47 +++++++++++++++++++++++++++++++++++++++++++++++
> >> 2 files changed, 77 insertions(+), 15 deletions(-)
> >>
> >> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> >> index 70b953ae6..8479630b8 100644
> >> --- a/lib/dpif-netdev.c
> >> +++ b/lib/dpif-netdev.c
> >> @@ -4191,7 +4191,7 @@ flow_put_on_pmd(struct dp_netdev_pmd_thread *pmd,
> >> const struct dpif_flow_put *put,
> >> struct dpif_flow_stats *stats)
> >> {
> >> - struct dp_netdev_flow *netdev_flow;
> >> + struct dp_netdev_flow *netdev_flow = NULL;
> >> int error = 0;
> >>
> >> if (stats) {
> >> @@ -4199,16 +4199,35 @@ flow_put_on_pmd(struct dp_netdev_pmd_thread
> *pmd,
> >> }
> >>
> >> ovs_mutex_lock(&pmd->flow_mutex);
> >> - netdev_flow = dp_netdev_pmd_lookup_flow(pmd, key, NULL);
> >> - if (!netdev_flow) {
> >> - if (put->flags & DPIF_FP_CREATE) {
> >> - dp_netdev_flow_add(pmd, match, ufid, put->actions,
> >> - put->actions_len, ODPP_NONE);
> >> + if (put->ufid) {
> >> + netdev_flow = dp_netdev_pmd_find_flow(pmd, put->ufid,
> >> + put->key, put->key_len);
> >> + } else {
> >> + /* Use key instead of the locally generated ufid
> >> + * to search netdev_flow. */
> >> + netdev_flow = dp_netdev_pmd_lookup_flow(pmd, key, NULL);
> >> + }
> >> +
> >> + if (put->flags & DPIF_FP_CREATE) {
> >> + if (!netdev_flow) {
> >> + dp_netdev_flow_add(pmd, match, ufid,
> >> + put->actions, put->actions_len,
> ODPP_NONE);
> >> } else {
> >> - error = ENOENT;
> >> + error = EEXIST;
> >> }
> >> - } else {
> >> - if (put->flags & DPIF_FP_MODIFY) {
> >> + goto exit;
> >> + }
> >> +
> >> + if (put->flags & DPIF_FP_MODIFY) {
> >> + if (!netdev_flow) {
> >> + error = ENOENT;
> >> + } else {
> >> + if (!put->ufid && !flow_equal(&match->flow,
> &netdev_flow->flow)) {
> >> + /* Overlapping flow. */
> >> + error = EINVAL;
> >> + goto exit;
> >> + }
> >> +
> >> struct dp_netdev_actions *new_actions;
> >> struct dp_netdev_actions *old_actions;
> >>
> >> @@ -4239,15 +4258,11 @@ flow_put_on_pmd(struct dp_netdev_pmd_thread
> *pmd,
> >> * counter, and subtracting it before outputting the
> stats */
> >> error = EOPNOTSUPP;
> >> }
> >> -
> >> ovsrcu_postpone(dp_netdev_actions_free, old_actions);
> >> - } else if (put->flags & DPIF_FP_CREATE) {
> >> - error = EEXIST;
> >> - } else {
> >> - /* Overlapping flow. */
> >> - error = EINVAL;
> >> }
> >> }
> >> +
> >> +exit:
> >> ovs_mutex_unlock(&pmd->flow_mutex);
> >> return error;
> >> }
> >> diff --git a/tests/pmd.at b/tests/pmd.at
> >> index 48f3d432d..b03cd831e 100644
> >> --- a/tests/pmd.at
> >> +++ b/tests/pmd.at
> >> @@ -1300,3 +1300,50 @@ OVS_WAIT_UNTIL([tail -n +$LINENUM
> ovs-vswitchd.log | grep "PMD load based sleeps
> >>
> >> OVS_VSWITCHD_STOP
> >> AT_CLEANUP
> >> +
> >> +AT_SETUP([PMD - revalidator wrongly modify userspace megaflows])
> >> +
> >> +OVS_VSWITCHD_START(
> >> +[add-port br0 p1 \
> >> + -- set bridge br0 datapath-type=dummy \
> >> + -- set interface p1 type=dummy-pmd \
> >> + -- add-port br0 p2 \
> >> + -- set interface p2 type=dummy-pmd
> >> +])
>
> OK. I found the reason why the test is failing on FreeSBD.
> OVS doesn't implement CPU/NUMA discovery on non-linux systems,
> so dummy NUMA configuration has to be provided in order for
> OVS to actually create PMD threads. Without dummy NUMA config
> no threads are started and so no traffic is forwarded.
>
I see. will fix it in the next version.
>
> See other tests in the fie on how dummy NUMA is provided in
> the OVS_VSWITCHD_START macro.
>
> >> +
> >> +dnl Add one openflow rule and generate a megaflow.
> >> +AT_CHECK([ovs-ofctl add-flow br0 'table=0,in_port=p1,ip,nw_dst=
> 10.1.2.0/24,actions=p2'])
> >> +AT_CHECK([ovs-appctl netdev-dummy/receive p1
> 'ipv4(src=10.0.0.1,dst=10.1.2.2),tcp(src=1,dst=2)'])
>
> Not very important, but this packet is invalid.
> You're no specifying important fields of ipv4 header, for
> example the ipv4.proto is not set and hence equals zero.
> In this case the tcp configuration is considered invalid,
> because it's not a tcp packet.
>
Ok, will fix it in the next version.
>
> It's not an OpenFlow formatting and OVS doesn't fill out any
> missing fields. Everything that is missing will be zero.
> It's better to provide a correct packet here and below in
> the test to avoid potential issues in the future.
>
> Best regards, Ilya Maximets.
>
> >> +
> >> +OVS_WAIT_UNTIL_EQUAL([ovs-appctl dpctl/dump-flows | sed 's/.*core:
> [[0-9]]*//'], [
> >>
> +recirc_id(0),in_port(1),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(dst=
> 10.1.2.2/255.255.255.0,frag=no), packets:0, bytes:0, used:never,
> actions:2])
> >
> > For some reason this check keeps failing on FreeBSD. Investigating...
> >
> > Best regards, Ilya Maximets.
> >
> >> +
> >> +AT_CHECK([ovs-appctl netdev-dummy/receive p1
> 'ipv4(src=10.0.0.1,dst=10.1.2.2),tcp(src=1,dst=2)'])
> >> +dnl Replace openflow rules, trigger the revalidation.
> >> +AT_CHECK([echo 'table=0,in_port=p1,ip,nw_dst=10.1.0.0/16
> actions=ct(commit)' | dnl
> >> + ovs-ofctl --bundle replace-flows br0 -])
> >> +AT_CHECK([ovs-appctl revalidator/wait])
> >> +
> >> +AT_CHECK([ovs-appctl netdev-dummy/receive p1
> 'ipv4(src=10.0.0.1,dst=10.1.0.2),tcp(src=1,dst=2)'])
> >> +OVS_WAIT_UNTIL_EQUAL([ovs-appctl dpctl/dump-flows | sed 's/.*core:
> [[0-9]]*//' | strip_xout_keep_actions], [
> >>
> +recirc_id(0),in_port(1),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(dst=
> 10.1.0.2/255.255.0.0,frag=no), packets:0, bytes:0, used:never,
> actions:ct(commit)
> >>
> +recirc_id(0),in_port(1),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(dst=
> 10.1.2.2/255.255.255.0,frag=no), packets:0, bytes:0, used:0.0s,
> actions:ct(commit)])
> >> +
> >> +dnl Hold the prefix 10.1.2.2/24 by another 10s.
> >> +AT_CHECK([ovs-appctl netdev-dummy/receive p1
> 'ipv4(src=10.0.0.1,dst=10.1.2.2),tcp(src=1,dst=2)'])
> >> +dnl Send more 10.1.0.2 to make 10.1.0.0/16 tuple preprend 10.1.2.0/24
> tuple in the pvector of subtables.
> >> +for i in $(seq 0 256); do
> >> + AT_CHECK([ovs-appctl netdev-dummy/receive p1
> 'ipv4(src=10.0.0.1,dst=10.1.0.2),tcp(src=1,dst=2)'])
> >> +done
> >> +
> >> +AT_CHECK([echo 'table=0,in_port=p1,ip,nw_dst=10.1.0.0/16 actions=p2'
> | dnl
> >> + ovs-ofctl --bundle replace-flows br0 -])
> >> +
> >> +AT_CHECK([ovs-appctl revalidator/wait])
> >> +AT_CHECK([ovs-appctl dpctl/dump-flows | sed 's/.*core: [[0-9]]*//' |
> strip_xout_keep_actions], [0], [
> >>
> +recirc_id(0),in_port(1),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(dst=
> 10.1.0.2/255.255.0.0,frag=no), packets:0, bytes:0, used:0.0s, actions:2
> >>
> +recirc_id(0),in_port(1),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(dst=
> 10.1.2.2/255.255.255.0,frag=no), packets:0, bytes:0, used:0.0s, actions:2
> >> +])
> >> +
> >> +OVS_VSWITCHD_STOP
> >> +AT_CLEANUP
> >
>
>
--
hepeng
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
