On 7/29/26 3:53 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 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 | 24 +++++++++++++++++++++---
> 1 file changed, 21 insertions(+), 3 deletions(-)
>
> diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
> index eaf332b156d73..3007d75b42145 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,25 @@ static int ovs_flow_cmd_new(struct sk_buff *skb,
> struct genl_info *info)
> goto err_unlock_ovs;
> }
> }
> +
> + if (unlikely(reply &&
> + ovs_flow_cmd_msg_size(acts, &new_flow->id,
> + ufid_flags) <
> + ovs_flow_cmd_msg_size(acts, &flow->id,
> + ufid_flags))) {
This version seems fine too me code-wise, but this construct is hard to
read. There isn't a particularly good way to format this, but maybe
something like the following will be easier on the eyes:
if (unlikely(reply)) {
size_t current, desired;
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,
info, false,
ufid_flags);
if (IS_ERR(resized)) {
error = PTR_ERR(resized);
goto err_unlock_ovs;
}
kfree_skb(reply);
reply = resized;
}
}
Best regards, Ilya Maximets.
> + 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);
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
