[ovs-discuss] [ovn] non-maskable OVS meta-flow fields not correctly handled

[ythomas1.ext at orange.com]

Hi Ben,

Thanks for your answer.

Ben Pfaff:
> On Thu, Nov 05, 2020 at 12:39:38PM +0000, ythomas1.ext at orange.com wrote:
> > I’m currently working on OVN to add MPLS logical fields (such as
> > 'mpls_label', 'mpls_bos') to support BGP/MPLS technology.
> >
> > As indicated in 'mf_field' struct in meta-flow.h, most OVS meta-flow
> > fields have n_bytes * 8 == n_bits size but there are few exceptions
> > such as 'mpls_label', 'mpls_bos', etc…
> >
> > In OVN case, when calling 'mf_mask_subfield' method from meta-flow.c,
> > for example for 'mpls_label' OVS meta-flow field which has 20 useful
> > bits over 32 bits, the 'mask' value parsed in OVN is set to 0xffff00ff
> > (which seems correct).
>
> OVN doesn't use that function much.  I see one use in constrain_match().

I agree with the fact that there is only one use in constrain_match(), but this call to mf_mask_subfield() is used to set Openflow matches, converted from the OVN matches expression.

>
> I don't understand where 0xffff00ff comes from.  The mask should not be
> discontiguous like that.

Sorry for the confusion, the actual value once in network byte order, has indeed only consecutive 1s (0x000fffff).

>
> > The problem is that in 'mf_set' method, 'mpls_label' OVS meta-flow
> > field is supposed to only have a NULL, all-1-bits or all-0-bits 'mask'
> > value to be set, as shown below:
>
> [...]
>
> > However, as mentioned in 'mf_set' method comment, “The caller is
> > responsible for ensuring that 'match' meets 'mf''s prerequisites”.
> >
> > So, since the 'mpls_label' OVS meta-flow field is indicated as not
> > maskable in meta-flow.h, shouldn't OVN ensure that 'mask' is set to
> > NULL when calling 'mf_mask_subfield' ?
>
> The mask isn't a prerequisite.  The prerequisite for mpls_label is that
> the flow is an MPLS flow, that is, it has an MPLS ethertype.  See
> mf_are_prereqs_ok().

Ok.
The issue does not indeed seem to be related to these prereqs.

As shown in the following traces, we can see that mpls_label and mpls_bos fields are ignored in mf_set(). Their mask not being NULL or all-1-bits or all-0-bits, we are falling in the switch case where they are not handled (https://github.com/openvswitch/ovs/blob/master/lib/meta-flow.c#L2328).

2020-11-10T16:05:50.821Z|00192|lflow|INFO|CONSIDER_LOGICAL_FLOW -> OVN match to translate: mpls && mpls.bos == 1 && mpls.label == 18
2020-11-10T16:05:50.821Z|00193|meta_flow|INFO|MF_MASK_SUBFIELD -> Value [0x00008847] and mask [0x0000ffff] to copy to match
2020-11-10T16:05:50.821Z|00194|meta_flow|INFO|MF_MASK_SUBFIELD -> Updated match: mpls
2020-11-10T16:05:50.821Z|00195|meta_flow|INFO|MF_MASK_SUBFIELD -> Value [0x00000001] and mask [0x00000001] to copy to match
2020-11-10T16:05:50.821Z|00196|meta_flow|INFO|MF_SET -> mf_field [mpls_bos] value and mask not set, returning OFPUTIL_P_NONE
2020-11-10T16:05:50.821Z|00197|meta_flow|INFO|MF_MASK_SUBFIELD -> Updated match: mpls
2020-11-10T16:05:50.821Z|00198|meta_flow|INFO|MF_MASK_SUBFIELD -> Value [0x00000012] and mask [0x000fffff] to copy to match
2020-11-10T16:05:50.821Z|00199|meta_flow|INFO|MF_SET -> mf_field [mpls_label] value and mask not set, returning OFPUTIL_P_NONE
2020-11-10T16:05:50.821Z|00200|meta_flow|INFO|MF_MASK_SUBFIELD -> Updated match: mpls

So what should be the fix ? Should OVN ensure that the mask be set to all zeroes or all ones when calling mf_mask_subfield, or should mf_set be adjusted (e.g. treat mpls fields as 'a nontrivial mask' (to quote https://github.com/openvswitch/ovs/blob/master/lib/meta-flow.c#L2324 ) after introducing suitable match_set_mpls_label_masked/match_set_mpls_bos_masked/match_set_mpls_tc_masked/match_set_mpls_ttl_masked functions) ?

Best regards,

Yannick




_________________________________________________________________________________________________________________________

Ce message et ses pieces jointes peuvent contenir des informations confidentielles ou privilegiees et ne doivent donc
pas etre diffuses, exploites ou copies sans autorisation. Si vous avez recu ce message par erreur, veuillez le signaler
a l'expediteur et le detruire ainsi que les pieces jointes. Les messages electroniques etant susceptibles d'alteration,
Orange decline toute responsabilite si ce message a ete altere, deforme ou falsifie. Merci.

This message and its attachments may contain confidential or privileged information that may be protected by law;
they should not be distributed, used or copied without authorisation.
If you have received this email in error, please notify the sender and delete this message and its attachments.
As emails may be altered, Orange is not liable for messages that have been modified, changed or falsified.
Thank you.