netdev - Re: [RFC PATCH net-next 02/15] ipv4: Notify route after insertion to the routing table

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: <20191003051326.GA4325@splinter>
Date:   Thu, 3 Oct 2019 08:16:30 +0300
From:   Ido Schimmel <idosch@...sch.org>
To:     David Ahern <dsahern@...il.com>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, jiri@...lanox.com,
        jakub.kicinski@...ronome.com, saeedm@...lanox.com,
        mlxsw@...lanox.com, Ido Schimmel <idosch@...lanox.com>
Subject: Re: [RFC PATCH net-next 02/15] ipv4: Notify route after insertion to
 the routing table

On Wed, Oct 02, 2019 at 07:34:35PM -0600, David Ahern wrote:
> On 10/2/19 2:40 AM, Ido Schimmel wrote:
> > @@ -1269,14 +1269,19 @@ int fib_table_insert(struct net *net, struct fib_table *tb,
> >  	new_fa->tb_id = tb->tb_id;
> >  	new_fa->fa_default = -1;
> >  
> > -	err = call_fib_entry_notifiers(net, event, key, plen, new_fa, extack);
> > +	/* Insert new entry to the list. */
> > +	err = fib_insert_alias(t, tp, l, new_fa, fa, key);
> >  	if (err)
> >  		goto out_free_new_fa;
> >  
> > -	/* Insert new entry to the list. */
> > -	err = fib_insert_alias(t, tp, l, new_fa, fa, key);
> > +	/* The alias was already inserted, so the node must exist. */
> > +	l = fib_find_node(t, &tp, key);
> > +	if (WARN_ON_ONCE(!l))
> > +		goto out_free_new_fa;
> 
> Maybe I am missing something but, the 'l' is only needed for the error
> path, so optimize for the success case and only lookup the node if the
> notifier fails.

Hi David,

You're correct that in this patch it is only needed by the error path, but
later on in patch 4 ("ipv4: Notify newly added route if should be
offloaded") we actually need it here as well. The FIB node can be NULL
and fib_insert_alias() will create one if that's the case.

> 
> > +
> > +	err = call_fib_entry_notifiers(net, event, key, plen, new_fa, extack);
> >  	if (err)
> > -		goto out_fib_notif;
> > +		goto out_remove_new_fa;
> >  
> >  	if (!plen)
> >  		tb->tb_num_default++;
> > @@ -1287,14 +1292,8 @@ int fib_table_insert(struct net *net, struct fib_table *tb,
> >  succeeded:
> >  	return 0;
> >  
> > -out_fib_notif:
> > -	/* notifier was sent that entry would be added to trie, but
> > -	 * the add failed and need to recover. Only failure for
> > -	 * fib_insert_alias is ENOMEM.
> > -	 */
> > -	NL_SET_ERR_MSG(extack, "Failed to insert route into trie");
> > -	call_fib_entry_notifiers(net, FIB_EVENT_ENTRY_DEL, key,
> > -				 plen, new_fa, NULL);
> > +out_remove_new_fa:
> > +	fib_remove_alias(t, tp, l, new_fa);
> >  out_free_new_fa:
> >  	kmem_cache_free(fn_alias_kmem, new_fa);
> >  out:
> > 
>