On Thu, Mar 20, 2014 at 4:37 AM, Lori Jakab <[email protected]> wrote:
> On 1/6/14, 7:55 PM, Jesse Gross wrote:
>> On Tue, Dec 24, 2013 at 9:02 AM, Lorand Jakab<[email protected]> wrote:
>>> diff --git a/datapath/actions.c b/datapath/actions.c
>>> index 30ea1d2..b90e715 100644
>>> --- a/datapath/actions.c
>>> +++ b/datapath/actions.c
>>> +static int pop_eth(struct sk_buff *skb)
>>> +{
>>> + skb_pull(skb, skb_network_offset(skb));
>>> + return 0;
>>> +}
>>
>> I think there needs to be some additional validation in this area. If
>> there's no Ethernet header then this will be a no-op but generally we
>> reject such actions at flow installation time rather than ignoring at
>> runtime. Furthermore, what does it mean for the validation of existing
>> L2 actions like set Ethernet or push vlan, which assume that an
>> Ethernet header is always present?
>>
>> Shouldn't we also update skb->csum?
>
>
> How about this incremental?
>
> diff --git a/datapath/actions.c b/datapath/actions.c
> index 802abce..adddadc 100644
> --- a/datapath/actions.c
> +++ b/datapath/actions.c
> @@ -145,6 +145,9 @@ static int set_eth_addr(struct sk_buff *skb,
>
>
> static int pop_eth(struct sk_buff *skb)
> {
> + if (skb->ip_summed == CHECKSUM_COMPLETE)
> + skb->csum = csum_sub(skb->csum, csum_partial(skb->data,
> + skb_network_offset(skb), 0));
> skb_pull(skb, skb_network_offset(skb));
I think you could just combine this whole thing into skb_pull_rcsum().
> diff --git a/datapath/flow_netlink.c b/datapath/flow_netlink.c
> index 7d193d8..8f0d5ab 100644
> --- a/datapath/flow_netlink.c
> +++ b/datapath/flow_netlink.c
> @@ -1289,6 +1289,14 @@ static int validate_tp_port(const struct sw_flow_key
> *flow_key)
> return -EINVAL;
> }
>
> +static int validate_eth_present(const struct sw_flow_key *flow_key)
> +{
> + if (flow_key->phy.noeth)
> + return -EINVAL;
I don't know if I would bother making this a separate function - it's
actually more code in each location and arguably less clear.
More importantly though, what happens if we have a pop followed by a
set action? I'm not sure that this will catch that.
>>> diff --git a/datapath/flow_netlink.c b/datapath/flow_netlink.c
>>> index 9b26528..8f71c49 100644
>>> --- a/datapath/flow_netlink.c
>>> +++ b/datapath/flow_netlink.c
>>> @@ -521,6 +522,8 @@ static int ovs_key_from_nlattrs(struct sw_flow_match
>>> *match, bool *exact_5tuple
>>> SW_FLOW_KEY_MEMCPY(match, eth.dst,
>>> eth_key->eth_dst, ETH_ALEN, is_mask);
>>> attrs &= ~(1ULL << OVS_KEY_ATTR_ETHERNET);
>>> + } else {
>>> + SW_FLOW_KEY_PUT(match, noeth, true, is_mask);
>>> }
>>
>> This macro sets not only the value but also ranges of the key that are
>> significant. Therefore, we should explicitly set the value in all
>> cases, rather than assuming the default is false.
>>
>> I think there's a potential for ambiguity here when megaflows are
>> used. If there is no Ethernet attribute does that mean that it must be
>> an L3 packet or does it mean that it is wildcarded as 'don-t care'? As
>> currently implemented, I believe that it would result in the former
>> but that would change behavior.
>
>
> How about the following instead:
>
>
> SW_FLOW_KEY_MEMCPY(match, eth.dst,
> eth_key->eth_dst, ETH_ALEN, is_mask);
> + SW_FLOW_KEY_PUT(match, phy.noeth, false, is_mask);
> attrs &= ~(1ULL << OVS_KEY_ATTR_ETHERNET);
> - } else {
> + } else if (!is_mask)
> SW_FLOW_KEY_PUT(match, phy.noeth, true, is_mask);
> - }
>
That looks good to me.
>>> +noethernet:
>>> if (nla_put_be16(skb, OVS_KEY_ATTR_ETHERTYPE, output->eth.type))
>>> goto nla_put_failure;
>>
>> Does it still make sense to send the EtherType? It's not present in
>> the packet and I believe that the information is contained in the
>> attributes that we do send (i.e. IPv4 or v6 attributes).
>
>
> We had a discussion about this in August last year:
>
>>>>>>> One such decision is how to handle the flow key. I set all fields in
>>>>>>> key->eth to 0, except the type, because we still need to know what
>>>>>>> kind
>>>>>>> of L3 packet do we have. Since a lot of code is accessing
>>>>>>> key->eth.type, this is easier than having this information in a
>>>>>>> different place, although it would be more elegant to set this field
>>>>>>> to
>>>>>>> 0 as well.
>>>>>>
>>>>>>
>>>>>>
>>>>>> Can we use skb->protocol for the cases where we currently access the
>>>>>> EtherType? Are there cases that you are particularly concerned about?
>>>>>
>>>>>
>>>>>
>>>>> The only case I'm concerned about is in the action validation code,
>>>>> particularly the validate_set() and validate_tp_port() functions, since
>>>>> they
>>>>> don't have access to the skb, and need to know the layer 3 protocol of
>>>>> the
>>>>> flow. In validate_set() we could relax the check for eth.type for
>>>>> OVS_KEY_ATTR_IPV4 and OVS_KEY_ATTR_IPV6, but I'm not sure what to do
>>>>> about
>>>>> validate_tp_port().
>>>>>
>>>>> In general, I think it would be a good idea for the flow key to have a
>>>>> field
>>>>> specifying the layer 3 protocol, when an ethernet header is not
>>>>> present.
>>>>
>>>>
>>>> That makes sense to me.
>>>
>>>
>>> You mean that we keep eth.type as the L3 protocol field, or define a new
>>> one, separate from the eth union?
>>
>> I think it's fine to keep using eth.type as the protocol field. I
>> think we can probably some holes to move the is_layer3 flag into the
>> non-tunnel portion of the flow though.
>
>
> Should we revisit this discussion?
I was just referring to the Netlink encoding here. Can we populate the
flow key in the kernel when we translate the flow?
>>> diff --git a/datapath/vport-lisp.c b/datapath/vport-lisp.c
>>> index c2698ae..b56eec7 100644
>>> --- a/datapath/vport-lisp.c
>>> +++ b/datapath/vport-lisp.c
>>
>> On the transmit path, we still assume that there might be an L2 header
>> and pull it off. Can we enforce a restriction that it must be an L3
>> packet at this point?
>
>
> Good point, we should definitely do that. I was thinking that we can use
> OVS_CB(skb)->pkt_key.phy.noeth in lisp_send() (and lisp_rcv()) for detecting
> packet type, is that ok?
>
>
>> Conversely, what about L2 ports that get packets
>> without an L2 header?
>
>
> That should not normally happen, as I replied to Ben in the user space part.
> However, in case it does, since it's unexpected behavior, should we drop?
Both of those sound fine.
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
