Re: [PATCH RFC net-next 07/10] net: dsa: mv88e6xxx: Track bridge mdb objects

[Joseph Huang]

On 4/30/2024 12:27 PM, Joseph Huang wrote:
> On 4/29/2024 8:59 PM, Vladimir Oltean wrote:
> > On Mon, Apr 29, 2024 at 06:07:25PM -0400, Joseph Huang wrote:
> > > Something like this (some layers omitted for brevity)?
> > >
> > >                                        +br_iterator
> > >                                        |  for each mdb
> > >                                        |    _br_switchdev_mdb_notify
> > >  rtnl_lock                             |      without F_DEFER flag
> > >   |                                    |      |
> > >   +switchdev_port_attr_set_deferred    |      +switchdev_port_obj_notify
> > >     |                                  |        |
> > >     +dsa_port_mrouter                  |        +dsa_user_port_obj_a/d
> > >       |                                |          |
> > >       +mv88e6xxx_port_mrouter----------+          
> > > +mv88e6xxx_port_obj_a/d
> > >                                          |
> > >   +--------------------------------------+
> > >   |
> > > rtnl_unlock
> >
> > At a _very_ superficial glance, I don't think you are properly
> > accounting for the fact that even with rtnl_lock() held, there are still
> > SWITCHDEV_OBJ_ID_PORT_MDB events which may be pending on the switchdev
> > chain. Without a switchdev_deferred_process() flush call, you won't be
> > getting rid of them, so when you rtnl_unlock(), they will still run.
> >
> > Even worse, holding rtnl_lock() will not stop the bridge multicast layer
> > from modifying its br->mdb_list; only br->multicast_lock will.
> >
> > So you may be better off also acquiring br->multicast_lock, and
> > notifying the MDB entries to the switchdev chain _with_the F_DEFER flag.
>
> Like this?
>
>                                        +br_iterator(dsa_cb)
>                                        |  lock br->multicask_lock
>                                        |  for each mdb
>                                        |    br_switchdev_mdb_notify
>  rtnl_lock                             |      |
>   |                                    |      +switchdev_port_obj_._defer
>   +switchdev_port_attr_set_deferred    |  unlock br->multicast_lock
>     |                                  |
>     +dsa_port_mrouter                  |
>       |                                |
>       +mv88e6xxx_port_mrouter----------+
>                                          |
>   +--------------------------------------+
>   |
> rtnl_unlock
>
> (potential task change)
>
> rtnl_lock
>   |
>   +switchdev_deferred_process
>   | flush all queued dfitems in queuing order
>   |
> rtnl_unlock
>
> I'm not that familiar with the bridge code, but is there any concern 
> with potential deadlock here (between rtnl_lock and br->multicast_lock)?

Hi Nik,

Do you know if it's safe to acquire rtnl_lock and br->multicast_lock in 
the following sequence? Is there any potential possibility for a deadlock?

rtnl_lock
spin_lock(br->multicast_lock)
spin_unlock(br->multicast_lock)
rtnl_unlock

If the operation is safe, the next question is should it be spin_lock or 
spin_lock_bh?

Thanks,
Joseph