Guillaume Nault <gnault@xxxxxxxxxx> 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 <pabeni@xxxxxxxxxx> > Signed-off-by: Guillaume Nault <gnault@xxxxxxxxxx> > --- > 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 <herbert@xxxxxxxxxxxxxxxxxxx> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt