On Fri, Jul 31, 2026 at 9:43 PM Ilya Maximets <[email protected]> wrote:
>
> 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.
>
Thanks, I agree this is easier to read. I will send v4 with the
reallocation check split as suggested.
> > + 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
