[ovs-dev] [PATCH v4 2/7] userspace: Support for push_eth and pop_eth actions

[Jiri Benc]

On Thu, 4 May 2017 10:02:22 +0000, Zoltán Balogh wrote:
> I'm afraid the kernel would not accept that OVS_ACTION_ATTR_PUSH_ETH
> action in this case.
> 
> Jiri, could you confirm, please?

It would accept it and it would ignore the eth_type field. The kernel
does not verify whether netlink attributes are longer than they should
be. It just accepts such attributes and silently ignores the excessive
data.

I would discourage doing this, though. The data structures between the
kernel and the user space should match.

(As a side note, we're currently having a discussion that this
treatment of excessively long attributes should be changed. Of course
this would require the user space asking for such new behavior by
setsockopt to keep compatibility with existing programs.)

> We could remove the eth_type from struct ovs_action_push_eth, then
> put an additional "set action" after putting "push_eth" in the
> odp_put_push_eth_action() function in order to set the ethertype of
> the packet.

This seems weird, though. Under which circumstances does ovs not know
what ethertype the packet has? The kernel knows about it (via
skb->protocol) all the time, with or without the Ethernet header. No
flow matching could work otherwise.

> That way we would split the push_eth action into a simpler push_eth
> and a set_field. However this would lead to modify
> odp_execute_set_action()as well, since changing of ethertype with
> set_field is not allowed:

Disallowing changing of ethertype is a sensible thing to do.

> I guess something similar should be done at kernel side too, since
> the kernel module would not accept set_field ethertype either.

It does not need it. The kernel knows what the ethertype is.

> Another option would be to convert OVS_ACTION_ATTR_PUSH_ETH action
> argument between userspace and kernel.
> 
> Do you have any other proposal?
> 
> Btw, the original comment message of struct ovs_action_push_eth
> indicates eth_type as a second member.

Thanks, this is a (documentation) bug. A leftover from one of the
previous versions that nobody noticed. I'll remove it upstream once
net-next reopens.

 Jiri