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

[Zoltán Balogh]

> On Tue, Apr 25, 2017 at 04:30:23PM +0000, Zoltán Balogh wrote:
> > From: Jan Scheurich <jan.scheur...@ericsson.com>
> >
> > Add support for actions push_eth and pop_eth to the netdev datapath and
> > the supporting libraries. This patch relies on the support for these actions
> > in the kernel datapath to be present.
> 
> Hi, can you help me to understand this part here?  I believe that this
> will lead to userspace sending a OVS_ACTION_ATTR_PUSH_ETH action
> argument to the kernel that is different from what the kernel actually
> wants.  What's the idea, and how is that problem avoided?
> 
> struct ovs_action_push_eth {
>       struct ovs_key_ethernet addresses;
> #ifndef __KERNEL__
>     __be16  eth_type;
> #endif
> };
> 
> Thanks,
> 
> Ben.

Hi,

I'm afraid the kernel would not accept that OVS_ACTION_ATTR_PUSH_ETH action in 
this case.

Jiri, could you confirm, please?

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.

void
odp_put_push_eth_action(struct ofpbuf *odp_actions,
                        const struct eth_addr *eth_src,
                        const struct eth_addr *eth_dst,
                        const ovs_be16 eth_type)
{
    struct ovs_action_push_eth eth;

    memset(&eth, 0, sizeof eth);
    if (eth_src) {
        eth.addresses.eth_src = *eth_src;
    }
    if (eth_dst) {
        eth.addresses.eth_dst = *eth_dst;
    }

    nl_msg_put_unspec(odp_actions, OVS_ACTION_ATTR_PUSH_ETH,
                      &eth, sizeof eth);
    commit_set_action(odp_actions, OVS_KEY_ATTR_ETHERTYPE, eth_type);
}

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:


static void
odp_execute_set_action(struct dp_packet *packet, const struct nlattr *a)
{
    enum ovs_key_attr type = nl_attr_type(a);
    const struct ovs_key_ipv4 *ipv4_key;
    const struct ovs_key_ipv6 *ipv6_key;
    struct pkt_metadata *md = &packet->md;

    switch (type) {
...

    case OVS_KEY_ATTR_ETHERTYPE:
    case OVS_KEY_ATTR_IN_PORT:
    case OVS_KEY_ATTR_VLAN:
    case OVS_KEY_ATTR_TCP_FLAGS:
    case OVS_KEY_ATTR_CT_STATE:
    case OVS_KEY_ATTR_CT_ORIG_TUPLE_IPV4:
    case OVS_KEY_ATTR_CT_ORIG_TUPLE_IPV6:
    case OVS_KEY_ATTR_CT_ZONE:
    case OVS_KEY_ATTR_CT_MARK:
    case OVS_KEY_ATTR_CT_LABELS:
    case __OVS_KEY_ATTR_MAX:
    default:
        OVS_NOT_REACHED();
    }
}

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


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.

/*
 * struct ovs_action_push_eth - %OVS_ACTION_ATTR_PUSH_ETH action argument.
 * @addresses: Source and destination MAC addresses.
 * @eth_type: Ethernet type
 */
struct ovs_action_push_eth {
        struct ovs_key_ethernet addresses;
};

Best regards,
Zoltan


_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev