Skip to content
Snippets Groups Projects

Deadlock Fixes

Merged Ghost User requested to merge deadlock-fixes into master

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
3076 3061 }
3077 3062
3078 3063 rcu_read_unlock();
3064
3065 // Schedule next zone signing
3066 zones_cancel_dnssec(zone);
  • Author Contributor

    You can do what you described in the comment here. Just move the event cancellation to the beginning of the block so you can be sure it won't be triggered during the execution. Also, there is another problem (probably the crash) I see the zones_dnssec_ev free the old pointer and so does the zones_cancel_dnssec. You should reuse the event and free it only when finally terminated because following may happen:

    (evsched, end of the function)zones_dnssec_ev:
    evsched_event_free(event->parent, event); # zd->dnssec_timer is now freed
    zd->dnssec_timer = NULL; # zd->dnssec_timer is now NULL
    zones_schedule_dnssec() # zd->dnssec_timer is now valid event
    
    (some other thread)zones_cancel_dnssec:
    evsched_cancel(scheduler, zd->dnssec_timer); # zd->dnssec_timer might be freed (crash), NULL (noop), valid (okay)
    evsched_event_free(scheduler, zd->dnssec_timer); # the event now cancelled, but it might have made it to reschedule so zd->dnssec_timer might be something else than was cancelled (so we've just freed a running event without cancellation)
    zd->dnssec_timer = NULL;

    If you reuse the same event, the worst thing that could happen is that you attempted to cancel it, but it got rescheduled. This is easily identified by evsched_cancel() that could be made to return different value (evsched.c:328) if it couldn't cancel a running event. If it couldn't cancel a running event, then you would need to call evsched_cancel() again until it returns positive result.

  • Author Contributor

    The first part of this comment I understand - good idea, I'll do that. As for the second part - that was exactly why I wanted to lock these operations using the zd lock. The crash was something else though, probably not related.

  • Jan Včelák
    Jan Včelák @jvcelak started a thread on commit 74ef199a
  • 2615 2613 evsched_t *scheduler = zd->server->sched;
    2616 2614
    2617 2615 if (zd->dnssec_timer) {
    2618 evsched_cancel(scheduler, zd->dnssec_timer);
    2619 evsched_event_free(scheduler, zd->dnssec_timer);
    2620 zd->dnssec_timer = NULL;
    2616 while (evsched_cancel(scheduler, zd->dnssec_timer) < 0) {
    • This will cause infinite loop if invalid parameters are supplied to evsched_cancel. :thumbsdown: I think evsched_cancel should return a different result code for error and for rescheduled events. And the loop should checked just for the one error code.

    • Author Contributor

      At this point, we are sure that the parameters are fine. I'll slap few asserts there to be sure. I think the evsched_cancel function should not return until the event, and the event planned by the original event are both cancelled. because I am certainly against checking for -2 or something like that (since evsched.c does not use Knot error codes).

    • Author Contributor

      I've created a new API function in evsched, that handles the planned events as well, @mvavrusa please review and change it if needed. It's 469e5086

    • I'm OK with asserts.

  • Ghost User
    Ghost User @ghost started a thread on commit 104efd92
  • 214 212
    215 213 /* Unlock calendar, this shouldn't happen. */
    216 214 pthread_mutex_unlock(&s->mx);
    217 return 0;
    215 return NULL;
    218 216
    219 217 }
    220 218
    221 219 int evsched_event_finished(evsched_t *s)
    222 220 {
    223 221 if (!s) {
    224 return -1;
    222 return KNOT_EINVAL;
  • Please register or sign in to reply
    Loading