On 25 Nov 2022, at 17:05, Adrian Moreno wrote:
> On 10/18/22 18:42, Eelco Chaudron wrote: >> The datapath supports installing wider flows, and OVS relies on >> this behavior. For example if ipv4(src=1.1.1.1/192.0.0.0, >> dst=1.1.1.2/192.0.0.0) exists, a wider flow (smaller mask) of >> ipv4(src=192.1.1.1/128.0.0.0,dst=192.1.1.2/128.0.0.0) is allowed >> to be added. >> >> However, if we try to add a wildcard rule, the installation fails: >> >> # ovs-appctl dpctl/add-flow system@myDP "in_port(1),eth_type(0x0800), \ >> ipv4(src=1.1.1.1/192.0.0.0,dst=1.1.1.2/192.0.0.0,frag=no)" 2 >> # ovs-appctl dpctl/add-flow system@myDP "in_port(1),eth_type(0x0800), \ >> ipv4(src=192.1.1.1/0.0.0.0,dst=49.1.1.2/0.0.0.0,frag=no)" 2 >> ovs-vswitchd: updating flow table (File exists) >> >> The reason is that the key used to determine if the flow is already >> present in the system uses the original key ANDed with the mask. >> This results in the IP address not being part of the (miniflow) key, >> i.e., being substituted with an all-zero value. When doing the actual >> lookup, this results in the key wrongfully matching the first flow, >> and therefore the flow does not get installed. The solution is to use >> the unmasked key for the existence check, the same way this is handled >> in the userspace datapath. >> >> Signed-off-by: Eelco Chaudron <[email protected]> >> --- >> lib/dpif-netdev.c | 33 +++++++++++++++++++++++++++++---- >> tests/dpif-netdev.at | 14 ++++++++++++++ >> 2 files changed, 43 insertions(+), 4 deletions(-) >> >> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c >> index a45b46014..daa00aa2f 100644 >> --- a/lib/dpif-netdev.c >> +++ b/lib/dpif-netdev.c >> @@ -3321,6 +3321,28 @@ netdev_flow_key_init_masked(struct netdev_flow_key >> *dst, >> (dst_u64 - miniflow_get_values(&dst->mf)) * 8); >> } >> +/* Initializes 'dst' as a copy of 'flow'. */ > > nit: s/dst/key/ Hi Adrian, Thanks for the review. There was already a v2 with an updated commit message, however, I just sent out a v3 with this change included. //Eelco >> +static inline void >> +netdev_flow_key_init(struct netdev_flow_key *key, >> + const struct flow *flow) >> +{ >> + uint64_t *dst = miniflow_values(&key->mf); >> + uint32_t hash = 0; >> + uint64_t value; >> + >> + miniflow_map_init(&key->mf, flow); >> + miniflow_init(&key->mf, flow); >> + >> + size_t n = dst - miniflow_get_values(&key->mf); >> + >> + FLOW_FOR_EACH_IN_MAPS (value, flow, key->mf.map) { >> + hash = hash_add64(hash, value); >> + } >> + >> + key->hash = hash_finish(hash, n * 8); >> + key->len = netdev_flow_key_size(n); >> +} >> + >> static inline void >> emc_change_entry(struct emc_entry *ce, struct dp_netdev_flow *flow, >> const struct netdev_flow_key *key) >> @@ -4195,7 +4217,7 @@ static int >> dpif_netdev_flow_put(struct dpif *dpif, const struct dpif_flow_put *put) >> { >> struct dp_netdev *dp = get_dp_netdev(dpif); >> - struct netdev_flow_key key, mask; >> + struct netdev_flow_key key; >> struct dp_netdev_pmd_thread *pmd; >> struct match match; >> ovs_u128 ufid; >> @@ -4244,9 +4266,12 @@ dpif_netdev_flow_put(struct dpif *dpif, const struct >> dpif_flow_put *put) >> /* Must produce a netdev_flow_key for lookup. >> * Use the same method as employed to create the key when adding >> - * the flow to the dplcs to make sure they match. */ >> - netdev_flow_mask_init(&mask, &match); >> - netdev_flow_key_init_masked(&key, &match.flow, &mask); >> + * the flow to the dplcs to make sure they match. >> + * We need to put in the unmasked key as flow_put_on_pmd() will first >> try >> + * to see if an entry exists doing a packet type lookup. As masked-out >> + * fields are interpreted as zeros, they could falsely match a wider IP >> + * address mask. Installation of the flow will use the match variable. >> */ >> + netdev_flow_key_init(&key, &match.flow); >> if (put->pmd_id == PMD_ID_NULL) { >> if (cmap_count(&dp->poll_threads) == 0) { >> diff --git a/tests/dpif-netdev.at b/tests/dpif-netdev.at >> index 3179e1645..32054c52e 100644 >> --- a/tests/dpif-netdev.at >> +++ b/tests/dpif-netdev.at >> @@ -636,6 +636,20 @@ OVS_VSWITCHD_STOP(["/flow: in_port is not an exact >> match/d >> /failed to put/d"]) >> AT_CLEANUP >> +AT_SETUP([dpif-netdev - check dpctl/add-flow wider ip match]) >> +OVS_VSWITCHD_START( >> + [add-port br0 p1 \ >> + -- set interface p1 type=dummy options:pstream=punix:$OVS_RUNDIR/p0.sock >> \ >> + -- set bridge br0 datapath-type=dummy]) >> + >> +AT_CHECK([ovs-appctl revalidator/pause]) >> +AT_CHECK([ovs-appctl dpctl/add-flow >> "in_port(1),eth_type(0x0800),ipv4(src=0.0.0.0/192.0.0.0,dst=0.0.0.0/192.0.0.0,frag=no)" >> "3"]) >> +AT_CHECK([ovs-appctl dpctl/add-flow >> "in_port(1),eth_type(0x0800),ipv4(src=192.1.1.1/0.0.0.0,dst=49.1.1.1/0.0.0.0,frag=no)" >> "3"]) >> +AT_CHECK([ovs-appctl revalidator/resume]) >> + >> +OVS_VSWITCHD_STOP >> +AT_CLEANUP >> + >> # SEND_UDP_PKTS([p_name], [p_ofport]) >> # >> # Sends 128 packets to port 'p_name' with different UDP destination ports. >> >> _______________________________________________ >> dev mailing list >> [email protected] >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> > > -- > Adrián Moreno > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
