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 09:53:50 -0400
From:   Sowmini Varadhan <sowmini.varadhan@...cle.com>
To:     Kirill Tkhai <ktkhai@...tuozzo.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)


Found my previous question:

https://www.mail-archive.com/netdev@vger.kernel.org/msg72330.html

(see section about "Comments are specifically ivinted.."

> 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.

I'm not averse to changing this to NETDEV_UNREGISTER
as long as it works for the 2 test cases below- you 
can test it by using rds-ping from rds-tools rpm, to
be used from/to init_net, from/to the netns  against
some external machine (i.e something not on the same
physical host)

> > 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.

I am familiar with ops_exit_list, but this is the sequence:
- when the module is loaded (or netns is started) it starts a 
  kernel listen socket on *.16385
- when you start the rds-pings above, it will create kernel
  tcp connections from/to the 16385 in the netns. And it will
  start socket keepalives for those connections. Each tcp 
  connection is associated with a rds_connection

As I recall, when I wrote the initial patchset, my problem
was that in order to let the module unload make progress,
all these sockets had to be cleaned up. But to clean up these
sockets, net_device cleanup had to complete (should not have
any new incoming connections to the listen endpoint on a 
non-loopback socket) so I ended up with a circular dependancy.

> 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.

Please share your patch, I can review it and maybe help to test
it..

As I was trying to say in my RFC, I am quite open to ways to make
this cleanup more obvious

--Sowmini