[ovs-dev] [PATCH 2/3] datapath: Avoid using wrong metadata for recic action.

Pravin Shelar pshelar at nicira.com
Wed Aug 6 22:48:00 UTC 2014


On Wed, Aug 6, 2014 at 3:04 PM, Andy Zhou <azhou at nicira.com> wrote:
> Looks good in general. Some minor comments inline.
>
> Acked-by: Andy Zhou <azhou at nicira.com>
>
> On Tue, Aug 5, 2014 at 4:46 PM, Pravin B Shelar <pshelar at nicira.com> wrote:
>> Recirc action needs to extract flow key from packet, it uses tun_info
>> from OVS_CB for setting tunnel meta data in flow key. But tun_info
>> can be overwritten by tunnel send action. This would result in wrong
>> flow key for the recirculation.
>> Following patch copies flow-key meta data from OVS_CB packet key
>> itself thus avoids this bug.
>>
>> Signed-off-by: Pravin B Shelar <pshelar at nicira.com>
>> ---
>>  datapath/actions.c      |    6 ++----
>>  datapath/flow.c         |   10 ++++++++++
>>  datapath/flow.h         |    9 +++++++++
>>  datapath/flow_netlink.c |    7 +------
>>  4 files changed, 22 insertions(+), 10 deletions(-)
>>
>> diff --git a/datapath/actions.c b/datapath/actions.c
>> index 5a0bfa9..6de65d3 100644
>> --- a/datapath/actions.c
>> +++ b/datapath/actions.c
>> @@ -649,17 +649,15 @@ static int execute_recirc(struct datapath *dp, struct sk_buff *skb,
>>                                  const struct nlattr *a)
>>  {
>>         struct sw_flow_key recirc_key;
>> -       uint32_t hash = OVS_CB(skb)->pkt_key->ovs_flow_hash;
>>         int err;
>>
>> -       err = ovs_flow_key_extract(skb, &recirc_key);
>> +       err = ovs_flow_key_extract_recirc(nla_get_u32(a), OVS_CB(skb)->pkt_key,
>> +                                         skb, &recirc_key);
>>         if (err) {
>>                 kfree_skb(skb);
>>                 return err;
>>         }
>>
>> -       recirc_key.ovs_flow_hash = hash;
>> -       recirc_key.recirc_id = nla_get_u32(a);
>>
>>         ovs_dp_process_packet_with_key(skb, &recirc_key, true);
>>
>> diff --git a/datapath/flow.c b/datapath/flow.c
>> index 7fdce6a..1feca85 100644
>> --- a/datapath/flow.c
>> +++ b/datapath/flow.c
>> @@ -718,3 +718,13 @@ int ovs_flow_key_extract_userspace(const struct nlattr *attr,
>>
>>         return key_extract(skb, key);
>>  }
>> +
>> +int ovs_flow_key_extract_recirc(u32 recirc_id,
>
> Could we make the recirc_id argument as const?
>
what is advantage of making recirc_is const.

>> +                               const struct sw_flow_key *key,
>> +                               struct sk_buff *skb,
>> +                               struct sw_flow_key *new_key)
>> +{
>> +       memcpy(new_key, key, OVS_SW_FLOW_KEY_METADATA_SIZE);
> It is a bit odd to make recirc_id 0 then overwrite it just one line
> below. May be we can memcpy sizeof (recirc_id) less bytes?

I am also using same macro to clear flow key meta data, so lets keep
it as it is.
I will push first two patches shortly.

>> +       new_key->recirc_id = recirc_id;
>> +       return key_extract(skb, new_key);
>> +}
>> diff --git a/datapath/flow.h b/datapath/flow.h
>> index 69664cd..ee63b8b 100644
>> --- a/datapath/flow.h
>> +++ b/datapath/flow.h
>> @@ -87,6 +87,11 @@ static inline void ovs_flow_tun_info_init(struct ovs_tunnel_info *tun_info,
>>         tun_info->options_len = opts_len;
>>  }
>>
>> +#define OVS_SW_FLOW_KEY_METADATA_SIZE                  \
>> +       (offsetof(struct sw_flow_key, recirc_id) +      \
>> +       FIELD_SIZEOF(struct sw_flow_key, recirc_id))
>> +
>> +
>>  struct sw_flow_key {
>>         u8 tun_opts[255];
>>         u8 tun_opts_len;
>> @@ -222,5 +227,9 @@ int ovs_flow_key_extract(struct sk_buff *skb, struct sw_flow_key *key);
>>  int ovs_flow_key_extract_userspace(const struct nlattr *attr,
>>                                    struct sk_buff *skb,
>>                                    struct sw_flow_key *key);
>> +int ovs_flow_key_extract_recirc(u32 recirc_id,
>
> same as above comment.
>> +                               const struct sw_flow_key *key,
>> +                               struct sk_buff *skb,
>> +                               struct sw_flow_key *new_key);
>>
>>  #endif /* flow.h */
>> diff --git a/datapath/flow_netlink.c b/datapath/flow_netlink.c
>> index ada21ae..75172de 100644
>> --- a/datapath/flow_netlink.c
>> +++ b/datapath/flow_netlink.c
>> @@ -1016,13 +1016,8 @@ int ovs_nla_get_flow_metadata(const struct nlattr *attr,
>>         memset(&match, 0, sizeof(match));
>>         match.key = key;
>>
>> -       key->tun_opts_len = 0;
>> -       memset(&key->tun_key, 0, sizeof(key->tun_key));
>> -       key->phy.priority = 0;
>> -       key->phy.skb_mark = 0;
>> +       memset(key, 0, OVS_SW_FLOW_KEY_METADATA_SIZE);
>>         key->phy.in_port = DP_MAX_PORTS;
>> -       key->ovs_flow_hash = 0;
>> -       key->recirc_id = 0;
>>
>>         return metadata_from_nlattrs(&match, &attrs, a, false);
>>  }
>> --
>> 1.7.1
>>
>> _______________________________________________
>> dev mailing list
>> dev at openvswitch.org
>> http://openvswitch.org/mailman/listinfo/dev



More information about the dev mailing list