Date:   Thu, 21 Mar 2019 23:59:45 +0100
From:   Guillaume Nault <gnault@redhat.com>
To:     linux-crypto@vger.kernel.org
Cc:     Atul Gupta <atul.gupta@chelsio.com>,
        Paolo Abeni <pabeni@redhat.com>,
        Eric Dumazet <eric.dumazet@gmail.com>
Subject: [RFC PATCH] crypto: chtls - handle inet_csk_reqsk_queue_add()
 failures
Message-ID: <c96b622f873160880843766d62f4ccac08abd302.1553208186.git.gnault@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-crypto-owner@vger.kernel.org
Precedence: bulk

While working on 9d3e1368bb45 ("tcp: handle inet_csk_reqsk_queue_add()
failures"), I missed this usage of inet_csk_reqsk_queue_add().

inet_csk_reqsk_queue_add() can fail. In that case, the operation is
aborted and inet_csk_destroy_sock() is called automatically, but we
need to free the request socket manually.

RFC:
Sending as RFC because I'm unsure about how to handle 'child' in that
case. I don't call add_to_reap_list(child) here, because
inet_csk_destroy_sock() has already run. Therefore, most of the
operations performed by add_to_reap_list()/process_reap_list() would be
redundant (but harmless). Except for two things:

  * process_reap_list() calls inet_csk_destroy_sock() if ->sk_state is
    TCP_CLOSE. We have to avoid that. Socket's state should be
    TCP_ESTABLISHED at the time of the call, but process_reap_list() is
    run asynchronously and I couldn't find anything that would
    prevent the rest of the system from accessing the socket before
    process_reap_list() picks it up (the socket is still accessible
    from cdev->tids). So I fear that calling add_to_reap_list() in the
    error path would let process_reap_list() call
    inet_csk_reqsk_queue_add() a second time if ->sk_state was
    concurrently set to TCP_CLOSE. That looks like a good reason not to
    call add_to_reap_list().

  * The only operation that process_reap_list() does and isn't already
    done by inet_csk_destroy_sock() is chtls_abort_conn(). This
    function has no effect if its sk_buff allocation fails. So it might
    be a no-op. That looks like another reason for not calling
    add_to_reap_list()...
    But what chtls_abort_conn() does when sk_buff allocation succeeds
    looks actually important. IIUC, it sends a message to trigger the
    removal of the socket (how would the socket be removed from ->tids
    otherwise?). That would make chtls_abort_conn() quite important.
    But then how do we avoid losing track of those defunct sockets when
    sk_buff allocation fails?

Compile-tested only.

Fixes: cc35c88ae4db ("crypto : chtls - CPL handler definition")
Reported-by: Paolo Abeni <pabeni@redhat.com>
Signed-off-by: Guillaume Nault <gnault@redhat.com>
---
 drivers/crypto/chelsio/chtls/chtls_cm.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/crypto/chelsio/chtls/chtls_cm.c b/drivers/crypto/chelsio/chtls/chtls_cm.c
index 59b75299fcbc..4bf1e3f65400 100644
--- a/drivers/crypto/chelsio/chtls/chtls_cm.c
+++ b/drivers/crypto/chelsio/chtls/chtls_cm.c
@@ -1384,11 +1384,16 @@ static void add_pass_open_to_parent(struct sock *child, struct sock *lsk,
 	if (sk_acceptq_is_full(lsk)) {
 		chtls_reqsk_free(oreq);
 		add_to_reap_list(child);
-	} else {
-		refcount_set(&oreq->rsk_refcnt, 1);
-		inet_csk_reqsk_queue_add(lsk, oreq, child);
-		lsk->sk_data_ready(lsk);
+		return;
+	}
+
+	refcount_set(&oreq->rsk_refcnt, 1);
+	if (!inet_csk_reqsk_queue_add(lsk, oreq, child)) {
+		chtls_reqsk_free(oreq);
+		/* child already dropped by inet_csk_destroy_sock() */
+		return;
 	}
+	lsk->sk_data_ready(lsk);
 }
 
 static void bl_add_pass_open_to_parent(struct sock *lsk, struct sk_buff *skb)
-- 
2.20.1