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: <78f2a7c3-e298-9b32-b1a6-20854e0de41b@redhat.com>
Date:   Thu, 21 Jul 2022 16:37:50 -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>,
        Chuck Lever III <chuck.lever@oracle.com>
Cc:     libtirpc List <libtirpc-devel@lists.sourceforge.net>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>
References: <53af8ec6-4ece-09b2-9499-d46d0fdfaa15@cfa.harvard.edu>
 <4D490A86-E873-40E7-A30E-9189B127C65C@oracle.com>
 <9241d8a5-d341-aa84-9aee-6e84377dbe9d@cfa.harvard.edu>
From:   Steve Dickson <steved@redhat.com>
In-Reply-To: <9241d8a5-d341-aa84-9aee-6e84377dbe9d@cfa.harvard.edu>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hey!

On 7/21/22 3:19 PM, Attila Kovacs wrote:
> Hi Chuck,
> 
> 
> I'm not adding any new functionality here. I'm merely trying to fix 
> what's already there but broken (in my opinion). :-)
I agree with this.... And I will be more that willing
to reformat the patches make it easier to review

I don't want nits like that to get in the way this good work,
but it would be good if you could added the
     Signed-off-by: Attila Kovacs <attila.kovacs@cfa.harvard.edu>

line, which is described in [1]

steved.

[1] https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html
> 
> -- A.
> 
> 
> On 7/21/22 14:45, Chuck Lever III wrote:
>>
>>
>>> On Jul 21, 2022, at 2:41 PM, Attila Kovacs 
>>> <attila.kovacs@cfa.harvard.edu> wrote:
>>>
>>> Hi again,
>>>
>>>
>>> I found yet more potential MT flaws in clnt_dg.c and clnt_vg.c.
>>
>> IIRC libtirpc is not MT-enabled because it was forked from the
>> Solaris libtirpc before MT-enablement was completed. I could
>> be remembering incorrectly.
>>
>> Therefore I think we would need some unit tests along with the
>> improvements you propose. There's a test suite somewhere, but
>> someone else will need to provide details.
>>
>> (and also, please inline your patch submissions rather than
>> attaching them, to make it easier for us to review. thanks!)
>>
>>
>>> 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.<clnt_mt_safe_destroy.patch>
>>
>> -- 
>> Chuck Lever
>>
>>
>>
>