Looks good in general. Some minor comments inline.
Acked-by: Andy Zhou <az...@nicira.com>
On Tue, Aug 5, 2014 at 4:46 PM, Pravin B Shelar <pshe...@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 <pshe...@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?
> + 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?
> + 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@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
