URL: https://github.com/SSSD/sssd/pull/107 Title: #107: WATCHDOG: Avoid non async-signal-safe from the signal_handler

lslebodn commented: """ On (12/12/16 01:20), fidencio wrote:

While debugging rhbz#1396912 some deadlock on sssd_be was noticed[0] and it's been caused by the use of non async-signal-safe functions from the signal_handler (please, see man 7 signal for more info about which are the async-signal-safe functions that can be used).

The removal of those functions we lose some debug messages,

If these messages are important we can write them(static strings) to stderr or other file descriptor directly with write which can be safely called inside signal handler.

which is the least of problems now. Also instead of re-setting the watchdog timeout in case of time-shift (please, see b8ceaeb for more info), let's just re-set the timer's time.

Along with this commit, the function to teardown the watchdog has been removed as it's not used anymore.

Must not be removed.

One of the main usesaces is to disable watchdog for debugging with gdb. It is very usefull. You might add comment there why it should not be removed.

NOTE: I didn't read the patch. Just commenting commit message.

LS

"""

See the full comment at https://github.com/SSSD/sssd/pull/107#issuecomment-266418517

URL: https://github.com/SSSD/sssd/pull/107 Title: #107: WATCHDOG: Avoid non async-signal-safe from the signal_handler

fidencio commented: """ On Mon, Dec 12, 2016 at 1:50 PM, Pavel Březina notifications@github.com wrote:

*@pbrezina* commented on this pull request.

In src/util/util_watchdog.c https://github.com/SSSD/sssd/pull/107#pullrequestreview-12447634:

...

•     *
  

•     * As restarting the watchdog from the signal_handler may
  

•     * lead sssd_be to a deadlock due to the use of non async-
  

•     * signal-safe function. let's just (re)set the timer's
  

•     * time.
  

•     */
  

•    /* As done when setting up the watchdog handler,
  

•     * we give 1 second head start to the watchdog event */
  

•    its.it_value.tv_sec = watchdog_ctx.interval.tv_sec + 1;
  

•    its.it_value.tv_nsec = 0;
  

•    its.it_interval.tv_sec = watchdog_ctx.interval.tv_sec;
  

•    its.it_interval.tv_nsec = 0;
  

•    ret = timer_settime(watchdog_ctx.timerid, 0, &its, NULL);
  

•    if (ret == -1) {
  

•        _exit(1);
  
  

This is not sufficient because we need to reset the tevent timer. I think watchdog itself is actually fired in the right time because the timer clock is adjusted. The problematic timer is the tevent one, which needs to be rescheduled.

Okay. I guess I see what you mean. Even with this timeout being reset the watchdog would end up killing the process due to the tevent timeout, is it? Considering that's the case I'd say there's nothing much that can be done from our side apart from ignoring the ticks on those cases. At least till we have it changed/fixed on tevent's side. And suggestions are welcome!

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/SSSD/sssd/pull/107#pullrequestreview-12447634, or mute the thread https://github.com/notifications/unsubscribe-auth/AAG4ehq9DnuKDBXQwCWLtnxBLhHl90yUks5rHUMRgaJpZM4LKVED .

Thanks for the review! -- Fabiano Fidêncio

"""

See the full comment at https://github.com/SSSD/sssd/pull/107#issuecomment-266431337

URL: https://github.com/SSSD/sssd/pull/107 Title: #107: WATCHDOG: Avoid non async-signal-safe from the signal_handler

fidencio commented: """ On Mon, Dec 12, 2016 at 3:37 PM, Jakub Hrozek notifications@github.com wrote:

So..I wonder how much of a hack this is but if the intent here is to reset the watchdog from a signal handler, could we send a signal to self (kill(getpid(), SIGHUP)) with a signal that is handled by tevent and in that tevent-driven signal handler, reset the watchdog?

Things are getting more and more hacky. I'm wondering here, what would be the problem to simply `_exit(1)` in case of a timeshift is detected?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/SSSD/sssd/pull/107#issuecomment-266446340, or mute the thread https://github.com/notifications/unsubscribe-auth/AAG4enqr9bqncI7VM76uw-d4O9L1-INtks5rHVwdgaJpZM4LKVED .

"""

