On 8/1/26 1:25 PM, Zhiling Zou wrote:
> 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 if the
> matched flow needs a larger reply than the request identifier allowed. 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: [email protected]
> Reported-by: Vega <[email protected]>
> Signed-off-by: Zhiling Zou <[email protected]>
> ---
> changes in v4:
> - Split the reply size comparison into current and desired variables as
> requested by Ilya Maximets.
> - v3 Link:
> https://lore.kernel.org/all/c773b2726dda8d90eed6d42b2f9741810a7a46e1.1785288876.git.zhil...@nebusec.ai/
>
> changes in v3:
> - Compare computed reply sizes instead of UFID contents.
> - Inline the rare reallocation check and add spacing around the update checks.
> - v2 Link:
> https://lore.kernel.org/all/71380bcfbf3aed9a6a8a9daeef6592fa5a4fb245.1785211788.git.zhil...@nebusec.ai/
>
> 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.zhil...@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..d73d729176e1f 100644
> --- a/net/openvswitch/datapath.c
> +++ b/net/openvswitch/datapath.c
> @@ -1113,9 +1113,8 @@ 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 +1126,29 @@ static int ovs_flow_cmd_new(struct sk_buff *skb,
> struct genl_info *info)
> goto err_unlock_ovs;
> }
> }
> +
> + if (unlikely(reply)) {
> + size_t current, desired;
Unfortunately, this breaks the build. Please, make sure the code
compiles without warnings before posting patches, don't just copy
suggestions.
Let's rename these into 'cur' and 'req' (required).
> +
> + current = ovs_flow_cmd_msg_size(acts, &new_flow->id,
> + ufid_flags);
> + desired = ovs_flow_cmd_msg_size(acts, &flow->id,
> + ufid_flags);
> + if (current < desired) {
> + struct sk_buff *resized;
> +
> + resized = ovs_flow_cmd_alloc_info(acts,
> &flow->id,
This line is too long. The line length check for netdev patches
is 80. Move the id to the next line.
> + info, false,
> + ufid_flags);
> + if (IS_ERR(resized)) {
> + error = PTR_ERR(resized);
> + goto err_unlock_ovs;
> + }
> + kfree_skb(reply);
> + reply = resized;
> + }
> + }
> +
> /* Update actions. */
> old_acts = ovsl_dereference(flow->sf_acts);
> rcu_assign_pointer(flow->sf_acts, acts);
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
