[ovs-dev] ofproto-dpif-upcall: Pass key to dpif_flow_get().

Windows datapath folks have reported instances where OVS userspace will
pass down a flow_get request to the datapath using a UFID even though the
datapath has no support for UFIDs. Since commit e672ff9b4d22
("ofproto-dpif: Restore metadata and registers on recirculation."), if a
flow dump provides a flow that userspace isn't aware of, and the flow
dump doesn't provide actions for that flow, then userspace will attempt
a flow_get using just the UFID. This is because the ofproto-dpif layer
doesn't pass the key down to the dpif layer even if it's available.
Prior to the above commit, the codepath was only hit if the key was not
available, which would have implied UFID support. This assumption is now
broken: An empty set of actions could also trigger flow_get, and
datapaths without UFID support are free to pass up empty actions lists.

This patch should fix the issue by ensuring that the flow key is passed
down, so the dpif layer should have all of the relevant information to
craft the appropriate datapath flow_get request for the underlying
datapath, whether UFID is supported or not.

Fixes: e672ff9b4d22 ("ofproto-dpif: Restore metadata and registers on recirculation.")
Reported-by: Sairam Venugopal <vsairam@vmware.com>
Signed-off-by: Joe Stringer <joe@ovn.org>
---
 ofproto/ofproto-dpif-upcall.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Acked-by: Jarno Rajahalme <jarno@ovn.org>

It seems to me that the UFID is still passed down alongside the key, assuming that is ok.

  Jarno

> On May 10, 2016, at 3:42 PM, Joe Stringer <joe@ovn.org> wrote:
> 
> Windows datapath folks have reported instances where OVS userspace will
> pass down a flow_get request to the datapath using a UFID even though the
> datapath has no support for UFIDs. Since commit e672ff9b4d22
> ("ofproto-dpif: Restore metadata and registers on recirculation."), if a
> flow dump provides a flow that userspace isn't aware of, and the flow
> dump doesn't provide actions for that flow, then userspace will attempt
> a flow_get using just the UFID. This is because the ofproto-dpif layer
> doesn't pass the key down to the dpif layer even if it's available.
> Prior to the above commit, the codepath was only hit if the key was not
> available, which would have implied UFID support. This assumption is now
> broken: An empty set of actions could also trigger flow_get, and
> datapaths without UFID support are free to pass up empty actions lists.
> 
> This patch should fix the issue by ensuring that the flow key is passed
> down, so the dpif layer should have all of the relevant information to
> craft the appropriate datapath flow_get request for the underlying
> datapath, whether UFID is supported or not.
> 
> Fixes: e672ff9b4d22 ("ofproto-dpif: Restore metadata and registers on recirculation.")
> Reported-by: Sairam Venugopal <vsairam@vmware.com>
> Signed-off-by: Joe Stringer <joe@ovn.org>
> ---
> ofproto/ofproto-dpif-upcall.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> index f62f46b8e337..52c42ab41a47 100644
> --- a/ofproto/ofproto-dpif-upcall.c
> +++ b/ofproto/ofproto-dpif-upcall.c
> @@ -1518,7 +1518,7 @@ ukey_create_from_dpif_flow(const struct udpif *udpif,
>         /* If the key or actions were not provided by the datapath, fetch the
>          * full flow. */
>         ofpbuf_use_stack(&buf, &stub, sizeof stub);
> -        err = dpif_flow_get(udpif->dpif, NULL, 0, &flow->ufid,
> +        err = dpif_flow_get(udpif->dpif, flow->key, flow->key_len, &flow->ufid,
>                             flow->pmd_id, &buf, &full_flow);
>         if (err) {
>             return err;
> -- 
> 2.1.4
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev

On 10 May 2016 at 16:00, Jarno Rajahalme <jarno@ovn.org> wrote:
> Acked-by: Jarno Rajahalme <jarno@ovn.org>
>
> It seems to me that the UFID is still passed down alongside the key, assuming that is ok.

It seems that the proper way is to only pass it down if
flow->ufid_present is true. I amended that and pushed to master and
branch-2.[45]. Thanks!