Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <3c506b75-30c1-5cc4-595d-0656d05a7a9b@redhat.com>
Date:   Thu, 21 Jul 2022 17:38:09 -0400
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.11.0
Subject: Re: Thread safe client destruction
Content-Language: en-US
To:     Attila Kovacs <attila.kovacs@cfa.harvard.edu>,
        libtirpc-devel@lists.sourceforge.net
Cc:     linux-nfs@vger.kernel.org
References: <53af8ec6-4ece-09b2-9499-d46d0fdfaa15@cfa.harvard.edu>
From:   Steve Dickson <steved@redhat.com>
In-Reply-To: <53af8ec6-4ece-09b2-9499-d46d0fdfaa15@cfa.harvard.edu>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hello,

On 7/21/22 2:41 PM, Attila Kovacs wrote:
> 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.
There is a problem here... This patch does contain the first patch
you posted. So when I apply this patch it fails.

So Please apply your first patch, then apply the fixes from
your second patch... test... then when all is good...
Resent the second patch which will apply, cleanly,
with the first patch.

steved.

> 
> -- A.