See the full comment at https://github.com/SSSD/sssd/pull/107#issuecomment-266447493

URL: https://github.com/SSSD/sssd/pull/107 Title: #107: WATCHDOG: Avoid non async-signal-safe from the signal_handler

simo5 commented: """ Yes we should ask, I think we really need to try to use monotonic clocks for most tasks. """

See the full comment at https://github.com/SSSD/sssd/pull/107#issuecomment-266450977

URL: https://github.com/SSSD/sssd/pull/107 Title: #107: WATCHDOG: Avoid non async-signal-safe from the signal_handler

simo5 commented: """ well you could have a globalk variable for the watchdog and change it from a custom signal handler, but the point of the watchdog is to go thorugh the tevent handler instead so that we are sure the machinery is working and not stuck somwhere. Resetting directly from the singal handler would bypass all processing and therefore render the watchdog useless I guess. """

See the full comment at https://github.com/SSSD/sssd/pull/107#issuecomment-266460639

URL: https://github.com/SSSD/sssd/pull/107 Author: fidencio Title: #107: WATCHDOG: Avoid non async-signal-safe from the signal_handler Action: synchronized

To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/107/head:pr107 git checkout pr107

URL: https://github.com/SSSD/sssd/pull/107 Title: #107: WATCHDOG: Avoid non async-signal-safe from the signal_handler

fidencio commented: """ Pavel,

On Tue, Dec 13, 2016 at 11:20 AM, Pavel Březina notifications@github.com wrote:

There are two scenarios:

1. timeshift during system boot -- it is very common to be several

hours 2. timeshift due to an ntp update when booted up -- usually only few seconds, not a big deal

The problem with tevent timer is that if we shift backwards the timer remains too far in the future. This applies to all timers, not only for the watchdog. Forward shift is not a problem, it just executes the timers immediately. Resetting the watchdog helps in a way that sssd is not killed, we don't have any capability to reschedule all timed event and we actually can not tell that sssd will be functioning properly (dyndns, sudo refresh, enumeration, domain refresh, even idle timer on socket activation)... all those operations that depends on time() would become unreliable.

I think the best thing to do would be restart the process (although the question is how would this affect the boot up) and patch tevent to deal with timeshift either by using monotonic clock or by detecting them and altering timers accordingly.

In the latest version of patch I've just called _exit(1) when the timeshift is detected. About patching tevent, I've seen some old discussions happening and it doesn't seem a trivial thing to do. Would the patch, as it is right now, be acceptable and then a work on tevent could be done later (yes, I'd add it to my queue and do it as soon as we have an agreement on doing this)?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/SSSD/sssd/pull/107#issuecomment-266700933, or mute the thread https://github.com/notifications/unsubscribe-auth/AAG4eiXYbcI_GV_YJeh4guURQkDwkfEZks5rHnF-gaJpZM4LKVED .

"""

See the full comment at https://github.com/SSSD/sssd/pull/107#issuecomment-266702069

URL: https://github.com/SSSD/sssd/pull/107 Title: #107: WATCHDOG: Avoid non async-signal-safe from the signal_handler

