Re: [PATCH net] net: socket: set sock->sk to NULL after calling proto_ops::release()

[Al Viro]

On Thu, Feb 21, 2019 at 02:13:56PM -0800, Eric Biggers wrote:

> Rather than fixing all these and relying on every socket type to get
> this right forever, just make __sock_release() set sock->sk to NULL
> itself after calling proto_ops::release().

	Is there any case when we would want sock->sk non-NULL when
sock->sk->sk_socket is NULL?

	IOW, why not make sock_orphan() take care of that, at the same
time we dissociate sock from socket?  After all, on the other end
of things we have both sock_graft() and sock_init_data() set both
sock->sk and sk->sk_socket...

	Looking at the assignments to sock->sk, I see
* atalk_release(), bcm_release(), raw_release(), pfkey_release(),
pppol2tp_release(), packet_release(), smc_release(), xsk_release() -
directly after sock_orphan()
* rxrpc_release() - directly *before* sock_orphan()

* ax25_release() - somewhat after sock_orphan(); no idea if anything
done in between needs it still set.  Doesn't looks like there could
be - readers of sock->sk in there are all in methods that can't
overlap with ->release().
* kcm_release() - similar.
* rds_release() - similar.
* tipc_release() - similar.
* unix_accept() - similar, and there I'm fairly sure it could be
done right after sock_orphan()
* netrom_release() - similar.  BTW, why the hell does nr_accept()
check for NULL sock->sk, while other methods (callable for exact
same sockets) assume that it's non-NULL?  AFAICS, the check in
nr_accept() is pointless - nr_create() can leave sock->sk NULL
only if it fails, in which case it's going to be immediately
hit by ->release() and freed by iput() in sock_release().
* rose_release() - similar, complete with ->accept() oddity.

* caif_release() - done *before* sock_orphan(), with other bit of
the latter duplicated just prior.  NFI why; it messes with
debugfs, so there's a whole kettle of copulating slugs involved ;-/

* ieee802154_sock_release(), inet_release(), pn_socket_release() -
done before calliing into protocol's ->close(), which ends up
calling sock_orphan(), either directly or via sk_common_release().
I suspect that we would be find with zeroing sock->sk delayed
until sock_orphan() in all of those, but that needs more RTFS
than I want to go into at the moment.

* netlink_release() - somewhat after sock_orphan(); not sure,
calls of ->netlink_unbind() done inbetween might want it still
set.

* qrtr_release() - lacks sock_orphan(), might be broken

* vsock_releae() - NFI.

* vcc_create() - clears sock->sk in the very beginning; AFAICS
that's pointless.