| Message ID | 167103556314.309509.17490804498492906420.stgit@ebuild (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v2] openvswitch: Fix flow lookup to use unmasked key | expand |
On 12/14/22 17:33, 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. Hi, Eelco. Thanks for the fix! Could you, please, add more information to the commit message on why this is a problem, with some examples? This will be useful for someone in the future trying to understand why we actually have to use an unmasked key here. Also, I suppose, 'Cc: stable@vger.kernel.org' tag is needed in the commit message since it's a fix for a bug that is actually impacts users and needs to be backported. Best regards, Ilya Maximets. > > 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> > > --- > Version history: > - v2: Fixed ENOME(N/M) error. Forgot to do a stg refresh. > > net/openvswitch/datapath.c | 25 ++++++++++++++++--------- > 1 file changed, 16 insertions(+), 9 deletions(-)
On 15 Dec 2022, at 12:45, Ilya Maximets wrote: > On 12/14/22 17:33, 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. > > Hi, Eelco. Thanks for the fix! > > Could you, please, add more information to the commit message on > why this is a problem, with some examples? This will be useful > for someone in the future trying to understand why we actually > have to use an unmasked key here. ACK will add the same description as in the OVS dpif-netdev commit. > Also, I suppose, 'Cc: stable@vger.kernel.org' tag is needed in the > commit message since it's a fix for a bug that is actually impacts > users and needs to be backported. Will do in the v3. > Best regards, Ilya Maximets. > >> >> 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> >> >> --- >> Version history: >> - v2: Fixed ENOME(N/M) error. Forgot to do 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..55b697c4d576 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 = -ENOMEM; + 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; }
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> --- Version history: - v2: Fixed ENOME(N/M) error. Forgot to do a stg refresh. net/openvswitch/datapath.c | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-)