On 11/15/22 17:16, Eelco Chaudron wrote:
> Hi Pravin,
>
> It looks like a previous fix you made, 190aa3e77880 ("openvswitch: Fix
> Frame-size larger than 1024 bytes warning."), is breaking stuff. With this
> change, the actual flow lookup, ovs_flow_tbl_lookup(), is done using a masked
> key, where it should be an unmasked key. This is maybe more clear if you take
> a look at the diff for the ufid addition, 74ed7ab9264c ("openvswitch: Add
> support for unique flow IDs.").
>
> Just reverting the change gets rid of the problem, but it will re-introduce
> the larger stack size. It looks like we either have it on the stack or
> dynamically allocate it each time. Let me know if you have any other clever
> fix ;)
I'd say that dynamic allocation should be fine.
We do alloc other things in the same function, and
I don't immediately see another simple way to fix
the problem without heavily re-working the logic.
My 2c.
Best regards, Ilya Maximets.
>
> We found this after debugging some customer-specific issue. More details are
> in the following OVS patch,
> https://patchwork.ozlabs.org/project/openvswitch/list/?series=328315
>
> Cheers,
>
> Eelco
>
>
> FYI the working revers:
>
>
> Revert "openvswitch: Fix Frame-size larger than 1024 bytes warning."
>
> This reverts commit 190aa3e77880a05332ea1ccb382a51285d57adb5.
>
> diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
> index 861dfb8daf4a..660d5fdd9b28 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,20 @@ 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);
> + ovs_match_init(&match, &key, true, &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 +1016,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);
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
