[sssd PR#46][opened] sss_client: Defer thread cancellation until completion of nss/pam operations
URL: https://github.com/SSSD/sssd/pull/46 Author: HouzuoGuo Title: #46: sss_client: Defer thread cancellation until completion of nss/pam operations Action: opened
PR body: """ https://fedorahosted.org/sssd/ticket/3156
The client code is not cancellation-safe, an application which has cancelled an NSS operation will experience subtle bugs, hence thread cancellation is deferred until completion of client operations. """
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/46/head:pr46 git checkout pr46
URL: https://github.com/SSSD/sssd/pull/46 Title: #46: sss_client: Defer thread cancellation until completion of nss/pam operations
centos-ci commented: """ Can one of the admins verify this patch? """
See the full comment at https://github.com/SSSD/sssd/pull/46#issuecomment-252853017
URL: https://github.com/SSSD/sssd/pull/46 Title: #46: sss_client: Defer thread cancellation until completion of nss/pam operations
centos-ci commented: """ Can one of the admins verify this patch? """
See the full comment at https://github.com/SSSD/sssd/pull/46#issuecomment-252853023
URL: https://github.com/SSSD/sssd/pull/46 Title: #46: sss_client: Defer thread cancellation until completion of nss/pam operations
jhrozek commented: """ ok to test """
See the full comment at https://github.com/SSSD/sssd/pull/46#issuecomment-252854747
URL: https://github.com/SSSD/sssd/pull/46 Title: #46: sss_client: Defer thread cancellation until completion of nss/pam operations
sumit-bose commented: """ Here is the original reproducer/test program from https://fedorahosted.org/sssd/ticket/1460 written by Fedora user werkt (I had to gzip it so that github likes it). I added a few fixes to remove some compiler warnings.
I verified that if the program was compiled without -DDISABLE_CANCEL it gets stuck with the olsdSSSD code without robust mutexes. With the new patch the reproducer works in both cases as expected. [sss_client_hang.c.gz](https://github.com/SSSD/sssd/files/555662/sss_client_hang.c.gz)
"""
See the full comment at https://github.com/SSSD/sssd/pull/46#issuecomment-256621283
URL: https://github.com/SSSD/sssd/pull/46 Title: #46: sss_client: Defer thread cancellation until completion of nss/pam operations
sumit-bose commented: """ Here is the original reproducer/test program from https://fedorahosted.org/sssd/ticket/1460 written by Fedora user werkt (I had to gzip it so that github likes it). I added a few fixes to remove some compiler warnings.
I verified that if the program was compiled without -DDISABLE_CANCEL it gets stuck with the old SSSD code without robust mutexes. With the new patch the reproducer works in both cases as expected. [sss_client_hang.c.gz](https://github.com/SSSD/sssd/files/555662/sss_client_hang.c.gz)
"""
See the full comment at https://github.com/SSSD/sssd/pull/46#issuecomment-256621283
URL: https://github.com/SSSD/sssd/pull/46 Title: #46: sss_client: Defer thread cancellation until completion of nss/pam operations
sumit-bose commented: """ The patch looks good and works as expected and removes the dependency to libpthread.
If I understand the it correctly there is a small functional difference between the old version with a robust mutex and the new pthread_setcancelstate() way. The mutex protects the communication between the client and SSSD's responders so that so that a single thread can send a request and wait for the response before another thread can send a different request. Since some requests, like e.g. getrgnam() for a group with many members and maybe even nested groups on the server-side, might need some time to make all data available there is quite a long timeout of 300s. So in the worst case a single thread can keep the mutex for 5 minutes.
With a robust mutex it might be possible for a master-thread to cancel the thread blocking the access to SSSD earlier so that other threads can continue and send requests to SSSD. If cancellation is disabled for the thread the application might be block for 5 minutes before anything can proceed. I'm not sure how many applications would be really affected by this. Additionally @fweimer 's comments in https://bugzilla.redhat.com/show_bug.cgi?id=1369130#c4 are still valid (although I guess in nearly all cases the cancellation would happen when the client is waiting for a response from SSSD).
I wonder if it would makes sense to add a configure option to select which code is used and make the pthread_setcancelstate() way the default. This makes it easy to switch to the robust mutexes and if there are no issue for some time the old code and the configure option can be removed.
On the other hand the magic of git makes it possible as well to recover the old code with reasonable effort and would avoid a new configure option and a code-path which is typically not tested in the default installation. @lslebodn, @jhrozek, any preference? """
See the full comment at https://github.com/SSSD/sssd/pull/46#issuecomment-256887672
URL: https://github.com/SSSD/sssd/pull/46 Title: #46: sss_client: Defer thread cancellation until completion of nss/pam operations
jhrozek commented: """ I wonder if @fweimer thinks it's worth merging the patch if the issue in https://bugzilla.redhat.com/show_bug.cgi?id=1369130#c4 still persists or if we should work a more on the patchset.
I like the idea about conditional build. Since we could add at least some tests using nss_wrapper, I wonder if this could be the way forward? """
See the full comment at https://github.com/SSSD/sssd/pull/46#issuecomment-257277989
URL: https://github.com/SSSD/sssd/pull/46 Title: #46: sss_client: Defer thread cancellation until completion of nss/pam operations
lslebodn commented: """ On (28/10/16 03:24), sumit-bose wrote:
On the other hand the magic of git makes it possible as well to recover the old code with reasonable effort and would avoid a new configure option and a code-path which is typically not tested in the default installation. @lslebodn, @jhrozek, any preference?
I would prefer git revert in case of bugs.
LS
"""
See the full comment at https://github.com/SSSD/sssd/pull/46#issuecomment-258475330
URL: https://github.com/SSSD/sssd/pull/46 Title: #46: sss_client: Defer thread cancellation until completion of nss/pam operations
sumit-bose commented: """ Looks lie a tie, @HouzuoGuo do you have any preference? """
See the full comment at https://github.com/SSSD/sssd/pull/46#issuecomment-258478904
URL: https://github.com/SSSD/sssd/pull/46 Title: #46: sss_client: Defer thread cancellation until completion of nss/pam operations
jhrozek commented: """ Well, if the old code would be disabled and there would be nobody using the new config option from the start, I'm fine with switching as well. """
See the full comment at https://github.com/SSSD/sssd/pull/46#issuecomment-258679368
URL: https://github.com/SSSD/sssd/pull/46 Title: #46: sss_client: Defer thread cancellation until completion of nss/pam operations
HouzuoGuo commented: """ Hello! I don't have a strong preference whether to retain the original behaviour in a compilation conditional or not, feel free to go ahead with your decision. """
See the full comment at https://github.com/SSSD/sssd/pull/46#issuecomment-258776215
URL: https://github.com/SSSD/sssd/pull/46 Title: #46: sss_client: Defer thread cancellation until completion of nss/pam operations
sumit-bose commented: """ Ok, ACK then. """
See the full comment at https://github.com/SSSD/sssd/pull/46#issuecomment-258806379
URL: https://github.com/SSSD/sssd/pull/46 Title: #46: sss_client: Defer thread cancellation until completion of nss/pam operations
Label: +Accepted
URL: https://github.com/SSSD/sssd/pull/46 Title: #46: sss_client: Defer thread cancellation until completion of nss/pam operations
Label: -Accepted
URL: https://github.com/SSSD/sssd/pull/46 Author: HouzuoGuo Title: #46: sss_client: Defer thread cancellation until completion of nss/pam operations Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/46/head:pr46 git checkout pr46
URL: https://github.com/SSSD/sssd/pull/46 Title: #46: sss_client: Defer thread cancellation until completion of nss/pam operations
HouzuoGuo commented: """ All right, `once` is now gone and mutex is now guarding cancellation state. """
See the full comment at https://github.com/SSSD/sssd/pull/46#issuecomment-259411671
URL: https://github.com/SSSD/sssd/pull/46 Title: #46: sss_client: Defer thread cancellation until completion of nss/pam operations
lslebodn commented: """ I asked Florian on IRC and he is fine with the last version. @sumit-bose do you have other comments; can we push it? """
See the full comment at https://github.com/SSSD/sssd/pull/46#issuecomment-262520348
URL: https://github.com/SSSD/sssd/pull/46 Title: #46: sss_client: Defer thread cancellation until completion of nss/pam operations
sumit-bose commented: """ I'm fine with the patch as well. Shall I re-run the test with the original reproducer or did you already run it? """
See the full comment at https://github.com/SSSD/sssd/pull/46#issuecomment-262525503
URL: https://github.com/SSSD/sssd/pull/46 Title: #46: sss_client: Defer thread cancellation until completion of nss/pam operations
sumit-bose commented: """ Testing went fine, but there is a "declared 'static' but never defined" issue. If this patch can be push with
diff --git a/src/sss_client/common.c b/src/sss_client/common.c index 676a1ba..b7a5ed7 100644 --- a/src/sss_client/common.c +++ b/src/sss_client/common.c @@ -1073,8 +1073,6 @@ struct sss_mutex { int old_cancel_state; };
-static void sss_nss_mc_mt_init(void); - static struct sss_mutex sss_nss_mtx = { .mtx = PTHREAD_MUTEX_INITIALIZER };
static struct sss_mutex sss_pam_mtx = { .mtx = PTHREAD_MUTEX_INITIALIZER };
It would be an ACK from me. """
See the full comment at https://github.com/SSSD/sssd/pull/46#issuecomment-262555532
URL: https://github.com/SSSD/sssd/pull/46 Author: HouzuoGuo Title: #46: sss_client: Defer thread cancellation until completion of nss/pam operations Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/46/head:pr46 git checkout pr46
URL: https://github.com/SSSD/sssd/pull/46 Author: HouzuoGuo Title: #46: sss_client: Defer thread cancellation until completion of nss/pam operations Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/46/head:pr46 git checkout pr46
URL: https://github.com/SSSD/sssd/pull/46 Title: #46: sss_client: Defer thread cancellation until completion of nss/pam operations
Label: +Pushed
URL: https://github.com/SSSD/sssd/pull/46 Title: #46: sss_client: Defer thread cancellation until completion of nss/pam operations
lslebodn commented: """ master: * d2f93542650c2f9613043acfa8e2f368972a70cd """
See the full comment at https://github.com/SSSD/sssd/pull/46#issuecomment-262760584
URL: https://github.com/SSSD/sssd/pull/46 Author: HouzuoGuo Title: #46: sss_client: Defer thread cancellation until completion of nss/pam operations Action: closed
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/46/head:pr46 git checkout pr46
sssd-devel@lists.fedorahosted.org
-
fidencio