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

[Aaron Conole <aconole@redhat.com>]

Ilya Maximets <i.maximets@ovn.org> 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);
>>