From patchwork Thu Jul 21 18:41:15 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Attila Kovacs X-Patchwork-Id: 12925688 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 234A3C43334 for ; Thu, 21 Jul 2022 18:41:21 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229613AbiGUSlT (ORCPT ); Thu, 21 Jul 2022 14:41:19 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:59668 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229379AbiGUSlT (ORCPT ); Thu, 21 Jul 2022 14:41:19 -0400 Received: from mail-qt1-x82c.google.com (mail-qt1-x82c.google.com [IPv6:2607:f8b0:4864:20::82c]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id D6A3D32B92 for ; Thu, 21 Jul 2022 11:41:17 -0700 (PDT) Received: by mail-qt1-x82c.google.com with SMTP id c20so1933095qtw.8 for ; Thu, 21 Jul 2022 11:41:17 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=cfa.harvard.edu; s=google; h=message-id:date:mime-version:user-agent:content-language:to:cc:from :subject; bh=+m4HI3ifoLTVsxzoe3VtJMXXum2ZrpP4hnL5fhcNW9E=; b=Lt35IxxZk4x/8q01iA57+mpN1bdf6hClHBBwbKxBdBloCEZUH6xAqrYAso4MnD5uzq fZwfeUeIc6NY9LvM7j+wJHZQazekQ8LqD5CMoFbB8AO9F8+d3H+dg/LvyP7g20F4uWy8 2f9ZXPcQn4dSbsfUl31YG0ybKpdYwEr3lLAjp0s3qTIkww+BUdeIgAIMGx5R2QxTxYbB rVspzSfMRRc/9qWq3Xuz8Oy+O06jyBNcozPtRTN/kph4jz+NAQCOfR9xN9efSNtkSe7f /tKijRcX53oJ9p6ro+DEDsJ0xIOG2ay0hJ7HhUX0yduP/ps/0jbCKZtJK0Ift3FQQtm4 paAA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:date:mime-version:user-agent :content-language:to:cc:from:subject; bh=+m4HI3ifoLTVsxzoe3VtJMXXum2ZrpP4hnL5fhcNW9E=; b=CPYDQ7ub5GCLvjts1h6ZqKq2k+o4R2ImqlyZt7gXfX9uCUUuuXxYEMpBobuOodYeG5 1tna8pnJINzZ4ffq+PaSik8chGwAAlhzB+mZI6PV3nJdgLEfVBKtjFSc2tlvb/JU8TeS Uegg32ZppgFVAWEDBLhGwLRJ7Hnd9HfwhAJ4Y5qvVUvXhiaguRkht4xjO/PsJjc2UBaJ FhM0pSvywPb9VSpfau+S9zGN1Nf4bI3p07U134g03OA+gKH0k6PtplMsVS34xsTNhA1u hywvYcn3X9PNlt76ZXgpQ0BUL6UpoLF0+HWHnAxdSGxpdeW62THxsmqDle1ajaBqmmtE sbuQ== X-Gm-Message-State: AJIora+T8ZT09XDUIxn7jufWcbZ8oj8DgMVA1mvrggwB2vTshDJD1q8D bQHTwkFmf1Sgl2CpzMZh/0grDj3b3ZBXzQ== X-Google-Smtp-Source: AGRyM1uSClTzAP9biecpslUgLuiEzhIpq90iFHoPRr/Yr97ngAvUIfJsSqzPVV4pyff9+L0qEaMBBA== X-Received: by 2002:ac8:5f52:0:b0:31f:19f:669c with SMTP id y18-20020ac85f52000000b0031f019f669cmr11417767qta.474.1658428876946; Thu, 21 Jul 2022 11:41:16 -0700 (PDT) Received: from [192.168.0.156] (pool-72-70-58-62.bstnma.fios.verizon.net. [72.70.58.62]) by smtp.gmail.com with ESMTPSA id k14-20020ac85fce000000b0031ed2038c15sm1791484qta.63.2022.07.21.11.41.15 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 21 Jul 2022 11:41:16 -0700 (PDT) Message-ID: <53af8ec6-4ece-09b2-9499-d46d0fdfaa15@cfa.harvard.edu> Date: Thu, 21 Jul 2022 14:41:15 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.11.0 Content-Language: en-US To: libtirpc-devel@lists.sourceforge.net Cc: linux-nfs@vger.kernel.org From: Attila Kovacs Subject: Thread safe client destruction Precedence: bulk List-ID: X-Mailing-List: linux-nfs@vger.kernel.org Hi again, I found yet more potential MT flaws in clnt_dg.c and clnt_vg.c. 1. 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 deadlocked. 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. 2. 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. The proposed fix is to introduce a simple mutexed counting variable into the client lock structure, which keeps track of the number of pending blocking operations on the client. This way, clnt_*_destroy() can check if there are any operations pending on a client, and keep waiting until all pending operations are completed, before the client is destroyed safely and its resources are freed. Attached is a patch with the above fixes. -- A. diff --git a/src/clnt_dg.c b/src/clnt_dg.c index b3d82e7..9f75e7c 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--; \ mutex_unlock(&clnt_fd_lock); \ thr_sigsetmask(SIG_SETMASK, &(mask), NULL); \ cond_signal(&fd_lock->cv); \ @@ -308,6 +309,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; @@ -568,11 +570,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); - cu->cu_fd_lock->active = TRUE; xdrs->x_op = XDR_FREE; dummy = (*xdr_res)(xdrs, res_ptr); + cu->cu_fd_lock->pending--; mutex_unlock(&clnt_fd_lock); thr_sigsetmask(SIG_SETMASK, &mask, NULL); cond_signal(&cu->cu_fd_lock->cv); @@ -600,6 +603,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; @@ -740,7 +744,7 @@ clnt_dg_destroy(cl) sigfillset(&newmask); thr_sigsetmask(SIG_SETMASK, &newmask, &mask); mutex_lock(&clnt_fd_lock); - while (cu_fd_lock->active) + while (cu_fd_lock->active || cu_fd_lock->pending > 0) cond_wait(&cu_fd_lock->cv, &clnt_fd_lock); if (cu->cu_closeit) (void)close(cu_fd); 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 a07e297..8f9ca74 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--; \ mutex_unlock(&clnt_fd_lock); \ thr_sigsetmask(SIG_SETMASK, &(mask), (sigset_t *) NULL); \ cond_signal(&fd_lock->cv); \ @@ -353,6 +354,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; @@ -491,10 +493,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--; mutex_unlock(&clnt_fd_lock); thr_sigsetmask(SIG_SETMASK, &(mask), NULL); cond_signal(&ct->ct_fd_lock->cv); @@ -529,6 +533,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; @@ -651,7 +656,7 @@ clnt_vc_destroy(cl) sigfillset(&newmask); thr_sigsetmask(SIG_SETMASK, &newmask, &mask); mutex_lock(&clnt_fd_lock); - while (ct_fd_lock->active) + while (ct_fd_lock->active || ct_fd_lock->pending > 0) cond_wait(&ct_fd_lock->cv, &clnt_fd_lock); if (ct->ct_closeit && ct->ct_fd != -1) { (void)close(ct->ct_fd);