Patch "openvswitch: Fix Frame-size larger than 1024 bytes warning" not correct.

* Patch "openvswitch: Fix Frame-size larger than 1024 bytes warning" not correct.
@ 2022-11-15 16:16 Eelco Chaudron
  2022-11-23 10:56 ` Eelco Chaudron
  2022-11-25 14:53 ` Ilya Maximets
  0 siblings, 2 replies; 4+ messages in thread
From: Eelco Chaudron @ 2022-11-15 16:16 UTC (permalink / raw)
  To: Pravin B Shelar, netdev; +Cc: Ilya Maximets, Aaron Conole, dev

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 ;)

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);


^ permalink raw reply related	[flat|nested] 4+ messages in thread