On Mon, Dec 15, 2025 at 5:05 AM Dumitru Ceara <[email protected]> wrote:
>
> On 11/10/25 5:05 PM, Numan Siddique wrote:
> > On Fri, Nov 7, 2025 at 1:48 PM Mark Michelson <[email protected]> wrote:
> >>
> >> On Thu, Nov 6, 2025 at 8:44 PM Han Zhou <[email protected]> wrote:
> >>>
> >>> On Thu, Nov 6, 2025 at 8:45 AM Numan Siddique <[email protected]> wrote:
> >>>>
> >>>> On Wed, Nov 5, 2025 at 7:28 PM Han Zhou <[email protected]> wrote:
> >>>>>
> >>>>>
> >>>>>
> >>>>> On Mon, Nov 3, 2025 at 6:17 PM <[email protected]> wrote:
> >>>>>>
> >>>>>> From: Numan Siddique <[email protected]>
> >>>>>>
> >>>>>> This patch series adds a new service OVN Bridge Controller
> >>>>>> (ovn-br-controller) which can be used to program OpenFlow rules to
> >>>>>> OVS bridges using OVN logical flows.
> >>>>>>
> >>>>>> The syntax and symantics of the logical flows is similar to the
> >>> logical
> >>>>>> flows generated by ovn-northd.
> >>>>>>
> >>>>>> Rational for adding this service:
> >>>>>> There's a need to configure the provider bridges with specific
> >>> OpenFlow
> >>>>>> rules after packets leave the OVN pipeline and enters these
> >>> provider
> >>>>>> bridges via the patch ports. Since OVN has a very good
> >>> abstraction
> >>>>>> of the OpenFlow rules using OVN logical flows, we can leverage
> >>> the same
> >>>>>> here so that CMS doesn't have to deal with the specifics of
> >>> OpenFlow
> >>>>>> protocol. Also if the CMS is written in golang or other
> >>> languages,
> >>>>>> CMS has to mostly rely on ovs-vsctl/ovs-ofctl to program the
> >>> flows.
> >>>>>> Adding this support in OVN avoids the CMS to exec these utils to
> >>>>>> add/delete and dump the existing OpenFlow rules.
> >>>>>>
> >>>>>> A user can also use this new service to program and manage any OVS
> >>>>>> bridge without using OVN and hence this service is named as
> >>>>>> "ovn-br-controller."
> >>>>>>
> >>>>>
> >>>>> Thanks Numan for this nice feature! I think it is very well structured,
> >>> and mostly looks good to me. I have sent my feedback and comments for some
> >>> of the patches. Feel free to add my Ack for the series when those are
> >>> addressed.
> >>>>
> >>>> Thanks for your comments Han. I'll address your comments for the
> >>>> patches and will add your Ack to the series.
> >>>>
> >>>> At this point, it's a bit hard for me to address the comments on
> >>>> moving the common code in ofctrl.c and br-ofctrl.c to a lib file.
> >>>> I'll address this as a follow up. Hope that's fine.
> >>>
> >>> Yes, I think this is fine.
> >>>
> >>> I replied with one more comment for patch 4. PTAL.
> >>>
> >>> Best,
> >>> Han
> >>
> >> I had another look through the code, I agree with Han that we should
> >> add TODOs to reduce code duplication. However, I think that with this
> >> feature being experimental, it's actually safer to have code that is
> >> separate and duplicated, just to ensure that there are no unintended
> >> side effects on "standard" br-int bridge programming. I know that the
> >> testsuite *should* catch these sorts of things, but we all know that
> >> the test coverage isn't 100%.
> >>
> >> I also mentioned in an earlier review that since all actions are
> >> allowed by br-controller, we should have a TODO to restrict the
> >> actions usable by ovn-br-controller. Again, since this is
> >> experimental, that can be done later.
> >>
> >> I had one minor nit for patch 7, but other than that,
> >>
> >> Acked-by: Mark Michelson <[email protected]>
> >
> >
> > Thank you Mark and Han for the reviews. I applied this series to the
> > main branch addressing the review comments.
> >
>
> Hi Numan,
>
> Coverity is complaining about potential NULL dereferences in
> ovnacts_encode() due to ep.meter_table, ep.group_table
> and ep.tunnel_ofport being NULL when ovnacts_encode()
> is called from br-controller/en-lflow.c:
>
> https://github.com/ovn-org/ovn/blob/5220faeb/br-controller/en-lflow.c#L336
>
> E.g.:
>
> 325 struct ovnact_encode_params ep = {
> 326 .lookup_port = lookup_port_cb,
> 327 .aux = &aux,
> 328 .is_switch = true,
> 329 .lflow_uuid = lflow->header_.uuid,
> 330
> 331 .pipeline = OVNACT_P_INGRESS,
> 332 .ingress_ptable = BR_OFTABLE_LOG_INGRESS_PIPELINE,
> 333 .egress_ptable = 0,
> 334 .output_ptable = output_ptable,
> 335 };
>
> CID 493668: (#1 of 1): Explicit null dereferenced (FORWARD_NULL)
> 2. var_deref_model: Passing &ep to ovnacts_encode, which dereferences null
> ep.meter_table.[show details]
> CID 493667:Explicit null dereferenced (FORWARD_NULL) [ "select issue" ]
> CID 493665:Explicit null dereferenced (FORWARD_NULL) [ "select issue" ]
>
>
> While I understand that's probably not going to happen in
> practice because of the logical flows that can be added on
> the br-controller it might be nice to add some defensive
> checks in the encode functions to make sure we never
> dereference NULL in those cases.
>
> Would you have time to prepare a patch by any chance?
Hi Dumitru,
I'll take a look into it.
Thanks
Numan
>
> Thanks,
> Dumitru
>
> > In patch 7 while adding physical flows to the OVS ports in the
> > provider bridges, I had missed adding a flow to
> > clear in_port for hairpin traffic i.e both inport and outport the same.
> >
> > This could happen if CMS adds ARP responder/icmp response logical
> > flows and sets "outport = inport; output;".
> >
> > So I also added that. Below are the changes
> >
> > -------------------------
> > diff --git a/br-controller/en-pflow.c b/br-controller/en-pflow.c
> > index 5f4c5b914f..be4bfafaf4 100644
> > --- a/br-controller/en-pflow.c
> > +++ b/br-controller/en-pflow.c
> > @@ -48,6 +48,7 @@ static void physical_flows_add(const struct ovn_bridge *);
> > static void put_load(uint64_t value, enum mf_field_id dst, int ofs, int
> > n_bits,
> > struct ofpbuf *);
> > static void put_resubmit(uint8_t table_id, struct ofpbuf *);
> > +static void put_stack(enum mf_field_id, struct ofpact_stack *);
> >
> > static void add_interface_flows(const char *bridge,
> > const struct ovsrec_interface *iface_rec,
> > @@ -98,7 +99,7 @@ physical_flows_add(const struct ovn_bridge *br)
> >
> > struct match match = MATCH_CATCHALL_INITIALIZER;
> >
> > - /* Table 0 and 65, priority 0, actions=NORMAL
> > + /* Table 0 and 121, priority 0, actions=NORMAL
> > * ===================================
> > *
> > */
> > @@ -110,6 +111,7 @@ physical_flows_add(const struct ovn_bridge *br)
> > br->ovs_br->header_.uuid.parts[0],
> > &match, &ofpacts,
> > &br->ovs_br->header_.uuid);
> >
> > + /* Priority-0 action to advance the packet from table 120 to 121. */
> > ofpbuf_clear(&ofpacts);
> > put_resubmit(BR_OFTABLE_LOG_TO_PHY, &ofpacts);
> > br_flow_add_physical_oflow(br->db_br->name, BR_OFTABLE_SAVE_INPORT, 0,
> > @@ -156,6 +158,14 @@ put_resubmit(uint8_t table_id, struct ofpbuf *ofpacts)
> > resubmit->table_id = table_id;
> > }
> >
> > +static void
> > +put_stack(enum mf_field_id field, struct ofpact_stack *stack)
> > +{
> > + stack->subfield.field = mf_from_id(field);
> > + stack->subfield.ofs = 0;
> > + stack->subfield.n_bits = stack->subfield.field->n_bits;
> > +}
> > +
> > static void
> > add_interface_flows(const char *bridge,
> > const struct ovsrec_interface *iface_rec,
> > @@ -189,4 +199,23 @@ add_interface_flows(const char *bridge,
> > iface_rec->header_.uuid.parts[0],
> > &match, ofpacts_p,
> > &iface_rec->header_.uuid);
> > +
> > + /* Priority-100 flow in table BR_OFTABLE_SAVE_INPORT to match on
> > + * both inport and outport to the interface's ofport number and
> > + * clear the in_port to OFPP_NONE before advancing the packet to
> > + * the next table - BR_OFTABLE_LOG_TO_PHY.
> > + * This is required for the hairpinned packets.
> > + */
> > + match_init_catchall(&match);
> > + match_set_reg(&match, MFF_LOG_INPORT - MFF_REG0, ofport);
> > + match_set_reg(&match, MFF_LOG_OUTPORT - MFF_REG0, ofport);
> > +
> > + ofpbuf_clear(ofpacts_p);
> > + put_stack(MFF_IN_PORT, ofpact_put_STACK_PUSH(ofpacts_p));
> > + put_load(ofp_to_u16(OFPP_NONE), MFF_IN_PORT, 0, 16, ofpacts_p);
> > + put_resubmit(BR_OFTABLE_LOG_TO_PHY, ofpacts_p);
> > + put_stack(MFF_IN_PORT, ofpact_put_STACK_POP(ofpacts_p));
> > + br_flow_add_physical_oflow(bridge, BR_OFTABLE_SAVE_INPORT, 100,
> > + iface_rec->header_.uuid.parts[0],
> > + &match, ofpacts_p,
> > &iface_rec->header_.uuid);
> > }
> > ------------------------------------
> >
> >
> > Thanks
> > Numan
> >
> >
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
