On Mon, Jun 08, 2015 at 10:09:50PM +0200, Hannes Frederic Sowa wrote:

On Fr, 2015-06-05 at 14:08 -0300, mleitner@redhat.com wrote:

quoted

      if (sp->do_auto_asconf) {
+             spin_lock_bh(&sock_net(sk)->sctp.addr_wq_lock);
              sp->do_auto_asconf = 0;
-             list_del(&sp->auto_asconf_list);
+             list_del_rcu(&sp->auto_asconf_list);
+             spin_unlock_bh(&sock_net(sk)->sctp.addr_wq_lock);
      }

This also looks a bit unsafe to me:

My proposal would be to sock_hold/sock_put the sockets when pushing them
onto the auto_asconf_list and defer the modifications on the list until

                                ^^^^^^^^^^^^--- you lost me here

we don't need to hold socket lock anymore (in syscalls we do have a reference
anyway).

Yup.. seems we have a use-after-free with this rcu usage on
auto_asconf_splist, because if the socket was destroyed by the time the
timeout handler is running, it may still see that socket and thus we
would need two additional procedures a) to take a sock_hold() when it is
inserted on that list, and release it via call_rcu() and b) to know how
to identify such dead sockets, most likely just by checking
sp->do_auto_asconf, and skip from acting on them.

Neil, WDYT?

addr_wq_lock then is only used either without lock_sock at all or only
in order addr_wq_lock -> lock_sock, which does not cause any locking
ordering issues.

No because we have to update this list on sctp_destroy_sock(), which is
called with lock_sock() held. If we add the precautions above, I think
it will be fine.

Thanks,
Marcelo