[ovs-dev] [bugfixes 2/3] datapath: Fix behavior of NLA_NESTED for pre-2.6.29 kernels.

[Jesse Gross]

On Mon, Jan 31, 2011 at 8:23 PM, Ben Pfaff <blp at nicira.com> wrote:
> On Mon, Jan 31, 2011 at 05:40:21PM -0800, Jesse Gross wrote:
>> On Mon, Jan 31, 2011 at 4:48 PM, Ben Pfaff <blp at nicira.com> wrote:
>> > Before v2.6.29, a NLA_NESTED attribute, if it was present, was not allowed
>> > to be empty. ?However, OVS depends on the ability to accept empty
>> > attributes. ?For example, a present but empty ODP_FLOW_ATTR_ACTIONS on
>> > ODP_FLOW_CMD_SET replaces the existing set of actions by an empty "drop"
>> > action, whereas a missing ODP_FLOW_ATTR_ACTIONS leaves the existing
>> > actions, if any, unchanged.
>> >
>> > The datapath code currently doesn't otherwise depend on the NLA_NESTED
>> > semantics (we only pass such attributes to nla_parse_nested() and
>> > nla_for_each_nested(), which don't care if the attribute's length is
>> > valid), so we might as well just use NLA_UNSPEC here.
>>
>> I don't understand this comment.  nla_parse_nested() absolutely cares
>> that the attribute's length is valid: it uses it to know how much data
>> is there to parse.  However, the previous round of parsing checks that
>> attributes are well formed and that the length of the attribute does
>> not exceed the available size.
>>
>> That being said, I only see two differences between NLA_NESTED and NLA_UNSPEC:
>> * If the size of the nested attributes is zero, no further size checks
>> are performed.
>> * If the size of the nested attributes is not zero and no length
>> parameter is specified the minimum size of nested attributes is
>> NLA_HDRLEN.
>>
>> nla_parse_nested() validates that there is at least enough space for
>> NLA_HDRLEN, so neither of these conditions are important.  So I think
>> that this change is fine but the description is very misleading.
>
> OK, here's an updated change log:
>
>    datapath: Fix behavior of NLA_NESTED for pre-2.6.29 kernels.
>
>    Before v2.6.29, a NLA_NESTED attribute, if it was present, was not allowed
>    to be empty.  However, OVS depends on the ability to accept empty
>    attributes.  For example, a present but empty ODP_FLOW_ATTR_ACTIONS on
>    ODP_FLOW_CMD_SET replaces the existing set of actions by an empty "drop"
>    action, whereas a missing ODP_FLOW_ATTR_ACTIONS leaves the existing
>    actions, if any, unchanged.
>
>    NLA_NESTED is different from NLA_UNSPEC in only two ways:
>
>    * If the size of the nested attributes is zero, no further size checks
>      are performed.
>
>    * If the size of the nested attributes is not zero and no length
>      parameter is specified the minimum size of nested attributes is
>      NLA_HDRLEN.
>
>    nla_parse_nested() validates that there is at least enough space for
>    NLA_HDRLEN, so neither of these conditions are important, and we might
>    as well use NLA_UNSPEC with old kernels.
>
>    Signed-off-by: Ben Pfaff <blp at nicira.com>

Thanks.  Can you put that (or some version of it) in the comment as well?  Then:
Acked-by: Jesse Gross <jesse at nicira.com>