Re: [PATCH net] net: openvswitch: fix TTL decrement action netlink message format

["Eelco Chaudron" <echaudro@redhat.com>]

On 20 Nov 2020, at 22:12, Jakub Kicinski wrote:

> On Thu, 19 Nov 2020 04:04:04 -0500 Eelco Chaudron wrote:
>> Currently, the openvswitch module is not accepting the correctly 
>> formated
>> netlink message for the TTL decrement action. For both setting and 
>> getting
>> the dec_ttl action, the actions should be nested in the
>> OVS_DEC_TTL_ATTR_ACTION attribute as mentioned in the openvswitch.h 
>> uapi.
>
> IOW this change will not break any known user space, correct?

It will not as there isn’t any yet. Unfortunately, the original patch 
was sent out without a userspace part. It was internally tested by the 
original authors and not properly reviewed to bring forward the issue. 
They did add some weird code to work around it.

> But existing OvS user space already expects it to work like you
> make it work now?
>
> What's the harm in leaving it as is?

Without this change, the different Datapaths in OVS behave differently, 
making the code to be datapath agnostic having to do all kinds of weird 
tricks to work around it.

But even worse, the patch in the current format could interpret 
additional options/attributes as actions, due to the actions not being 
encapsulated/nested within the actual attribute.

>> Fixes: 744676e77720 ("openvswitch: add TTL decrement action")
>> Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
>
> Can we get a review from OvS folks? Matteo looks good to you (as the
> original author)?

See Matteo’s reply, looks like he is ok with this change.

>> -	err = __ovs_nla_copy_actions(net, attr, key, sfa, eth_type,
>> +	err = __ovs_nla_copy_actions(net, actions, key, sfa, eth_type,
>>  				     vlan_tci, mpls_label_count, log);
>>  	if (err)
>>  		return err;
>
> You're not canceling any nests on error, I assume this is normal.

Yes, on error the sfa actions are not used.

>> +	add_nested_action_end(*sfa, action_start);
>>  	add_nested_action_end(*sfa, start);
>>  	return 0;
>>  }