netdev - Re: [PATCH RFC RFC] rds: Use NETDEV_UNREGISTER in rds_tcp_dev_event() (then kill NETDEV_UNREGISTER

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:   Fri, 16 Mar 2018 16:17:24 +0300
From:   Kirill Tkhai <ktkhai@...tuozzo.com>
To:     Sowmini Varadhan <sowmini.varadhan@...cle.com>
Cc:     santosh.shilimkar@...cle.com, davem@...emloft.net,
        netdev@...r.kernel.org, linux-rdma@...r.kernel.org,
        rds-devel@....oracle.com, edumazet@...gle.com
Subject: Re: [PATCH RFC RFC] rds: Use NETDEV_UNREGISTER in rds_tcp_dev_event()
 (then kill NETDEV_UNREGISTER_FINAL)

On 16.03.2018 16:00, Sowmini Varadhan wrote:
> On (03/16/18 15:38), Kirill Tkhai wrote:
>>
>> 467fa15356acf by Sowmini Varadhan added NETDEV_UNREGISTER_FINAL dependence
>> with the commentary:
>>
>> 	/* rds-tcp registers as a pernet subys, so the ->exit will only
>> 	 * get invoked after network acitivity has quiesced. We need to
>> 	 * clean up all sockets  to quiesce network activity, and use
>> 	 * the unregistration of the per-net loopback device as a trigger
>> 	 * to start that cleanup.
>> 	 */
>>
>> It seems all the protocols pernet subsystems meet this situation, but they
>> solve it in generic way. What does rds so specific have?
> 
> The difference with rds is this: most consumers of netns associate
> a net_device with the netns, and cleanup of the netns state 
> happens as part of the net_device teardown without the constraint
> above. rds-tcp does has a netns tied to listening socket, not
> to a specific network interface (net_device) so it registers
> as a pernet-subsys. But this means that cleanup has to be
> cone carefully (see comments in net_namespace.h before
> register_pernet_subsys)

This is not a problem, and rds-tcp is not the only pernet_subsys registering
a socket. It's OK to close it from .exit method. There are many examples,
let me point you to icmp_sk_ops as one of them. But it's not the only.

> For rds-tcp, we need to be able to do the right thing in both of these
> cases
> 1. modprobe -r rds-tcp (cleanup of rds-tcp state should happen in
>    every namespace, including init_net)
> 2. netns delete (rds_tcp.ko should remain loaded for other namespaces)

The same as above, every pernet_subsys does this. It's not a problem.
exit and exit_batch methods are called in both of the cases.

Please, see __unregister_pernet_operations()->ops_exit_list for the details.

>> This commit makes event handler to iterate rds_tcp_conn_list and
>> kill them. If we change the stage to NETDEV_UNREGISTER, what will change?
> 
> The above two cases need to work correctly.

Yeah, but let's find another way to have the same.

>> Can unregistered loopback device on dead net add new items to rds_tcp_conn_list?
>> How it's possible?
> 
> I dont understand the question- no unregistered loopback devices
> cannot add items. 

If we replace NETDEV_UNREGISTER_FINAL with NETDEV_UNREGISTER, the only change
which happens is we call rds_tcp_kill_sock() earlier. So, it may be a reason
of problems only if someone changes the list during the time between
NETDEV_UNREGISTER and NETDEV_UNREGISTER_FINAL are called for loopback.
But since this time noone related to this net can extend the list,
there is no a problem to do that.

> fwiw, I had asked questions about this (netns per net_device
> vs netns for module) on the netdev list a few years ago, I can
> try to hunt down that thread for you later (nobody replied to 
> it, but maybe it will help answer your questions).

After your words it looks like we may simply do all the things
in exit method.

Kirill