[ovs-dev] Check flow's dl_type before setting ct_orig_tuple in 'pkt_metadata_from_flow()'

From: Numan Siddique <nusiddiq@redhat.com>

Normally flow's dl_type will be a valid value. However when a packet is sent to
the controller, dl_type is not stored in the 'ofputil_packet_in_private'. When
the controller resumes (OFPRAW_NXT_RESUME) the packet, the flow's dl_type will be
0. If the flow's ct_state has valid value, then the 'pkt_metadata_from_flow'
neither sets the ct_orig_tuple from the flow nor resets it. This results in invalid
value ct_orig_tuple in the pkt_metadata.

This patch handles this situation by checking the dl_type before setting the
ct_orig_tuple. If dl_type is 0, it resets it.

Reported-by: Daniel Alvarez Sanchez <dalvarez@redhat.com>
Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2017-October/339868.html
Signed-off-by: Numan Siddique <nusiddiq@redhat.com>
---
 lib/flow.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

On Wed, Oct 25, 2017 at 01:44:23PM +0530, nusiddiq@redhat.com wrote:
> From: Numan Siddique <nusiddiq@redhat.com>
> 
> Normally flow's dl_type will be a valid value. However when a packet is sent to
> the controller, dl_type is not stored in the 'ofputil_packet_in_private'. When
> the controller resumes (OFPRAW_NXT_RESUME) the packet, the flow's dl_type will be
> 0. If the flow's ct_state has valid value, then the 'pkt_metadata_from_flow'
> neither sets the ct_orig_tuple from the flow nor resets it. This results in invalid
> value ct_orig_tuple in the pkt_metadata.
> 
> This patch handles this situation by checking the dl_type before setting the
> ct_orig_tuple. If dl_type is 0, it resets it.
> 
> Reported-by: Daniel Alvarez Sanchez <dalvarez@redhat.com>
> Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2017-October/339868.html
> Signed-off-by: Numan Siddique <nusiddiq@redhat.com>
> ---
>  lib/flow.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/flow.h b/lib/flow.h
> index 6ae5a674d..8dd1a09a0 100644
> --- a/lib/flow.h
> +++ b/lib/flow.h
> @@ -929,7 +929,7 @@ pkt_metadata_from_flow(struct pkt_metadata *md, const struct flow *flow)
>      md->ct_label = flow->ct_label;
>  
>      md->ct_orig_tuple_ipv6 = false;
> -    if (is_ct_valid(flow, NULL, NULL)) {
> +    if (flow->dl_type && is_ct_valid(flow, NULL, NULL)) {
>          if (flow->dl_type == htons(ETH_TYPE_IP)) {
>              md->ct_orig_tuple.ipv4 = (struct ovs_key_ct_tuple_ipv4) {
>                  flow->ct_nw_src,

Thanks for the patch.  It builds and passes all the tests.

There is still one case where ct_orig_tuple will not be initialized,
which is the case where dl_type is nonzero but not IPv4 or IPV6; for
example, if it is ARP.  Should we be careful to handle that case also?

On Wed, Oct 25, 2017 at 10:46 PM, Ben Pfaff <blp@ovn.org> wrote:

> On Wed, Oct 25, 2017 at 01:44:23PM +0530, nusiddiq@redhat.com wrote:
> > From: Numan Siddique <nusiddiq@redhat.com>
> >
> > Normally flow's dl_type will be a valid value. However when a packet is
> sent to
> > the controller, dl_type is not stored in the
> 'ofputil_packet_in_private'. When
> > the controller resumes (OFPRAW_NXT_RESUME) the packet, the flow's
> dl_type will be
> > 0. If the flow's ct_state has valid value, then the
> 'pkt_metadata_from_flow'
> > neither sets the ct_orig_tuple from the flow nor resets it. This results
> in invalid
> > value ct_orig_tuple in the pkt_metadata.
> >
> > This patch handles this situation by checking the dl_type before setting
> the
> > ct_orig_tuple. If dl_type is 0, it resets it.
> >
> > Reported-by: Daniel Alvarez Sanchez <dalvarez@redhat.com>
> > Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2017-
> October/339868.html
> > Signed-off-by: Numan Siddique <nusiddiq@redhat.com>
> > ---
> >  lib/flow.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/lib/flow.h b/lib/flow.h
> > index 6ae5a674d..8dd1a09a0 100644
> > --- a/lib/flow.h
> > +++ b/lib/flow.h
> > @@ -929,7 +929,7 @@ pkt_metadata_from_flow(struct pkt_metadata *md,
> const struct flow *flow)
> >      md->ct_label = flow->ct_label;
> >
> >      md->ct_orig_tuple_ipv6 = false;
> > -    if (is_ct_valid(flow, NULL, NULL)) {
> > +    if (flow->dl_type && is_ct_valid(flow, NULL, NULL)) {
> >          if (flow->dl_type == htons(ETH_TYPE_IP)) {
> >              md->ct_orig_tuple.ipv4 = (struct ovs_key_ct_tuple_ipv4) {
> >                  flow->ct_nw_src,
>
> Thanks for the patch.  It builds and passes all the tests.
>
> There is still one case where ct_orig_tuple will not be initialized,
> which is the case where dl_type is nonzero but not IPv4 or IPV6; for
> example, if it is ARP.  Should we be careful to handle that case also?
>

Agree. Updated the patch.

Thanks for the review.

Numan