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? 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