jhrozek commented: """ On Tue, Dec 13, 2016 at 02:44:44AM -0800, Simo Sorce wrote:

On Tue, 2016-12-13 at 02:25 -0800, fidencio wrote:

...

Pavel,

On Tue, Dec 13, 2016 at 11:20 AM, Pavel Březina notifications@github.com wrote:

...

There are two scenarios:

1. timeshift during system boot -- it is very common to be several

hours 2. timeshift due to an ntp update when booted up -- usually only few seconds, not a big deal

The problem with tevent timer is that if we shift backwards the timer remains too far in the future. This applies to all timers, not only for the watchdog. Forward shift is not a problem, it just executes the timers immediately. Resetting the watchdog helps in a way that sssd is not killed, we don't have any capability to reschedule all timed event and we actually can not tell that sssd will be functioning properly (dyndns, sudo refresh, enumeration, domain refresh, even idle timer on socket activation)... all those operations that depends on time() would become unreliable.

I think the best thing to do would be restart the process (although the question is how would this affect the boot up) and patch tevent to deal with timeshift either by using monotonic clock or by detecting them and altering timers accordingly.

In the latest version of patch I've just called _exit(1) when the timeshift is detected. About patching tevent, I've seen some old discussions happening and it doesn't seem a trivial thing to do. Would the patch, as it is right now, be acceptable and then a work on tevent could be done later (yes, I'd add it to my queue and do it as soon as we have an agreement on doing this)?

This is really a blunt tool (calling exit()), but until tevent can be fixed the only other option would be to use some wrapper to keep track of all existing timed events and cancel and restart them all if the clock changes abruptly.

that's why I suggested signaling self to a tevent-driven signal handler from where we can just set up the timer anew.

If there is any other way to 'break out' of the POSIX signal handler into somewhere where we can call tevent/talloc (or in general unsafe calls) I'm all ears.

The easiest option is to teach tevent about the existence of monotonic clocks and allow to set a default clock type to use as well as overrides for specific timers that want to fire on the actual time not just X seconds in the future.

Simo.

-- Simo Sorce * Red Hat, Inc * New York

-- You are receiving this because you commented. Reply to this email directly or view it on GitHub: https://github.com/SSSD/sssd/pull/107#issuecomment-266706120

"""

See the full comment at https://github.com/SSSD/sssd/pull/107#issuecomment-266745077

URL: https://github.com/SSSD/sssd/pull/107 Title: #107: WATCHDOG: Avoid non async-signal-safe from the signal_handler

jhrozek commented: """ On Tue, Dec 13, 2016 at 07:06:58AM -0800, Simo Sorce wrote:

On Tue, 2016-12-13 at 05:59 -0800, Jakub Hrozek wrote:

...

On Tue, Dec 13, 2016 at 02:44:44AM -0800, Simo Sorce wrote:

...

On Tue, 2016-12-13 at 02:25 -0800, fidencio wrote:

...

Pavel,

On Tue, Dec 13, 2016 at 11:20 AM, Pavel Březina notifications@github.com wrote:

...

There are two scenarios:

1. timeshift during system boot -- it is very common to be several

hours 2. timeshift due to an ntp update when booted up -- usually only few seconds, not a big deal

The problem with tevent timer is that if we shift backwards the timer remains too far in the future. This applies to all timers, not only for the watchdog. Forward shift is not a problem, it just executes the timers immediately. Resetting the watchdog helps in a way that sssd is not killed, we don't have any capability to reschedule all timed event and we actually can not tell that sssd will be functioning properly (dyndns, sudo refresh, enumeration, domain refresh, even idle timer on socket activation)... all those operations that depends on time() would become unreliable.

I think the best thing to do would be restart the process (although the question is how would this affect the boot up) and patch tevent to deal with timeshift either by using monotonic clock or by detecting them and altering timers accordingly.

In the latest version of patch I've just called _exit(1) when the timeshift is detected. About patching tevent, I've seen some old discussions happening and it doesn't seem a trivial thing to do. Would the patch, as it is right now, be acceptable and then a work on tevent could be done later (yes, I'd add it to my queue and do it as soon as we have an agreement on doing this)?

This is really a blunt tool (calling exit()), but until tevent can be fixed the only other option would be to use some wrapper to keep track of all existing timed events and cancel and restart them all if the clock changes abruptly.

that's why I suggested signaling self to a tevent-driven signal handler from where we can just set up the timer anew.

If there is any other way to 'break out' of the POSIX signal handler into somewhere where we can call tevent/talloc (or in general unsafe calls) I'm all ears.

I guess I need to understand better what exactly you want to do to be able to advice on something. I can think of a coulpe of options, none of them particularly elegant :)

OK, let me try to explain better.

A machine drifts time. Then an SSSD process receives SIGRT in watchdog_handler() and detects the time has drifted, so it avoids increasing the watchdog ticks counter -- this is done in watchdog_detect_timeshift() at the moment.

At that point, in the current master, we call teardown_watchdog() and setup_watchdog() to set a new watchdog (the part that is based on tevent timers). This is unsafe to do in a signal handler because it involves malloc and free among others called from tevent.

What I'm trying to figure out is how to reset the watchdog when I detect in watchdog_detect_timeshift() the time is out of sync and the tevent timer that resets the ticks will not arrive until the sssd process receives enough SIGRT signals to get itself killed.

Does the question make sense now?

"""

