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]

Message-ID: <20190916053801.GG2286@nanopsycho.orion>
Date:   Mon, 16 Sep 2019 07:38:01 +0200
From:   Jiri Pirko <jiri@...nulli.us>
To:     David Ahern <dsahern@...il.com>
Cc:     netdev@...r.kernel.org, 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

Sun, Sep 15, 2019 at 10:05:47PM CEST, dsahern@...il.com wrote:
>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.

Well I think that what you describe is out of scope of this patch. It is
another optimization of fib_notifier. The aim of this patchset is not
optimization of fib_notifier, but devlink netns change. This patchset is
just a dependency.

Can't we do optimization in another patchset? I already struggled to
keep this one within 15-patch limit.

Thanks

>
>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);
>>  
>
>
>
>
>