On 06/20/2013 01:29 PM, Daniel Borkmann wrote:
On 06/20/2013 04:35 PM, Vlad Yasevich wrote:
On 06/18/2013 04:55 AM, Daniel Borkmann wrote:
Rather instead of having the endpoint clean the garbage for the
socket, use a sk_destruct handler sctp_destruct_sock(), that does
the job for that when there are no more references on the socket.
With this patch it is possible to run sctp_put_port while the socket
is not locked.
The flow goes something like this:
sctp_close()
sk_bh_lock_sock();
sk_common_release()
sctp_destroy_sock()
endpoint_put()
endpoint_destroy() <-- This is where we used to do sctp_put_port
sk_bh_unlock_sock();
sock_put()
sk_free()
__sk_free()
sctp_destruct_sock()
sctp_put_port()
I haven't found any race conditions yet, but that doesn't mean they
don't exist.
I think an easy solution would be to do sctp_put_port in sctp_unhash(),
but I haven't traced all the paths.
Hm, compared to the current (pre-patch) solution, sctp_put_port() does not
necessarily need to be called at sk_common_release() time if refs are still
on the endpoint, so that endpoint_destroy() is further deferred in time.
Thus,
if we would do the sctp_put_port() in sctp_unhash(), we could free it at an
earlier time than with endpoint_destroy(). This does not necessarily
need to
be a bad or wrong way, but with the current approach it's done at an later
point in time afaik.
You are right. Doing it in sctp_unhash() could be possibly too early.
If it's only about the locking, what if we just hold
that socket lock around sctp_put_port() in the current patch?
But besides that, if at such a late point in time someone still has
access to
that socket member (right before we do the kfree(sk)), we would be
pretty much
screwed. :-) Despite having the socket lock or not, the port hashtable
has it's
own protection from what I see.
Yes it does and I've been looking to see if this is sufficient enough
for our purposes. It looks like our saving grace is the fact that
we set the sk_state to CLOSED sctp_endpoint_free(). Otherwise, we'd
have a race between sctp_endpoint_destroy() and conflict detection
in sctp_get_port_local. This seems a bit fragile and we are making
it a bit so more with this patch.
I think it would be better to see if we can remove the socket from
the port table a bit earlier if possbile.
-vlad
--
To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
