netdev - Re: [patch net-next 10/10] mlxsw: spectrum_router: Don't abort on l3mdev rules

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]

Message-ID: <20170313152225.GA8034@splinter.mtl.com>
Date:   Mon, 13 Mar 2017 17:22:25 +0200
From:   Ido Schimmel <idosch@...sch.org>
To:     David Ahern <dsa@...ulusnetworks.com>
Cc:     Jiri Pirko <jiri@...nulli.us>, netdev@...r.kernel.org,
        davem@...emloft.net, idosch@...lanox.com, mlxsw@...lanox.com,
        shm@...ulusnetworks.com, kuznet@....inr.ac.ru, jmorris@...ei.org,
        yoshfuji@...ux-ipv6.org, kaber@...sh.net, lorenzo@...gle.com,
        mateusz.bajorski@...ia.com
Subject: Re: [patch net-next 10/10] mlxsw: spectrum_router: Don't abort on
 l3mdev rules

On Mon, Mar 13, 2017 at 08:59:11AM -0600, David Ahern wrote:
> On 3/13/17 1:38 AM, Jiri Pirko wrote:
> > From: Ido Schimmel <idosch@...lanox.com>
> > 
> > Now that port netdevs can be enslaved to a VRF master we need to make
> > sure the device's routing tables won't be flushed upon the insertion of
> > a l3mdev rule.
> > 
> > Note that we assume the notified l3mdev rule is a simple rule as used by
> > the VRF master. We don't check for the presence of other selectors such
> > as 'iif' and 'oif'.
> > 
> > Signed-off-by: Ido Schimmel <idosch@...lanox.com>
> > Signed-off-by: Jiri Pirko <jiri@...lanox.com>
> > ---
> >  drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
> > index de54382..fa73ee2 100644
> > --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
> > +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
> > @@ -2552,7 +2552,8 @@ static void mlxsw_sp_router_fib_event_work(struct work_struct *work)
> >  		break;
> >  	case FIB_EVENT_RULE_ADD: /* fall through */
> >  	case FIB_EVENT_RULE_DEL:
> > -		if (!fib_work->fr_info.rule->def)
> > +		if (!fib_work->fr_info.rule->def &&
> > +		    !fib_work->fr_info.rule->l3mdev)
> >  			mlxsw_sp_router_fib4_abort(mlxsw_sp);
> >  		fib_rule_put(fib_work->fr_info.rule);
> >  		break;
> > 
> 
> You do not want to abort if the default rules are re-ordered. For
> example, the rule for the local table is moved from priority 0 to just
> before the main. ie., from this order:

Are you aware of configurations employing the VRF device and leaving the
rule for the local table at priority 0?

> 
> $ ip ru ls
> 0:	from all lookup local
> 32766:	from all lookup main
> 32767:	from all lookup default
> 
> to this order
> $ ip ru ls
> 1000:	from all lookup [l3mdev-table]
> 32765:	from all lookup local
> 32766:	from all lookup main
> 32767:	from all lookup default
> 
> should not abort offloads.
> 
> The default flag added to rules in patch 1 is not really needed; you can
> key off basic references to hardcoded table ids: RT_TABLE_MAIN and
> RT_TABLE_LOCAL are set in stone. Simple rules (from all lookup X) where
> only the priority changes should be enough information to not abort.

OK. Will look into that. I didn't like having the local rule first, so
this might the solution.

> In addition, if rules are added that reference oif or iif that is not a
> port netdev for this h/w then why abort offloads? e.g, consider PBR
> rules related to the management interface.

I'm aware, but not sure this belongs in this patchset. I prefer to deal
with the l3mdev rule and re-ordering of the default rules first, then
move to more advanced use cases.

Thanks David.