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 <[email protected]>
Signed-off-by: Guillaume Nault <[email protected]>
---
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
Guillaume Nault <[email protected]> wrote:
> 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 <[email protected]>
> Signed-off-by: Guillaume Nault <[email protected]>
> ---
> 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)
Dave, Eric, could you guys take a look at this patch please?
Thanks!
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt