[ovs-dev] [PATCH V2 branch-2.8 1/2] datapath: Properly set L4 keys on "later" IP fragments

Gregory Rose gvrose8192 at gmail.com
Thu Sep 5 15:01:52 UTC 2019



On 9/4/2019 10:37 PM, Justin Pettit wrote:
> Thanks, Greg.  I pushed the patches you sent for all the appropriate branches (2.5-2.8).  I sent out patches to do bug patch releases for all those branches, too, that will include these.  Hopefully we'll get those merged and all the releases out tomorrow.
>
> --Justin

Awesome!  Thanks!

>
>> On Sep 4, 2019, at 10:07 AM, Greg Rose <gvrose8192 at gmail.com> wrote:
>>
>> Upstream commit:
>>     commit ad06a566e118e57b852cab5933dbbbaebb141de3
>>     Author: Greg Rose <gvrose8192 at gmail.com>
>>     Date:   Tue Aug 27 07:58:09 2019 -0700
>>
>>     openvswitch: Properly set L4 keys on "later" IP fragments
>>
>>     When IP fragments are reassembled before being sent to conntrack, the
>>     key from the last fragment is used.  Unless there are reordering
>>     issues, the last fragment received will not contain the L4 ports, so the
>>     key for the reassembled datagram won't contain them.  This patch updates
>>     the key once we have a reassembled datagram.
>>
>>     The handle_fragments() function works on L3 headers so we pull the L3/L4
>>     flow key update code from key_extract into a new function
>>     'key_extract_l3l4'.  Then we add a another new function
>>     ovs_flow_key_update_l3l4() and export it so that it is accessible by
>>     handle_fragments() for conntrack packet reassembly.
>>
>>     Co-authored-by: Justin Pettit <jpettit at ovn.org>
>>     Signed-off-by: Greg Rose <gvrose8192 at gmail.com>
>>     Acked-by: Pravin B Shelar <pshelar at ovn.org>
>>     Signed-off-by: David S. Miller <davem at davemloft.net>
>>
>> Signed-off-by: Greg Rose <gvrose8192 at gmail.com>
>>
>> ---
>>
>> V2 - Fix compile errors
>> ---
>> datapath/conntrack.c |   5 ++
>> datapath/flow.c      | 157 +++++++++++++++++++++++++++++----------------------
>> datapath/flow.h      |   1 +
>> 3 files changed, 95 insertions(+), 68 deletions(-)
>>
>> diff --git a/datapath/conntrack.c b/datapath/conntrack.c
>> index 1990984..2c3e441 100644
>> --- a/datapath/conntrack.c
>> +++ b/datapath/conntrack.c
>> @@ -537,6 +537,11 @@ static int handle_fragments(struct net *net, struct sw_flow_key *key,
>> 		return -EPFNOSUPPORT;
>> 	}
>>
>> +	/* The key extracted from the fragment that completed this datagram
>> +	 * likely didn't have an L4 header, so regenerate it.
>> +	 */
>> +	ovs_flow_key_update_l3l4(skb, key);
>> +
>> 	key->ip.frag = OVS_FRAG_TYPE_NONE;
>> 	skb_clear_hash(skb);
>> 	skb->ignore_df = 1;
>> diff --git a/datapath/flow.c b/datapath/flow.c
>> index 99dae79..083288f 100644
>> --- a/datapath/flow.c
>> +++ b/datapath/flow.c
>> @@ -493,79 +493,14 @@ invalid:
>> }
>>
>> /**
>> - * key_extract - extracts a flow key from an Ethernet frame.
>> + * key_extract_l3l4 - extracts L3/L4 header information.
>>   * @skb: sk_buff that contains the frame, with skb->data pointing to the
>> - * Ethernet header
>> + *       L3 header
>>   * @key: output flow key
>> - *
>> - * The caller must ensure that skb->len >= ETH_HLEN.
>> - *
>> - * Returns 0 if successful, otherwise a negative errno value.
>> - *
>> - * Initializes @skb header fields as follows:
>> - *
>> - *    - skb->mac_header: the L2 header.
>> - *
>> - *    - skb->network_header: just past the L2 header, or just past the
>> - *      VLAN header, to the first byte of the L2 payload.
>> - *
>> - *    - skb->transport_header: If key->eth.type is ETH_P_IP or ETH_P_IPV6
>> - *      on output, then just past the IP header, if one is present and
>> - *      of a correct length, otherwise the same as skb->network_header.
>> - *      For other key->eth.type values it is left untouched.
>> - *
>> - *    - skb->protocol: the type of the data starting at skb->network_header.
>> - *      Equals to key->eth.type.
>>   */
>> -static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)
>> +static int key_extract_l3l4(struct sk_buff *skb, struct sw_flow_key *key)
>> {
>> 	int error;
>> -	struct ethhdr *eth;
>> -
>> -	/* Flags are always used as part of stats */
>> -	key->tp.flags = 0;
>> -
>> -	skb_reset_mac_header(skb);
>> -
>> -	/* Link layer. */
>> -	clear_vlan(key);
>> -	if (ovs_key_mac_proto(key) == MAC_PROTO_NONE) {
>> -		if (unlikely(eth_type_vlan(skb->protocol)))
>> -			return -EINVAL;
>> -
>> -		skb_reset_network_header(skb);
>> -		key->eth.type = skb->protocol;
>> -	} else {
>> -		eth = eth_hdr(skb);
>> -		ether_addr_copy(key->eth.src, eth->h_source);
>> -		ether_addr_copy(key->eth.dst, eth->h_dest);
>> -
>> -		__skb_pull(skb, 2 * ETH_ALEN);
>> -		/* We are going to push all headers that we pull, so no need to
>> -		 * update skb->csum here.
>> -		 */
>> -
>> -		if (unlikely(parse_vlan(skb, key)))
>> -			return -ENOMEM;
>> -
>> -		key->eth.type = parse_ethertype(skb);
>> -		if (unlikely(key->eth.type == htons(0)))
>> -			return -ENOMEM;
>> -
>> -		/* Multiple tagged packets need to retain TPID to satisfy
>> -		 * skb_vlan_pop(), which will later shift the ethertype into
>> -		 * skb->protocol.
>> -		 */
>> -		if (key->eth.cvlan.tci & htons(VLAN_TAG_PRESENT))
>> -			skb->protocol = key->eth.cvlan.tpid;
>> -		else
>> -			skb->protocol = key->eth.type;
>> -
>> -		skb_reset_network_header(skb);
>> -		__skb_push(skb, skb->data - skb_mac_header(skb));
>> -	}
>> -
>> -	skb_reset_mac_len(skb);
>>
>> 	/* Network layer. */
>> 	if (key->eth.type == htons(ETH_P_IP)) {
>> @@ -756,6 +691,92 @@ static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)
>> 	return 0;
>> }
>>
>> +/**
>> + * key_extract - extracts a flow key from an Ethernet frame.
>> + * @skb: sk_buff that contains the frame, with skb->data pointing to the
>> + * Ethernet header
>> + * @key: output flow key
>> + *
>> + * The caller must ensure that skb->len >= ETH_HLEN.
>> + *
>> + * Returns 0 if successful, otherwise a negative errno value.
>> + *
>> + * Initializes @skb header fields as follows:
>> + *
>> + *    - skb->mac_header: the L2 header.
>> + *
>> + *    - skb->network_header: just past the L2 header, or just past the
>> + *      VLAN header, to the first byte of the L2 payload.
>> + *
>> + *    - skb->transport_header: If key->eth.type is ETH_P_IP or ETH_P_IPV6
>> + *      on output, then just past the IP header, if one is present and
>> + *      of a correct length, otherwise the same as skb->network_header.
>> + *      For other key->eth.type values it is left untouched.
>> + *
>> + *    - skb->protocol: the type of the data starting at skb->network_header.
>> + *      Equals to key->eth.type.
>> + */
>> +static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)
>> +{
>> +	struct ethhdr *eth;
>> +
>> +	/* Flags are always used as part of stats */
>> +	key->tp.flags = 0;
>> +
>> +	skb_reset_mac_header(skb);
>> +
>> +	/* Link layer. */
>> +	clear_vlan(key);
>> +	if (ovs_key_mac_proto(key) == MAC_PROTO_NONE) {
>> +		if (unlikely(eth_type_vlan(skb->protocol)))
>> +			return -EINVAL;
>> +
>> +		skb_reset_network_header(skb);
>> +		key->eth.type = skb->protocol;
>> +	} else {
>> +		eth = eth_hdr(skb);
>> +		ether_addr_copy(key->eth.src, eth->h_source);
>> +		ether_addr_copy(key->eth.dst, eth->h_dest);
>> +
>> +		__skb_pull(skb, 2 * ETH_ALEN);
>> +		/* We are going to push all headers that we pull, so no need to
>> +		 * update skb->csum here.
>> +		 */
>> +
>> +		if (unlikely(parse_vlan(skb, key)))
>> +			return -ENOMEM;
>> +
>> +		key->eth.type = parse_ethertype(skb);
>> +		if (unlikely(key->eth.type == htons(0)))
>> +			return -ENOMEM;
>> +
>> +		/* Multiple tagged packets need to retain TPID to satisfy
>> +		 * skb_vlan_pop(), which will later shift the ethertype into
>> +		 * skb->protocol.
>> +		 */
>> +		if (key->eth.cvlan.tci & htons(VLAN_CFI_MASK))
>> +			skb->protocol = key->eth.cvlan.tpid;
>> +		else
>> +			skb->protocol = key->eth.type;
>> +
>> +		skb_reset_network_header(skb);
>> +		__skb_push(skb, skb->data - skb_mac_header(skb));
>> +	}
>> +
>> +	skb_reset_mac_len(skb);
>> +
>> +	/* Fill out L3/L4 key info, if any */
>> +	return key_extract_l3l4(skb, key);
>> +}
>> +
>> +/* In the case of conntrack fragment handling it expects L3 headers,
>> + * add a helper.
>> + */
>> +int ovs_flow_key_update_l3l4(struct sk_buff *skb, struct sw_flow_key *key)
>> +{
>> +	return key_extract_l3l4(skb, key);
>> +}
>> +
>> int ovs_flow_key_update(struct sk_buff *skb, struct sw_flow_key *key)
>> {
>> 	int res;
>> diff --git a/datapath/flow.h b/datapath/flow.h
>> index 07af912..78edb36 100644
>> --- a/datapath/flow.h
>> +++ b/datapath/flow.h
>> @@ -275,6 +275,7 @@ u64 ovs_flow_used_time(unsigned long flow_jiffies);
>>
>> /* Update the non-metadata part of the flow key using skb. */
>> int ovs_flow_key_update(struct sk_buff *skb, struct sw_flow_key *key);
>> +int ovs_flow_key_update_l3l4(struct sk_buff *skb, struct sw_flow_key *key);
>> int ovs_flow_key_extract(const struct ip_tunnel_info *tun_info,
>> 			 struct sk_buff *skb,
>> 			 struct sw_flow_key *key);
>> -- 
>> 1.8.3.1
>>



More information about the dev mailing list