* Thread safe client destruction
@ 2022-07-21 18:41 Attila Kovacs
2022-07-21 18:45 ` Chuck Lever III
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Attila Kovacs @ 2022-07-21 18:41 UTC (permalink / raw)
To: libtirpc-devel; +Cc: linux-nfs
[-- Attachment #1: Type: text/plain, Size: 1351 bytes --]
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.
[-- Attachment #2: clnt_mt_safe_destroy.patch --]
[-- Type: text/x-patch, Size: 4391 bytes --]
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);
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: Thread safe client destruction
2022-07-21 18:41 Thread safe client destruction Attila Kovacs
@ 2022-07-21 18:45 ` Chuck Lever III
2022-07-21 19:19 ` Attila Kovacs
2022-07-21 21:38 ` Steve Dickson
2022-07-22 10:49 ` [PATCH v2 1/1]: " Attila Kovacs
2 siblings, 1 reply; 7+ messages in thread
From: Chuck Lever III @ 2022-07-21 18:45 UTC (permalink / raw)
To: Attila Kovacs; +Cc: libtirpc List, Linux NFS Mailing List
> 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Thread safe client destruction
2022-07-21 18:45 ` Chuck Lever III
@ 2022-07-21 19:19 ` Attila Kovacs
2022-07-21 20:37 ` Steve Dickson
0 siblings, 1 reply; 7+ messages in thread
From: Attila Kovacs @ 2022-07-21 19:19 UTC (permalink / raw)
To: Chuck Lever III; +Cc: libtirpc List, Linux NFS Mailing List
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). :-)
-- 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
>
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Thread safe client destruction
2022-07-21 19:19 ` Attila Kovacs
@ 2022-07-21 20:37 ` Steve Dickson
0 siblings, 0 replies; 7+ messages in thread
From: Steve Dickson @ 2022-07-21 20:37 UTC (permalink / raw)
To: Attila Kovacs, Chuck Lever III; +Cc: libtirpc List, Linux NFS Mailing List
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
>>
>>
>>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Thread safe client destruction
2022-07-21 18:41 Thread safe client destruction Attila Kovacs
2022-07-21 18:45 ` Chuck Lever III
@ 2022-07-21 21:38 ` Steve Dickson
2022-07-21 22:00 ` Attila Kovacs
2022-07-22 10:49 ` [PATCH v2 1/1]: " Attila Kovacs
2 siblings, 1 reply; 7+ messages in thread
From: Steve Dickson @ 2022-07-21 21:38 UTC (permalink / raw)
To: Attila Kovacs, libtirpc-devel; +Cc: linux-nfs
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Thread safe client destruction
2022-07-21 21:38 ` Steve Dickson
@ 2022-07-21 22:00 ` Attila Kovacs
0 siblings, 0 replies; 7+ messages in thread
From: Attila Kovacs @ 2022-07-21 22:00 UTC (permalink / raw)
To: Steve Dickson, libtirpc-devel; +Cc: linux-nfs
Hi Steve,
Yes, the two patches affect the same code area, so if they are both
relative to master (which is how I submitted them originally) then they
conflict.
Here is the second patch as a clean incremental on the first.
-- A.
-----------------------------------------------------------------------
diff --git a/src/clnt_dg.c b/src/clnt_dg.c
index 7c5d22e..24c825b 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,11 +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);
- cu->cu_fd_lock->active = TRUE;
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);
@@ -603,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;
@@ -743,7 +747,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 3c73e65..7610b4a 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,7 +660,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);
-------------------------------------------------------------
On 7/21/22 17:38, Steve Dickson wrote:
> 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.
>
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v2 1/1]: Thread safe client destruction
2022-07-21 18:41 Thread safe client destruction Attila Kovacs
2022-07-21 18:45 ` Chuck Lever III
2022-07-21 21:38 ` Steve Dickson
@ 2022-07-22 10:49 ` Attila Kovacs
2 siblings, 0 replies; 7+ messages in thread
From: Attila Kovacs @ 2022-07-22 10:49 UTC (permalink / raw)
To: libtirpc-devel; +Cc: linux-nfs
Hi Steve et al.,
Correction to the last patch I submitted. That one unfortunately created
a new potential deadlock in itself while trying to fix a potential stack
corruption. Here is some more explanation of the bug the earlier patch
introduced and the how the updated patch gets around it:
clnt_*.c have a number of functions that wait on a condition variable to
be signaled. The signaling wakes up (at least) one of the threads
blocked on that condition, but not all of them. So, it is up to each
waiting call to cascade the signal to the next blocked thread, so they
can wake up as appropriate to process the notification.
Originally, all waiting calls checked for the 'active' condition to
decide whether to proceed. If there was an active operation ongoing, all
these blocked calls went back into waiting, without cascading the
signal. This was perfectly fine, because none of the calls could
potentially do anything while the client's lock was 'active'.
With the safe client destruction fix, however, the clnt_*_destroy() call
is now checking on a different condition (really it needs to check
simply if there are any pending operations waiting to complete before
the client can be closed). However, even if the destruction must wait,
another pending operation on the client could (and should) proceed
still, if there the lock is not 'active' during wake-up. Thus,
clnt_*_destroy() must cascade the notification to other blocked calls if
the 'active' state is not set before going back to waiting.
-------------------------------------------------------
diff --git a/src/clnt_dg.c b/src/clnt_dg.c
index 7c5d22e..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,11 +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);
- cu->cu_fd_lock->active = TRUE;
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);
@@ -603,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;
@@ -743,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);
}
-------------------------------------------------------
Signed-off-by: Attila Kovacs <attila.kovacs@cfa.harvard.edu>
On 7/21/22 14:41, 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.
>
> -- A.
^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-07-22 10:50 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-21 18:41 Thread safe client destruction Attila Kovacs
2022-07-21 18:45 ` Chuck Lever III
2022-07-21 19:19 ` Attila Kovacs
2022-07-21 20:37 ` Steve Dickson
2022-07-21 21:38 ` Steve Dickson
2022-07-21 22:00 ` Attila Kovacs
2022-07-22 10:49 ` [PATCH v2 1/1]: " Attila Kovacs
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.