From patchwork Mon Jul 25 16:06:45 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Attila Kovacs X-Patchwork-Id: 12928399 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id EE0A4C43334 for ; Mon, 25 Jul 2022 20:42:37 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S235916AbiGYUmg (ORCPT ); Mon, 25 Jul 2022 16:42:36 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:46510 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S237133AbiGYUme (ORCPT ); Mon, 25 Jul 2022 16:42:34 -0400 Received: from mail-qk1-x72d.google.com (mail-qk1-x72d.google.com [IPv6:2607:f8b0:4864:20::72d]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 880B222BDF for ; Mon, 25 Jul 2022 13:42:33 -0700 (PDT) Received: by mail-qk1-x72d.google.com with SMTP id g1so9615819qki.7 for ; Mon, 25 Jul 2022 13:42:33 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=cfa.harvard.edu; s=google; h=from:to:cc:subject:date:message-id:mime-version :content-transfer-encoding; bh=DBgOCZVjCfLSrGPma3nuECbYEVoJ2GP0xe2GTjQBAmA=; b=UHWAeq7284Wg5A7tu2wd/NGckBLidB7/OT/bNipjcoWH/fWfSMWvs+8BKYM8hzo7g4 gKxSUwoHrjymxB134vOfVAaQt7inTe+qwzzgOYScpmUffftMToAxz1v9ieyabqzilqs0 d8U2bWuUxxsnaYC/5x4X0xs1Hihc4gSvMbPplpIfrxzbN90S9Jcuh8MUI1l5TKfBQwSP Cj2oC0u95NNKe4tH1j1mcc6j+kj00QDeLQshDXOwINW6mMvpBaa3efzpo9pYcliQDOB6 h3KvFg8sqV7oBFJDmui/uwRDV2fwtZxyZUNhyh2GTOeWCCeKaUBM9ZjaRCwMtUE4AGbr rFng== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:from:to:cc:subject:date:message-id:mime-version :content-transfer-encoding; bh=DBgOCZVjCfLSrGPma3nuECbYEVoJ2GP0xe2GTjQBAmA=; b=KDHN6ACyhouqqI9KgjqN1qBP0qqJGU6C9TiV9zUaaY+R67EZPYfPz97euz0AkcpTe6 jlW7UMnm7VyjaDlnjTaJMERRXamKDcxcgyuiAulhDQfihDu/v24zqkWSJZNrNMpK61NE ix5Fj0Gr4I7OBOC6+iMQEVOfkrAgM39kA/I7ljGs4CK+CciudNhLBkDy8oO/k2G3puxp Da84nslRyOJhWK1X94PK66yGeKtwdWstfYO74XRJK/EmTqHTZnIy7aKB9RaEvTlygO/E cQ3ZIbeH9oISuqvlC+Qds8fCGnztK7SAMMsJ4cXmoYFoIR8qktiwLuAWATiv3ysZIkQF nNVA== X-Gm-Message-State: AJIora9IOD8XqyhWIgz4UHeMMuzJq7BXwecash2gGedKiA1Nu7wZM5BH NJlZohyoqH/LUiZM6YLzMk47mum2976v6A== X-Google-Smtp-Source: AGRyM1vLFrJFnhMO64BhW6J+O9Qs2mEg8TnurEF0z+SQ02bWQMi2cW11xJTAAhSbx9bKxq8NBBx3Yw== X-Received: by 2002:a37:8043:0:b0:6b6:214f:157d with SMTP id b64-20020a378043000000b006b6214f157dmr10334095qkd.371.1658781752531; Mon, 25 Jul 2022 13:42:32 -0700 (PDT) Received: from pihe ([65.112.10.207]) by smtp.gmail.com with ESMTPSA id a187-20020ae9e8c4000000b006b4689e3425sm9275434qkg.129.2022.07.25.13.42.31 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 25 Jul 2022 13:42:32 -0700 (PDT) Received: from pihe (localhost [127.0.0.1]) by pihe (8.17.1/8.17.1) with ESMTPS id 26PG7oct015744 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NOT); Mon, 25 Jul 2022 12:07:50 -0400 Received: (from pumukli@localhost) by pihe (8.17.1/8.17.1/Submit) id 26PG7o7W015620; Mon, 25 Jul 2022 12:07:50 -0400 From: Attila Kovacs To: Libtirpc-devel Mailing List Cc: Linux NFS Mailing list Subject: [PATCH 1/2] clnt_dg_freeres() uncleared set active state may deadlock. Date: Mon, 25 Jul 2022 12:06:45 -0400 Message-Id: <20220725160646.15610-1-attila.kovacs@cfa.harvard.edu> X-Mailer: git-send-email 2.37.1 MIME-Version: 1.0 Precedence: bulk List-ID: X-Mailing-List: linux-nfs@vger.kernel.org From: Attila Kovacs In clnt_dg.c in clnt_dg_freeres(), cu_fd_lock->active is set to TRUE, with no corresponding clearing when the operation (*xdr_res() call) is completed. This would leave other waiting operations blocked indefinitely, effectively deadlocking the client. For comparison, clnt_vd_freeres() in clnt_vc.c does not set the active state to TRUE. I believe the vc behavior is correct, while the dg behavior is a bug. Signed-off-by: Attila Kovacs --- src/clnt_dg.c | 1 - 1 file changed, 1 deletion(-) diff --git a/src/clnt_dg.c b/src/clnt_dg.c index 7c5d22e..b2043ac 100644 --- a/src/clnt_dg.c +++ b/src/clnt_dg.c @@ -573,7 +573,6 @@ clnt_dg_freeres(cl, xdr_res, res_ptr) mutex_lock(&clnt_fd_lock); while (cu->cu_fd_lock->active) cond_wait(&cu->cu_fd_lock->cv, &clnt_fd_lock); - cu->cu_fd_lock->active = TRUE; xdrs->x_op = XDR_FREE; dummy = (*xdr_res)(xdrs, res_ptr); thr_sigsetmask(SIG_SETMASK, &mask, NULL); From patchwork Mon Jul 25 16:06:46 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Attila Kovacs X-Patchwork-Id: 12928398 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 696F3C433EF for ; Mon, 25 Jul 2022 20:42:37 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S237237AbiGYUmf (ORCPT ); Mon, 25 Jul 2022 16:42:35 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:46520 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S237233AbiGYUmf (ORCPT ); Mon, 25 Jul 2022 16:42:35 -0400 Received: from mail-qk1-x72d.google.com (mail-qk1-x72d.google.com [IPv6:2607:f8b0:4864:20::72d]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 4D25322BE8 for ; Mon, 25 Jul 2022 13:42:34 -0700 (PDT) Received: by mail-qk1-x72d.google.com with SMTP id z18so9320097qki.2 for ; Mon, 25 Jul 2022 13:42:34 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=cfa.harvard.edu; s=google; h=from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=XUev4aVqJ5kVJbWbmM/RDsUFZ/tOjyhFiv32RH06njk=; b=TGNccJHyXhTG4ei5lVYklvsVkmYzPmGaVdc5RPQSxeewkol6O6kKKT+XuyJl7tiyh1 KDuIm+qYvPLAXMaTcjKdwTWqzsiec9WchqT6LAsvjw6GxbhytKxIxSePxpovoZBKbGet +RJREzf5lpZcGs3pyC9Y052MM/EiS/MufMRveBgZNsXs0Hw4VMWmGJrY96lzWXKvBuov CqKi90rWl2guYFCO/V+9UuUavFUTxV0Y5u/r72Z7jL1ofInn0rcobPLe4tXhayqvNXpo USJUTysZl0BsYkGjyBreXjicgt/YLgXxujpBppqfyANSixXRpypfkhsd/TRJt/3Vg3FH /ohw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=XUev4aVqJ5kVJbWbmM/RDsUFZ/tOjyhFiv32RH06njk=; b=jh2UeYU5jwMYEqu5arH7rHWSk7UV3SmYo9x91GettpAhsrESUVF+KVbrbxewz6DyhI N/l53vU5De+qZQAVukmO1vRDRwatsCFMnq5Dh4hjezY52u50AnyQ/q73HQwwoOvB2mjV cAxkOKkQa/7mYnVkocKTBSYX1IygYs9UGUx/sg3Sj01xrGyfEetZKMeiz8tgKNOzOexO VRwVWd2DEdgrM3F9T6obMkuioKz2BSkB8WFbMDsiyePxPdl/g3/iZriKSR4UsDDhFjGt 7OKv7O4l7TpBU0babdP0Z3q7C+kzxM9jsqPYPQrXVtX0mi3NvXZrD1qEw76tiuuXlmzA 7AcA== X-Gm-Message-State: AJIora+EX/0108TTOPkrv7BgwuDL46Gqk4C2edS+FtV0xKKX2OjjG98+ Ti+943vvEVm5YAy5vlflDu+FwCVAscKAnw== X-Google-Smtp-Source: AGRyM1u1GstvecG7qBqGLB2mX4uS7/1SGY6prDnDSs/QkinEbEeLYMi+T0sKegVz+XZXTOiJYSer3w== X-Received: by 2002:a05:620a:1907:b0:6a6:2fac:462f with SMTP id bj7-20020a05620a190700b006a62fac462fmr10162573qkb.213.1658781753171; Mon, 25 Jul 2022 13:42:33 -0700 (PDT) Received: from pihe ([65.112.10.207]) by smtp.gmail.com with ESMTPSA id a187-20020ae9e8c4000000b006b4689e3425sm9275434qkg.129.2022.07.25.13.42.32 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 25 Jul 2022 13:42:32 -0700 (PDT) Received: from pihe (localhost [127.0.0.1]) by pihe (8.17.1/8.17.1) with ESMTPS id 26PG8ovj015839 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NOT); Mon, 25 Jul 2022 12:08:50 -0400 Received: (from pumukli@localhost) by pihe (8.17.1/8.17.1/Submit) id 26PG8oph015747; Mon, 25 Jul 2022 12:08:50 -0400 From: Attila Kovacs To: Libtirpc-devel Mailing List Cc: Linux NFS Mailing list Subject: [PATCH 2/2] thread safe clnt destruction. Date: Mon, 25 Jul 2022 12:06:46 -0400 Message-Id: <20220725160646.15610-2-attila.kovacs@cfa.harvard.edu> X-Mailer: git-send-email 2.37.1 In-Reply-To: <20220725160646.15610-1-attila.kovacs@cfa.harvard.edu> References: <20220725160646.15610-1-attila.kovacs@cfa.harvard.edu> MIME-Version: 1.0 Precedence: bulk List-ID: X-Mailing-List: linux-nfs@vger.kernel.org From: Attila Kovacs If clnt_dg_destroy() or clnt_vc_destroy() is awoken with other blocked operations pending (such as clnt_*_call(), clnt_*_control(), or clnt_*_freeres()) but no active operation currently being executed, then the client gets destroyed. Then, as the other blocked operations get subsequently awoken, they will try operate on an invalid client handle, potentially causing unpredictable behavior and stack corruption. Signed-off-by: Attila Kovacs --- src/clnt_dg.c | 13 ++++++++++++- src/clnt_fd_locks.h | 2 ++ src/clnt_vc.c | 13 ++++++++++++- 3 files changed, 26 insertions(+), 2 deletions(-) diff --git a/src/clnt_dg.c b/src/clnt_dg.c index b2043ac..166af63 100644 --- a/src/clnt_dg.c +++ b/src/clnt_dg.c @@ -101,6 +101,7 @@ extern mutex_t clnt_fd_lock; #define release_fd_lock(fd_lock, mask) { \ mutex_lock(&clnt_fd_lock); \ fd_lock->active = FALSE; \ + fd_lock->pending--; \ thr_sigsetmask(SIG_SETMASK, &(mask), NULL); \ cond_signal(&fd_lock->cv); \ mutex_unlock(&clnt_fd_lock); \ @@ -311,6 +312,7 @@ clnt_dg_call(cl, proc, xargs, argsp, xresults, resultsp, utimeout) sigfillset(&newmask); thr_sigsetmask(SIG_SETMASK, &newmask, &mask); mutex_lock(&clnt_fd_lock); + cu->cu_fd_lock->pending++; while (cu->cu_fd_lock->active) cond_wait(&cu->cu_fd_lock->cv, &clnt_fd_lock); cu->cu_fd_lock->active = TRUE; @@ -571,10 +573,12 @@ clnt_dg_freeres(cl, xdr_res, res_ptr) sigfillset(&newmask); thr_sigsetmask(SIG_SETMASK, &newmask, &mask); mutex_lock(&clnt_fd_lock); + cu->cu_fd_lock->pending++; while (cu->cu_fd_lock->active) cond_wait(&cu->cu_fd_lock->cv, &clnt_fd_lock); xdrs->x_op = XDR_FREE; dummy = (*xdr_res)(xdrs, res_ptr); + cu->cu_fd_lock->pending--; thr_sigsetmask(SIG_SETMASK, &mask, NULL); cond_signal(&cu->cu_fd_lock->cv); mutex_unlock(&clnt_fd_lock); @@ -602,6 +606,7 @@ clnt_dg_control(cl, request, info) sigfillset(&newmask); thr_sigsetmask(SIG_SETMASK, &newmask, &mask); mutex_lock(&clnt_fd_lock); + cu->cu_fd_lock->pending++; while (cu->cu_fd_lock->active) cond_wait(&cu->cu_fd_lock->cv, &clnt_fd_lock); cu->cu_fd_lock->active = TRUE; @@ -742,8 +747,14 @@ clnt_dg_destroy(cl) sigfillset(&newmask); thr_sigsetmask(SIG_SETMASK, &newmask, &mask); mutex_lock(&clnt_fd_lock); - while (cu_fd_lock->active) + /* wait until all pending operations on client are completed. */ + while (cu_fd_lock->pending > 0) { + /* If a blocked operation can be awakened, then do it. */ + if (cu_fd_lock->active == FALSE) + cond_signal(&cu_fd_lock->cv); + /* keep waiting... */ cond_wait(&cu_fd_lock->cv, &clnt_fd_lock); + } if (cu->cu_closeit) (void)close(cu_fd); XDR_DESTROY(&(cu->cu_outxdrs)); diff --git a/src/clnt_fd_locks.h b/src/clnt_fd_locks.h index 359f995..6ba62cb 100644 --- a/src/clnt_fd_locks.h +++ b/src/clnt_fd_locks.h @@ -50,6 +50,7 @@ static unsigned int fd_locks_prealloc = 0; /* per-fd lock */ struct fd_lock_t { bool_t active; + int pending; /* Number of pending operations on fd */ cond_t cv; }; typedef struct fd_lock_t fd_lock_t; @@ -180,6 +181,7 @@ fd_lock_t* fd_lock_create(int fd, fd_locks_t *fd_locks) { item->fd = fd; item->refs = 1; item->fd_lock.active = FALSE; + item->fd_lock.pending = 0; cond_init(&item->fd_lock.cv, 0, (void *) 0); TAILQ_INSERT_HEAD(list, item, link); } else { diff --git a/src/clnt_vc.c b/src/clnt_vc.c index 3c73e65..5bbc78b 100644 --- a/src/clnt_vc.c +++ b/src/clnt_vc.c @@ -153,6 +153,7 @@ extern mutex_t clnt_fd_lock; #define release_fd_lock(fd_lock, mask) { \ mutex_lock(&clnt_fd_lock); \ fd_lock->active = FALSE; \ + fd_lock->pending--; \ thr_sigsetmask(SIG_SETMASK, &(mask), (sigset_t *) NULL); \ cond_signal(&fd_lock->cv); \ mutex_unlock(&clnt_fd_lock); \ @@ -357,6 +358,7 @@ clnt_vc_call(cl, proc, xdr_args, args_ptr, xdr_results, results_ptr, timeout) sigfillset(&newmask); thr_sigsetmask(SIG_SETMASK, &newmask, &mask); mutex_lock(&clnt_fd_lock); + ct->ct_fd_lock->pending++; while (ct->ct_fd_lock->active) cond_wait(&ct->ct_fd_lock->cv, &clnt_fd_lock); ct->ct_fd_lock->active = TRUE; @@ -495,10 +497,12 @@ clnt_vc_freeres(cl, xdr_res, res_ptr) sigfillset(&newmask); thr_sigsetmask(SIG_SETMASK, &newmask, &mask); mutex_lock(&clnt_fd_lock); + ct->ct_fd_lock->pending++; while (ct->ct_fd_lock->active) cond_wait(&ct->ct_fd_lock->cv, &clnt_fd_lock); xdrs->x_op = XDR_FREE; dummy = (*xdr_res)(xdrs, res_ptr); + ct->ct_fd_lock->pending--; thr_sigsetmask(SIG_SETMASK, &(mask), NULL); cond_signal(&ct->ct_fd_lock->cv); mutex_unlock(&clnt_fd_lock); @@ -533,6 +537,7 @@ clnt_vc_control(cl, request, info) sigfillset(&newmask); thr_sigsetmask(SIG_SETMASK, &newmask, &mask); mutex_lock(&clnt_fd_lock); + ct->ct_fd_lock->pending++; while (ct->ct_fd_lock->active) cond_wait(&ct->ct_fd_lock->cv, &clnt_fd_lock); ct->ct_fd_lock->active = TRUE; @@ -655,8 +660,14 @@ clnt_vc_destroy(cl) sigfillset(&newmask); thr_sigsetmask(SIG_SETMASK, &newmask, &mask); mutex_lock(&clnt_fd_lock); - while (ct_fd_lock->active) + /* wait until all pending operations on client are completed. */ + while (ct_fd_lock->pending > 0) { + /* If a blocked operation can be awakened, then do it. */ + if (ct_fd_lock->active == FALSE) + cond_signal(&ct_fd_lock->cv); + /* keep waiting... */ cond_wait(&ct_fd_lock->cv, &clnt_fd_lock); + } if (ct->ct_closeit && ct->ct_fd != -1) { (void)close(ct->ct_fd); }