Message ID | 9e3b73cd6967927fc6654cbdcd7b9e7431441c3f.1576488935.git.martin.varghese@nokia.com |
---|---|
State | Changes Requested |
Delegated to: | David Miller |
Headers | show |
Series | New openvswitch MPLS actions for layer 2 tunnelling | expand |
On Mon, 16 Dec 2019 19:33:43 +0530, Martin Varghese wrote: > diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h > index a87b44c..b7221ad 100644 > --- a/include/uapi/linux/openvswitch.h > +++ b/include/uapi/linux/openvswitch.h > @@ -673,6 +673,25 @@ struct ovs_action_push_mpls { > }; > > /** > + * struct ovs_action_ptap_push_mpls - %OVS_ACTION_ATTR_PTAP_PUSH_MPLS action > + * argument. > + * @mpls_lse: MPLS label stack entry to push. > + * @mpls_ethertype: Ethertype to set in the encapsulating ethernet frame. > + * @l2_tun: Flag to specify the place of insertion of MPLS header. > + * When true, the MPLS header will be inserted at the start of the packet. > + * When false, the MPLS header will be inserted at the start of the l3 header. > + * > + * The only values @mpls_ethertype should ever be given are %ETH_P_MPLS_UC and > + * %ETH_P_MPLS_MC, indicating MPLS unicast or multicast. Other are rejected. > + */ > +struct ovs_action_ptap_push_mpls { > + __be32 mpls_lse; > + __be16 mpls_ethertype; /* Either %ETH_P_MPLS_UC or %ETH_P_MPLS_MC */ > + bool l2_tun; In file included from <command-line>:32: ./usr/include/linux/openvswitch.h:674:2: error: unknown type name ‘bool’ 674 | bool l2_tun; | ^~~~ make[3]: *** [usr/include/linux/openvswitch.hdrtest] Error 1 > +}; > + > + Why the double new line? Please use checkpatch --strict.
On Mon, Dec 16, 2019 at 04:13:55PM -0800, Jakub Kicinski wrote: > On Mon, 16 Dec 2019 19:33:43 +0530, Martin Varghese wrote: > > diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h > > index a87b44c..b7221ad 100644 > > --- a/include/uapi/linux/openvswitch.h > > +++ b/include/uapi/linux/openvswitch.h > > @@ -673,6 +673,25 @@ struct ovs_action_push_mpls { > > }; > > > > /** > > + * struct ovs_action_ptap_push_mpls - %OVS_ACTION_ATTR_PTAP_PUSH_MPLS action > > + * argument. > > + * @mpls_lse: MPLS label stack entry to push. > > + * @mpls_ethertype: Ethertype to set in the encapsulating ethernet frame. > > + * @l2_tun: Flag to specify the place of insertion of MPLS header. > > + * When true, the MPLS header will be inserted at the start of the packet. > > + * When false, the MPLS header will be inserted at the start of the l3 header. > > + * > > + * The only values @mpls_ethertype should ever be given are %ETH_P_MPLS_UC and > > + * %ETH_P_MPLS_MC, indicating MPLS unicast or multicast. Other are rejected. > > + */ > > +struct ovs_action_ptap_push_mpls { > > + __be32 mpls_lse; > > + __be16 mpls_ethertype; /* Either %ETH_P_MPLS_UC or %ETH_P_MPLS_MC */ > > + bool l2_tun; > > In file included from <command-line>:32: > ./usr/include/linux/openvswitch.h:674:2: error: unknown type name ‘bool’ > 674 | bool l2_tun; > | ^~~~ > make[3]: *** [usr/include/linux/openvswitch.hdrtest] Error 1 > Does that mean bool cannot be used in interface header files ? but what is the alternative u8? > > +}; > > + > > + > > Why the double new line? Please use checkpatch --strict. Noted Thanks for your time.
On Tue, 17 Dec 2019 22:45:39 +0530, Martin Varghese wrote: > On Mon, Dec 16, 2019 at 04:13:55PM -0800, Jakub Kicinski wrote: > > On Mon, 16 Dec 2019 19:33:43 +0530, Martin Varghese wrote: > > > diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h > > > index a87b44c..b7221ad 100644 > > > --- a/include/uapi/linux/openvswitch.h > > > +++ b/include/uapi/linux/openvswitch.h > > > @@ -673,6 +673,25 @@ struct ovs_action_push_mpls { > > > }; > > > > > > /** > > > + * struct ovs_action_ptap_push_mpls - %OVS_ACTION_ATTR_PTAP_PUSH_MPLS action > > > + * argument. > > > + * @mpls_lse: MPLS label stack entry to push. > > > + * @mpls_ethertype: Ethertype to set in the encapsulating ethernet frame. > > > + * @l2_tun: Flag to specify the place of insertion of MPLS header. > > > + * When true, the MPLS header will be inserted at the start of the packet. > > > + * When false, the MPLS header will be inserted at the start of the l3 header. > > > + * > > > + * The only values @mpls_ethertype should ever be given are %ETH_P_MPLS_UC and > > > + * %ETH_P_MPLS_MC, indicating MPLS unicast or multicast. Other are rejected. > > > + */ > > > +struct ovs_action_ptap_push_mpls { > > > + __be32 mpls_lse; > > > + __be16 mpls_ethertype; /* Either %ETH_P_MPLS_UC or %ETH_P_MPLS_MC */ > > > + bool l2_tun; > > > > In file included from <command-line>:32: > > ./usr/include/linux/openvswitch.h:674:2: error: unknown type name ‘bool’ > > 674 | bool l2_tun; > > | ^~~~ > > make[3]: *** [usr/include/linux/openvswitch.hdrtest] Error 1 > > > > Does that mean bool cannot be used in interface header files ? but what is > the alternative u8? I think you have a 16 bit hole there anyway due to the alignment, so I'd declare the space as __u16 (underscores needed since this is the user space type), and then validate in the kernel that the higher bits are unused i.e. return -EINVAL if the field is not 0 or 1, to allow for a future use of those bits.
diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h index a87b44c..b7221ad 100644 --- a/include/uapi/linux/openvswitch.h +++ b/include/uapi/linux/openvswitch.h @@ -673,6 +673,25 @@ struct ovs_action_push_mpls { }; /** + * struct ovs_action_ptap_push_mpls - %OVS_ACTION_ATTR_PTAP_PUSH_MPLS action + * argument. + * @mpls_lse: MPLS label stack entry to push. + * @mpls_ethertype: Ethertype to set in the encapsulating ethernet frame. + * @l2_tun: Flag to specify the place of insertion of MPLS header. + * When true, the MPLS header will be inserted at the start of the packet. + * When false, the MPLS header will be inserted at the start of the l3 header. + * + * The only values @mpls_ethertype should ever be given are %ETH_P_MPLS_UC and + * %ETH_P_MPLS_MC, indicating MPLS unicast or multicast. Other are rejected. + */ +struct ovs_action_ptap_push_mpls { + __be32 mpls_lse; + __be16 mpls_ethertype; /* Either %ETH_P_MPLS_UC or %ETH_P_MPLS_MC */ + bool l2_tun; +}; + + +/** * struct ovs_action_push_vlan - %OVS_ACTION_ATTR_PUSH_VLAN action argument. * @vlan_tpid: Tag protocol identifier (TPID) to push. * @vlan_tci: Tag control identifier (TCI) to push. The CFI bit must be set @@ -892,7 +911,8 @@ struct check_pkt_len_arg { * @OVS_ACTION_ATTR_CHECK_PKT_LEN: Check the packet length and execute a set * of actions if greater than the specified packet length, else execute * another set of actions. - * + * @OVS_ACTION_ATTR_PUSH_MPLS: Push a new MPLS label stack entry at specified + * offset from start of packet. * Only a single header can be set with a single %OVS_ACTION_ATTR_SET. Not all * fields within a header are modifiable, e.g. the IPv4 protocol and fragment * type may not be changed. @@ -927,6 +947,7 @@ enum ovs_action_attr { OVS_ACTION_ATTR_METER, /* u32 meter ID. */ OVS_ACTION_ATTR_CLONE, /* Nested OVS_CLONE_ATTR_*. */ OVS_ACTION_ATTR_CHECK_PKT_LEN, /* Nested OVS_CHECK_PKT_LEN_ATTR_*. */ + OVS_ACTION_ATTR_PTAP_PUSH_MPLS, /* struct ovs_action_ptap_push_mpls. */ __OVS_ACTION_ATTR_MAX, /* Nothing past this will be accepted * from userspace. */ diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c index 4c83954..5138473 100644 --- a/net/openvswitch/actions.c +++ b/net/openvswitch/actions.c @@ -161,16 +161,17 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb, const struct nlattr *attr, int len); static int push_mpls(struct sk_buff *skb, struct sw_flow_key *key, - const struct ovs_action_push_mpls *mpls) + __be32 mpls_lse, __be16 mpls_ethertype, __u16 mac_len) { int err; - err = skb_mpls_push(skb, mpls->mpls_lse, mpls->mpls_ethertype, - skb->mac_len, - ovs_key_mac_proto(key) == MAC_PROTO_ETHERNET); + err = skb_mpls_push(skb, mpls_lse, mpls_ethertype, mac_len, !!mac_len); if (err) return err; + if (!mac_len) + key->mac_proto = MAC_PROTO_NONE; + invalidate_flow_key(key); return 0; } @@ -185,6 +186,9 @@ static int pop_mpls(struct sk_buff *skb, struct sw_flow_key *key, if (err) return err; + if (ethertype == htons(ETH_P_TEB)) + key->mac_proto = MAC_PROTO_ETHERNET; + invalidate_flow_key(key); return 0; } @@ -1229,10 +1233,24 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb, execute_hash(skb, key, a); break; - case OVS_ACTION_ATTR_PUSH_MPLS: - err = push_mpls(skb, key, nla_data(a)); + case OVS_ACTION_ATTR_PUSH_MPLS: { + struct ovs_action_push_mpls *mpls = nla_data(a); + + err = push_mpls(skb, key, mpls->mpls_lse, + mpls->mpls_ethertype, skb->mac_len); break; + } + case OVS_ACTION_ATTR_PTAP_PUSH_MPLS: { + struct ovs_action_ptap_push_mpls *mpls = nla_data(a); + __u16 mac_len = 0; + + if (!mpls->l2_tun) + mac_len = skb->mac_len; + err = push_mpls(skb, key, mpls->mpls_lse, + mpls->mpls_ethertype, mac_len); + break; + } case OVS_ACTION_ATTR_POP_MPLS: err = pop_mpls(skb, key, nla_get_be16(a)); break; diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c index 65c2e34..8d650e3 100644 --- a/net/openvswitch/flow_netlink.c +++ b/net/openvswitch/flow_netlink.c @@ -79,6 +79,7 @@ static bool actions_may_change_flow(const struct nlattr *actions) case OVS_ACTION_ATTR_SET_MASKED: case OVS_ACTION_ATTR_METER: case OVS_ACTION_ATTR_CHECK_PKT_LEN: + case OVS_ACTION_ATTR_PTAP_PUSH_MPLS: default: return true; } @@ -3005,6 +3006,7 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr, [OVS_ACTION_ATTR_METER] = sizeof(u32), [OVS_ACTION_ATTR_CLONE] = (u32)-1, [OVS_ACTION_ATTR_CHECK_PKT_LEN] = (u32)-1, + [OVS_ACTION_ATTR_PTAP_PUSH_MPLS] = sizeof(struct ovs_action_ptap_push_mpls), }; const struct ovs_action_push_vlan *vlan; int type = nla_type(a); @@ -3072,6 +3074,33 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr, case OVS_ACTION_ATTR_RECIRC: break; + case OVS_ACTION_ATTR_PTAP_PUSH_MPLS: { + const struct ovs_action_ptap_push_mpls *mpls = nla_data(a); + + if (!eth_p_mpls(mpls->mpls_ethertype)) + return -EINVAL; + + if (!mpls->l2_tun) { + if (vlan_tci & htons(VLAN_CFI_MASK) || + (eth_type != htons(ETH_P_IP) && + eth_type != htons(ETH_P_IPV6) && + eth_type != htons(ETH_P_ARP) && + eth_type != htons(ETH_P_RARP) && + !eth_p_mpls(eth_type))) + return -EINVAL; + mpls_label_count++; + } else { + if (mac_proto != MAC_PROTO_NONE) { + mpls_label_count = 1; + mac_proto = MAC_PROTO_NONE; + } else { + mpls_label_count++; + } + } + eth_type = mpls->mpls_ethertype; + break; + } + case OVS_ACTION_ATTR_PUSH_MPLS: { const struct ovs_action_push_mpls *mpls = nla_data(a); @@ -3109,6 +3138,11 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr, * recirculation. */ proto = nla_get_be16(a); + + if (proto == htons(ETH_P_TEB) && + mac_proto != MAC_PROTO_NONE) + return -EINVAL; + mpls_label_count--; if (!eth_p_mpls(proto) || !mpls_label_count)