See the full comment at https://github.com/SSSD/sssd/pull/107#issuecomment-266778681

URL: https://github.com/SSSD/sssd/pull/107 Title: #107: WATCHDOG: Avoid non async-signal-safe from the signal_handler

jhrozek commented: """ On Tue, Dec 13, 2016 at 08:16:33AM -0800, Simo Sorce wrote:

On Tue, 2016-12-13 at 08:02 -0800, Jakub Hrozek wrote:

...

On Tue, Dec 13, 2016 at 07:06:58AM -0800, Simo Sorce wrote:

...

On Tue, 2016-12-13 at 05:59 -0800, Jakub Hrozek wrote:

...

On Tue, Dec 13, 2016 at 02:44:44AM -0800, Simo Sorce wrote:

...

On Tue, 2016-12-13 at 02:25 -0800, fidencio wrote:

...

Pavel,

On Tue, Dec 13, 2016 at 11:20 AM, Pavel Březina notifications@github.com wrote:

> There are two scenarios: > > 1. timeshift during system boot -- it is very common to be several > hours > 2. timeshift due to an ntp update when booted up -- usually only few > seconds, not a big deal > > The problem with tevent timer is that if we shift backwards the timer > remains too far in the future. This applies to all timers, not only for the > watchdog. Forward shift is not a problem, it just executes the timers > immediately. Resetting the watchdog helps in a way that sssd is not killed, > we don't have any capability to reschedule all timed event and we actually > can not tell that sssd will be functioning properly (dyndns, sudo refresh, > enumeration, domain refresh, even idle timer on socket activation)... all > those operations that depends on time() would become unreliable. > > I think the best thing to do would be restart the process (although the > question is how would this affect the boot up) and patch tevent to deal > with timeshift either by using monotonic clock or by detecting them and > altering timers accordingly. >

In the latest version of patch I've just called _exit(1) when the timeshift is detected. About patching tevent, I've seen some old discussions happening and it doesn't seem a trivial thing to do. Would the patch, as it is right now, be acceptable and then a work on tevent could be done later (yes, I'd add it to my queue and do it as soon as we have an agreement on doing this)?

This is really a blunt tool (calling exit()), but until tevent can be fixed the only other option would be to use some wrapper to keep track of all existing timed events and cancel and restart them all if the clock changes abruptly.

that's why I suggested signaling self to a tevent-driven signal handler from where we can just set up the timer anew.

If there is any other way to 'break out' of the POSIX signal handler into somewhere where we can call tevent/talloc (or in general unsafe calls) I'm all ears.

I guess I need to understand better what exactly you want to do to be able to advice on something. I can think of a coulpe of options, none of them particularly elegant :)

OK, let me try to explain better.

A machine drifts time. Then an SSSD process receives SIGRT in watchdog_handler() and detects the time has drifted, so it avoids increasing the watchdog ticks counter -- this is done in watchdog_detect_timeshift() at the moment.

At that point, in the current master, we call teardown_watchdog() and setup_watchdog() to set a new watchdog (the part that is based on tevent timers). This is unsafe to do in a signal handler because it involves malloc and free among others called from tevent.

What I'm trying to figure out is how to reset the watchdog when I detect in watchdog_detect_timeshift() the time is out of sync and the tevent timer that resets the ticks will not arrive until the sssd process receives enough SIGRT signals to get itself killed.

Does the question make sense now?

Yes, I see a few ways, a file descriptor (like tevent does for handling signals) used to fire a tevent_fd event that will perform these actions. Or a global variable, that is checked in the main loop at idle times and resets the watchdog, finally a mutex on which a dedicated thread waits, the thread only job is to run the reset if the mutex is released (but then the mutex needs to be re-acquired, probably on the next tick), but then again not sure if the code run in the setup_watchdog() is thread safe.

Thank you.

Without doing any coding, I like the idea of the fd the best.

"""

See the full comment at https://github.com/SSSD/sssd/pull/107#issuecomment-266785347

URL: https://github.com/SSSD/sssd/pull/107 Title: #107: WATCHDOG: Avoid non async-signal-safe from the signal_handler

Label: +Changes requested

URL: https://github.com/SSSD/sssd/pull/107 Author: fidencio Title: #107: WATCHDOG: Avoid non async-signal-safe from the signal_handler Action: synchronized

To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/107/head:pr107 git checkout pr107

URL: https://github.com/SSSD/sssd/pull/107 Title: #107: WATCHDOG: Avoid non async-signal-safe from the signal_handler

Label: -Changes requested

URL: https://github.com/SSSD/sssd/pull/107 Title: #107: WATCHDOG: Avoid non async-signal-safe from the signal_handler

jhrozek commented: """ Hmm, I'm sorry, I think this is my fault for suggesting we move more stuff out of the POSIX signal handler, but I don't think we can remove the watchdog overflow. Moving the watchdog overflow out of the signal handler and into the tevent loop would effectively disable watchdog, because if sssd is really blocked, the tevent fd handler would never be called and the watchdog would never kill the process..

So I think the only part that should be moved is the re-initializing of the watchdog in case we detect a timeshift..

btw I only realized this when I tried to test that watchdog still works. I set a ridiculously short timeout (2) and a breakpoint in sdap_save_user. I only saw SIGRT incoming, but the process was never killed. """

See the full comment at https://github.com/SSSD/sssd/pull/107#issuecomment-268495109

URL: https://github.com/SSSD/sssd/pull/107 Author: fidencio Title: #107: WATCHDOG: Avoid non async-signal-safe from the signal_handler Action: synchronized

To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/107/head:pr107 git checkout pr107

URL: https://github.com/SSSD/sssd/pull/107 Title: #107: WATCHDOG: Avoid non async-signal-safe from the signal_handler

fidencio commented: """ Okay, patch has been updated. """

See the full comment at https://github.com/SSSD/sssd/pull/107#issuecomment-268503583

URL: https://github.com/SSSD/sssd/pull/107 Title: #107: WATCHDOG: Avoid non async-signal-safe from the signal_handler

lslebodn commented: """ retest this please """

See the full comment at https://github.com/SSSD/sssd/pull/107#issuecomment-269941038

URL: https://github.com/SSSD/sssd/pull/107 Title: #107: WATCHDOG: Avoid non async-signal-safe from the signal_handler

lslebodn commented: """ On (02/01/17 06:15), fidencio wrote:

On Mon, Jan 2, 2017 at 8:35 AM, lslebodn notifications@github.com wrote:

...

retest this please

Is this comment for the author of the patch or is this comment for the reviewer of the patch?

It is a command for jenkins plugin to retest a PR.

https://github.com/jenkinsci/ghprb-plugin/blob/master/README.md https://wiki.jenkins-ci.org/display/JENKINS/Github+pull+request+builder+plug...

There seems to be some issues with epel repository and therefore installation of packages failed https://ci.centos.org/job/sssd-CentOS6/260/console

LS

"""

See the full comment at https://github.com/SSSD/sssd/pull/107#issuecomment-269985115

URL: https://github.com/SSSD/sssd/pull/107 Author: fidencio Title: #107: WATCHDOG: Avoid non async-signal-safe from the signal_handler Action: synchronized

To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/107/head:pr107 git checkout pr107

URL: https://github.com/SSSD/sssd/pull/107 Title: #107: WATCHDOG: Avoid non async-signal-safe from the signal_handler

