On 10/26/22 07:17, Ales Musil wrote: > > > On Tue, Oct 25, 2022 at 9:52 PM Ilya Maximets <[email protected] > <mailto:[email protected]>> wrote: > > On 9/6/22 15:10, Ales Musil wrote: > > The iface is removed from the "ifaces" hmap > > only if the ofp_port != OFPP_NONE (iface_destroy__). > > However, during the creation all ports are inserted > > regardless the ofp_port. > > > > This can later on lead to use-after-free > > in "iface_from_ofp_port". To prevent that match > > the removal function and add only ports that > > have ofp_port != OFPP_NONE. > > > > Reported-by: Dumitru Ceara <[email protected] > <mailto:[email protected]>> > > Signed-off-by: Ales Musil <[email protected] <mailto:[email protected]>> > > --- > > v2: Rebase on top of current master. > > Add more descriptive commit message. > > --- > > vswitchd/bridge.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > Hi, Ales. Sorry for the late reply. > > > diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c > > index 25ce45e3d..9e089f268 100644 > > --- a/vswitchd/bridge.c > > +++ b/vswitchd/bridge.c > > @@ -2127,8 +2127,10 @@ iface_create(struct bridge *br, const struct > ovsrec_interface *iface_cfg, > > iface->netdev = netdev; > > iface->type = iface_get_type(iface_cfg, br->cfg); > > iface->cfg = iface_cfg; > > - hmap_insert(&br->ifaces, &iface->ofp_port_node, > > - hash_ofp_port(ofp_port)); > > + if (iface->ofp_port != OFPP_NONE) { > > How is that possible? Reading the code and comments, > iface_do_create() supposed to set the ofp_port to some > meaningful value on success and we should not be able > to reach this part of the code otherwise. > > If this is possible, then we likely have a bug somewhere > in the interface creation code. > > Or am I missing something? > > Do you have an example or a test case that can trigger > this condition? > > Best regards, Ilya Maximets. > > > This showed up a long time ago during a regular OvS test, I have not seen it > since then. > Unfortunately there is no reliable way to trigger it.
I marked this change as 'deferred' for now since the condition should not be possible in practice. We can return to the discussion once we have more data. Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
