ovs_flow_cmd_new() preallocates the optional reply skb before it takes ovs_mutex and before it knows which existing flow will be updated. That is normally fine because the skb is sized from the request flow identifier. That identifier also becomes the inserted flow's identifier. For updates, however, a request with a UFID may miss the UFID lookup and then fall back to the flow key lookup. That lookup can legitimately find an existing key-identified flow. UFIDs are optional and the flow key is the primary identifier. For echoed replies, ovs_flow_cmd_fill_info() writes the matched flow's identifier, not the request identifier used for the preallocation. A short request UFID can therefore leave too little room for the key identifier. The fill can then fail with -EMSGSIZE and hit the BUG_ON(error < 0) in the update path. Once the update target has been resolved, reallocate the reply skb with the matched flow's identifier when it differs from the request identifier. Do this before replacing the actions so the request can still fail cleanly if the rare extra allocation fails. Fixes: 74ed7ab9264c ("openvswitch: Add support for unique flow IDs.") Cc: stable@vger.kernel.org Reported-by: Vega Signed-off-by: Zhiling Zou --- changes in v2: - Preserve valid key/UFID mixed flow updates as requested by Ilya Maximets. - Reallocate the echoed reply skb with the matched flow identifier before replacing actions. - v1 Link: https://lore.kernel.org/all/fa4f85fe7becb164a8a1849aa77ceeb1c08b078c.1784881178.git.zhilinz@nebusec.ai/ net/openvswitch/datapath.c | 28 +++++++++++++++++++++++++--- 1 file changed, 25 insertions(+), 3 deletions(-) diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c index eaf332b156d73..18732337ea5d1 100644 --- a/net/openvswitch/datapath.c +++ b/net/openvswitch/datapath.c @@ -800,6 +800,16 @@ static bool should_fill_actions(uint32_t ufid_flags) return !(ufid_flags & OVS_UFID_F_OMIT_ACTIONS); } +static bool ovs_flow_id_mismatch(const struct sw_flow_id *a, + const struct sw_flow_id *b) +{ + if (a->ufid_len != b->ufid_len) + return true; + + return ovs_identifier_is_ufid(a) && + !!memcmp(a->ufid, b->ufid, a->ufid_len); +} + static size_t ovs_flow_cmd_msg_size(const struct sw_flow_actions *acts, const struct sw_flow_id *sfid, uint32_t ufid_flags) @@ -1113,9 +1123,7 @@ static int ovs_flow_cmd_new(struct sk_buff *skb, struct genl_info *info) error = -EEXIST; goto err_unlock_ovs; } - /* The flow identifier has to be the same for flow updates. - * Look for any overlapping flow. - */ + /* Look for any overlapping flow. */ if (unlikely(!ovs_flow_cmp(flow, &match))) { if (ovs_identifier_is_key(&flow->id)) flow = ovs_flow_tbl_lookup_exact(&dp->table, @@ -1127,6 +1135,20 @@ static int ovs_flow_cmd_new(struct sk_buff *skb, struct genl_info *info) goto err_unlock_ovs; } } + if (unlikely(reply && + ovs_flow_id_mismatch(&new_flow->id, &flow->id))) { + struct sk_buff *new_reply; + + new_reply = ovs_flow_cmd_alloc_info(acts, &flow->id, + info, false, + ufid_flags); + if (IS_ERR(new_reply)) { + error = PTR_ERR(new_reply); + goto err_unlock_ovs; + } + kfree_skb(reply); + reply = new_reply; + } /* Update actions. */ old_acts = ovsl_dereference(flow->sf_acts); rcu_assign_pointer(flow->sf_acts, acts); -- 2.43.0