Ilya Maximets <[email protected]> writes: > 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.
+1 > 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 [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
