Re: [ovs-dev] [PATCH net] openvswitch: Fix flow lookup to use unmasked key

[Eelco Chaudron <echaudro@redhat.com>]

On 14 Dec 2022, at 16:14, Eelco Chaudron wrote:

> The commit mentioned below causes the ovs_flow_tbl_lookup() function
> to be called with the masked key. However, it's supposed to be called
> with the unmasked key.
>
> This change reverses the commit below, but rather than having the key
> on the stack, it's allocated.
>
> Fixes: 190aa3e77880 ("openvswitch: Fix Frame-size larger than 1024 bytes warning.")
>
> Signed-off-by: Eelco Chaudron <echaudro@redhat.com>

Please ignore this version, as I sent out out without doing a stg refresh :(

> ---
>  net/openvswitch/datapath.c |   25 ++++++++++++++++---------
>  1 file changed, 16 insertions(+), 9 deletions(-)
>
> diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
> index 861dfb8daf4a..23b233caa7fd 100644
> --- a/net/openvswitch/datapath.c
> +++ b/net/openvswitch/datapath.c
> @@ -948,6 +948,7 @@ static int ovs_flow_cmd_new(struct sk_buff *skb, struct genl_info *info)
>  	struct sw_flow_mask mask;
>  	struct sk_buff *reply;
>  	struct datapath *dp;
> +	struct sw_flow_key *key;
>  	struct sw_flow_actions *acts;
>  	struct sw_flow_match match;
>  	u32 ufid_flags = ovs_nla_get_ufid_flags(a[OVS_FLOW_ATTR_UFID_FLAGS]);
> @@ -975,24 +976,26 @@ static int ovs_flow_cmd_new(struct sk_buff *skb, struct genl_info *info)
>  	}
>
>  	/* Extract key. */
> -	ovs_match_init(&match, &new_flow->key, false, &mask);
> +	key = kzalloc(sizeof(*key), GFP_KERNEL);
> +	if (!key) {
> +		error = -ENOMEN;
> +		goto err_kfree_key;
> +	}
> +
> +	ovs_match_init(&match, key, false, &mask);
>  	error = ovs_nla_get_match(net, &match, a[OVS_FLOW_ATTR_KEY],
>  				  a[OVS_FLOW_ATTR_MASK], log);
>  	if (error)
>  		goto err_kfree_flow;
>
> +	ovs_flow_mask_key(&new_flow->key, key, true, &mask);
> +
>  	/* Extract flow identifier. */
>  	error = ovs_nla_get_identifier(&new_flow->id, a[OVS_FLOW_ATTR_UFID],
> -				       &new_flow->key, log);
> +				       key, log);
>  	if (error)
>  		goto err_kfree_flow;
>
> -	/* unmasked key is needed to match when ufid is not used. */
> -	if (ovs_identifier_is_key(&new_flow->id))
> -		match.key = new_flow->id.unmasked_key;
> -
> -	ovs_flow_mask_key(&new_flow->key, &new_flow->key, true, &mask);
> -
>  	/* Validate actions. */
>  	error = ovs_nla_copy_actions(net, a[OVS_FLOW_ATTR_ACTIONS],
>  				     &new_flow->key, &acts, log);
> @@ -1019,7 +1022,7 @@ static int ovs_flow_cmd_new(struct sk_buff *skb, struct genl_info *info)
>  	if (ovs_identifier_is_ufid(&new_flow->id))
>  		flow = ovs_flow_tbl_lookup_ufid(&dp->table, &new_flow->id);
>  	if (!flow)
> -		flow = ovs_flow_tbl_lookup(&dp->table, &new_flow->key);
> +		flow = ovs_flow_tbl_lookup(&dp->table, key);
>  	if (likely(!flow)) {
>  		rcu_assign_pointer(new_flow->sf_acts, acts);
>
> @@ -1089,6 +1092,8 @@ static int ovs_flow_cmd_new(struct sk_buff *skb, struct genl_info *info)
>
>  	if (reply)
>  		ovs_notify(&dp_flow_genl_family, reply, info);
> +
> +	kfree(key);
>  	return 0;
>
>  err_unlock_ovs:
> @@ -1098,6 +1103,8 @@ static int ovs_flow_cmd_new(struct sk_buff *skb, struct genl_info *info)
>  	ovs_nla_free_flow_actions(acts);
>  err_kfree_flow:
>  	ovs_flow_free(new_flow, false);
> +err_kfree_key:
> +	kfree(key);
>  error:
>  	return error;
>  }
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev