netdev - Re: [patch net-next v3 11/12] mlxsw: spectrum_router: Request a dump of FIB tables during init

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 30 Nov 2016 18:32:29 +0200
From:   Ido Schimmel <idosch@...sch.org>
To:     Hannes Frederic Sowa <hannes@...essinduktion.org>
Cc:     Jiri Pirko <jiri@...nulli.us>, netdev@...r.kernel.org,
        davem@...emloft.net, idosch@...lanox.com, eladr@...lanox.com,
        yotamg@...lanox.com, nogahf@...lanox.com, arkadis@...lanox.com,
        ogerlitz@...lanox.com, roopa@...ulusnetworks.com,
        dsa@...ulusnetworks.com, nikolay@...ulusnetworks.com,
        andy@...yhouse.net, vivien.didelot@...oirfairelinux.com,
        andrew@...n.ch, f.fainelli@...il.com, alexander.h.duyck@...el.com,
        kaber@...sh.net
Subject: Re: [patch net-next v3 11/12] mlxsw: spectrum_router: Request a dump
 of FIB tables during init

Hi Hannes,

On Wed, Nov 30, 2016 at 04:37:48PM +0100, Hannes Frederic Sowa wrote:
> On 30.11.2016 11:09, Jiri Pirko wrote:
> > From: Ido Schimmel <idosch@...lanox.com>
> > 
> > Make sure the device has a complete view of the FIB tables by invoking
> > their dump during module init.
> > 
> > Signed-off-by: Ido Schimmel <idosch@...lanox.com>
> > Signed-off-by: Jiri Pirko <jiri@...lanox.com>
> > ---
> >  .../net/ethernet/mellanox/mlxsw/spectrum_router.c  | 23 ++++++++++++++++++++++
> >  1 file changed, 23 insertions(+)
> > 
> > diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
> > index 14bed1d..d176047 100644
> > --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
> > +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
> > @@ -2027,8 +2027,23 @@ static int mlxsw_sp_router_fib_event(struct notifier_block *nb,
> >  	return NOTIFY_DONE;
> >  }
> >  
> > +static void mlxsw_sp_router_fib_dump_flush(struct notifier_block *nb)
> > +{
> > +	struct mlxsw_sp *mlxsw_sp = container_of(nb, struct mlxsw_sp, fib_nb);
> > +
> > +	/* Flush pending FIB notifications and then flush the device's
> > +	 * table before requesting another dump. Do that with RTNL held,
> > +	 * as FIB notification block is already registered.
> > +	 */
> > +	mlxsw_core_flush_owq();
> > +	rtnl_lock();
> > +	mlxsw_sp_router_fib_flush(mlxsw_sp);
> > +	rtnl_unlock();
> > +}
> > +
> >  int mlxsw_sp_router_init(struct mlxsw_sp *mlxsw_sp)
> >  {
> > +	fib_dump_cb_t *cb = mlxsw_sp_router_fib_dump_flush;
> >  	int err;
> >  
> >  	INIT_LIST_HEAD(&mlxsw_sp->router.nexthop_neighs_list);
> > @@ -2048,8 +2063,16 @@ int mlxsw_sp_router_init(struct mlxsw_sp *mlxsw_sp)
> >  
> >  	mlxsw_sp->fib_nb.notifier_call = mlxsw_sp_router_fib_event;
> >  	register_fib_notifier(&mlxsw_sp->fib_nb);
> 
> Sorry to pick in here again:
> 
> There is a race here. You need to protect the registration of the fib
> notifier as well by the sequence counter. Updates here are not ordered
> in relation to this code below.

You mean updates that can be received after you registered the notifier
and until the dump started? I'm aware of that and that's OK. This
listener should be able to handle duplicates.

I've a follow up patchset that introduces a new event in switchdev
notification chain called SWITCHDEV_SYNC, which is sent when port
netdevs are enslaved / released  from a master device (points in time
where kernel<->device can get out of sync). It will invoke
re-propagation of configuration from different parts of the stack
(e.g. bridge driver, 8021q driver, fib/neigh code), which can result
in duplicates.

> I think just move the register notification into the fib_notifier_dump
> function, rename it to fib_notifier_init and use it here:

I separated the two on purpose. For example, rocker only needs to
register notifier, but doesn't need the dump.

> 
> > +	if (!fib_notifier_dump(&mlxsw_sp->fib_nb, &init_net, cb)) {
> > +		err = -EBUSY;
> > +		goto err_fib_notifier_dump;
> > +	}
> > +
> >  	return 0;
> 
> Thanks,
> Hannes
>