Re: [PATCH V2 net 1/2] openvswitch: Properly set L4 keys on "later" IP fragments

[Gregory Rose <gvrose8192@gmail.com>]

On 8/26/2019 10:13 PM, Pravin Shelar wrote:
> On Mon, Aug 26, 2019 at 1:46 PM Greg Rose <gvrose8192@gmail.com> wrote:
>> 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@ovn.org>
>> Signed-off-by: Greg Rose <gvrose8192@gmail.com>
>> ---
>>   net/openvswitch/conntrack.c |   5 ++
>>   net/openvswitch/flow.c      | 161 ++++++++++++++++++++++++++------------------
>>   net/openvswitch/flow.h      |   1 +
>>   3 files changed, 101 insertions(+), 66 deletions(-)
>>
> ...
> ...
>> +/**
>> + * 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)
>> +{
>> +       int res;
>> +
>> +       res = key_extract_l3l4(skb, key);
>> +       if (!res)
>> +               key->mac_proto &= ~SW_FLOW_KEY_INVALID;
>> +
> Since this is not full key extract, this flag can not be unset.
>
> Otherwise looks good.

Thanks Pravin,

I'll send along a V3 patch set with that change.

- Greg