netdev - Re: [patch net-next 02/15] net: fib_notifier: make FIB notifier per-netns

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:   Sun, 15 Sep 2019 14:05:47 -0600
From:   David Ahern <dsahern@...il.com>
To:     Jiri Pirko <jiri@...nulli.us>, netdev@...r.kernel.org
Cc:     davem@...emloft.net, idosch@...lanox.com,
        jakub.kicinski@...ronome.com, tariqt@...lanox.com,
        saeedm@...lanox.com, kuznet@....inr.ac.ru, yoshfuji@...ux-ipv6.org,
        shuah@...nel.org, mlxsw@...lanox.com
Subject: Re: [patch net-next 02/15] net: fib_notifier: make FIB notifier
 per-netns

On 9/14/19 12:45 AM, Jiri Pirko wrote:
>  #define FIB_DUMP_MAX_RETRIES 5
> -int register_fib_notifier(struct notifier_block *nb,
> +int register_fib_notifier(struct net *net, struct notifier_block *nb,
>  			  void (*cb)(struct notifier_block *nb))
>  {
>  	int retries = 0;
>  	int err;
>  
>  	do {
> -		unsigned int fib_seq = fib_seq_sum();
> -		struct net *net;
> -
> -		rcu_read_lock();
> -		for_each_net_rcu(net) {
> -			err = fib_net_dump(net, nb);
> -			if (err)
> -				goto err_fib_net_dump;
> -		}
> -		rcu_read_unlock();
> -
> -		if (fib_dump_is_consistent(nb, cb, fib_seq))
> +		unsigned int fib_seq = fib_seq_sum(net);
> +
> +		err = fib_net_dump(net, nb);
> +		if (err)
> +			return err;
> +
> +		if (fib_dump_is_consistent(net, nb, cb, fib_seq))
>  			return 0;
>  	} while (++retries < FIB_DUMP_MAX_RETRIES);

This is still more complicated than it needs to be. Why lump all
fib_notifier_ops into 1 dump when they are separate databases with
separate seq numbers? Just dump them 1 at a time and retry that 1
database as needed.

ie., This:
    list_for_each_entry_rcu(ops, &net->fib_notifier_ops, list) {
should be in register_fib_notifier and not fib_net_dump.

as it stands you are potentially replaying way more than is needed when
a dump is inconsistent.


>  
>  	return -EBUSY;
> -
> -err_fib_net_dump:
> -	rcu_read_unlock();
> -	return err;
>  }
>  EXPORT_SYMBOL(register_fib_notifier);
>  
> -int unregister_fib_notifier(struct notifier_block *nb)
> +int unregister_fib_notifier(struct net *net, struct notifier_block *nb)
>  {
> -	return atomic_notifier_chain_unregister(&fib_chain, nb);
> +	struct fib_notifier_net *fn_net = net_generic(net, fib_notifier_net_id);
> +
> +	return atomic_notifier_chain_unregister(&fn_net->fib_chain, nb);
>  }
>  EXPORT_SYMBOL(unregister_fib_notifier);
>