Re: [PATCH v3 2/2] sctp: fix ASCONF list handling

[Marcelo Ricardo Leitner <mleitner@redhat.com>]

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