On 10/14/20 2:36 PM, Jan Scheurich wrote:
>>>> Fix that by clearing padding bytes while encoding, and checking that
>>>> these bytes are all zeros on decoding.
>>>
>>> Is the latter strictly necessary? It may break existing controllers that do
>>> not
>> initialize the padding bytes to zero.
>>> Wouldn't it be sufficient to just zero the padding bytes at reception?
>>
>> I do not have a strong opinion. I guess, we could not fail OF request if
>> padding is not all zeroes for backward compatibility.
>> Anyway, it seems like I missed one part of this change (see inline).
>>
>> On the other hand, AFAIU, NXOXM_NSH_ is not standardized, so, technically,
>> we could change the rules here. As an option, we could apply the patch
>> without checking for all-zeroes padding and backport it this way to stable
>> branches. Afterwards, we could introduce the 'is_all_zeros' check and
>> mention this change in release notes for the new version. Anyway OpenFlow
>> usually requires paddings to be all-zeroes for most of matches and actions,
>> so
>> this should be a sane requirement for controllers.
>> What do you think?
>>
>
> I think there is little to gain by enforcing strict rules on zeroed padding
> bytes in a future release. It just creates grief with users of OVS by
> unnecessarily breaking backward compatibility without any benefit for OVS. No
> matter if OVS is has the right to do so or not.
OK. Agree.
>
>>>> diff --git a/lib/ofp-ed-props.c b/lib/ofp-ed-props.c index
>>>> 28382e012..5a4b12d9f 100644
>>>> --- a/lib/ofp-ed-props.c
>>>> +++ b/lib/ofp-ed-props.c
>>>> @@ -48,6 +48,9 @@ decode_ed_prop(const struct ofp_ed_prop_header
>>>> **ofp_prop,
>>>> if (len > sizeof(*opnmt) || len > *remaining) {
>>>> return OFPERR_NXBAC_BAD_ED_PROP;
>>>> }
>>>> + if (!is_all_zeros(opnmt->pad, sizeof opnmt->pad)) {
>>>> + return OFPERR_NXBRC_MUST_BE_ZERO;
>>>> + }
>>>> struct ofpact_ed_prop_nsh_md_type *pnmt =
>>>> ofpbuf_put_uninit(out, sizeof(*pnmt));
>>
>> This should be 'ofpbuf_put_zeroes' because 'struct
>> ofpact_ed_prop_nsh_md_type'
>> contains padding too that must be cleared while constructing ofpacts.
>> Since OVS compares decoded ofpacts' and not the original OF messages, this
>> should do the trick.
>
> Agree.
>
>>
>> I'll send v2 with this change and will remove 'is_all_zeros' check for this
>> fix.
>
> Thanks, Jan
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