jhrozek commented: """ hmm, it seems I was wrong and at least with systemd (is that the difference?) when we kill the whole process group also the nss and pam responders (that were started explicitly) are killed.

So I was wrong. Can you please retest if you see the same? But in general I think we can stick to the previous version, sorry about guiding you down the wrong path. """

See the full comment at https://github.com/SSSD/sssd/pull/107#issuecomment-270622372

URL: https://github.com/SSSD/sssd/pull/107 Author: fidencio Title: #107: WATCHDOG: Avoid non async-signal-safe from the signal_handler Action: synchronized

To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/107/head:pr107 git checkout pr107

URL: https://github.com/SSSD/sssd/pull/107 Title: #107: WATCHDOG: Avoid non async-signal-safe from the signal_handler

Label: +Accepted

URL: https://github.com/SSSD/sssd/pull/107 Title: #107: WATCHDOG: Avoid non async-signal-safe from the signal_handler

jhrozek commented: """ btw now I'm wondering if the setpgrp should be a separate patch also for stable branches because I guess the bug was present in sssd for quite a long time? """

See the full comment at https://github.com/SSSD/sssd/pull/107#issuecomment-270637274

URL: https://github.com/SSSD/sssd/pull/107 Title: #107: WATCHDOG: Avoid non async-signal-safe from the signal_handler

fidencio commented: """ On Thu, Jan 5, 2017 at 2:03 PM, lslebodn notifications@github.com wrote:

On (05/01/17 04:52), Jakub Hrozek wrote:

...

btw now I'm wondering if the setpgrp should be a separate patch also for

stable branches because I guess the bug was present in sssd for quite a long time?

...

sure; Should Fabiano prepare new PR or it can be in this one. Because it's related.

As the patch should come before this one I'd prefer to have both in this very same PR.

Best Regards, -- Fabiano Fidêncio

"""

See the full comment at https://github.com/SSSD/sssd/pull/107#issuecomment-270639914

URL: https://github.com/SSSD/sssd/pull/107 Title: #107: WATCHDOG: Avoid non async-signal-safe from the signal_handler

Label: +Changes requested

URL: https://github.com/SSSD/sssd/pull/107 Author: fidencio Title: #107: WATCHDOG: Avoid non async-signal-safe from the signal_handler Action: synchronized

To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/107/head:pr107 git checkout pr107

URL: https://github.com/SSSD/sssd/pull/107 Title: #107: WATCHDOG: Avoid non async-signal-safe from the signal_handler

Label: -Changes requested

URL: https://github.com/SSSD/sssd/pull/107 Author: fidencio Title: #107: WATCHDOG: Avoid non async-signal-safe from the signal_handler Action: synchronized

To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/107/head:pr107 git checkout pr107

URL: https://github.com/SSSD/sssd/pull/107 Title: #107: WATCHDOG: Avoid non async-signal-safe from the signal_handler

fidencio commented: """ Patchset updated! """

See the full comment at https://github.com/SSSD/sssd/pull/107#issuecomment-271558289

URL: https://github.com/SSSD/sssd/pull/107 Title: #107: WATCHDOG: Avoid non async-signal-safe from the signal_handler

jhrozek commented: """ To me as well. I tested again the watchdog restart and the timeshift and both cases work fine. """

See the full comment at https://github.com/SSSD/sssd/pull/107#issuecomment-272409701

URL: https://github.com/SSSD/sssd/pull/107 Title: #107: WATCHDOG: Avoid non async-signal-safe from the signal_handler

lslebodn commented: """ master: * e6a5f8c58539fc31fd81fac89cfc85703b4250ea * 087162b85e191af51637904702813969b35eaadc

sssd-1-14: * 0606a71b698c4acf954ba7284e62acbd0aa5e52d * 442985a7af2262fab57f56c7a8cd40af10081610

LS

"""

See the full comment at https://github.com/SSSD/sssd/pull/107#issuecomment-275086247

URL: https://github.com/SSSD/sssd/pull/107 Author: fidencio Title: #107: WATCHDOG: Avoid non async-signal-safe from the signal_handler Action: closed

To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/107/head:pr107 git checkout pr107