lkml.org 
[lkml]   [2023]   [Jun]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    Subject[tip: timers/core] posix-timers: Ensure timer ID search-loop limit is valid
    The following commit has been merged into the timers/core branch of tip:

    Commit-ID: cec58cad4614f44b76624f8d62ef771c8725c483
    Gitweb: https://git.kernel.org/tip/cec58cad4614f44b76624f8d62ef771c8725c483
    Author: Thomas Gleixner <tglx@linutronix.de>
    AuthorDate: Thu, 01 Jun 2023 20:58:47 +02:00
    Committer: Thomas Gleixner <tglx@linutronix.de>
    CommitterDate: Mon, 05 Jun 2023 17:03:36 +02:00

    posix-timers: Ensure timer ID search-loop limit is valid

    posix_timer_add() tries to allocate a posix timer ID by starting from the
    cached ID which was stored by the last successful allocation.

    This is done in a loop searching the ID space for a free slot one by
    one. The loop has to terminate when the search wrapped around to the
    starting point.

    But that's racy vs. establishing the starting point. That is read out
    lockless, which leads to the following problem:

    CPU0 CPU1
    posix_timer_add()
    start = sig->posix_timer_id;
    lock(hash_lock);
    ... posix_timer_add()
    if (++sig->posix_timer_id < 0)
    start = sig->posix_timer_id;
    sig->posix_timer_id = 0;

    So CPU1 can observe a negative start value, i.e. -1, and the loop break
    never happens because the condition can never be true:

    if (sig->posix_timer_id == start)
    break;

    While this is unlikely to ever turn into an endless loop as the ID space is
    huge (INT_MAX), the racy read of the start value caught the attention of
    KCSAN and Dmitry unearthed that incorrectness.

    Rewrite it so that all id operations are under the hash lock.

    Reported-by: syzbot+5c54bd3eb218bb595aa9@syzkaller.appspotmail.com
    Reported-by: Dmitry Vyukov <dvyukov@google.com>
    Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
    Reviewed-by: Frederic Weisbecker <frederic@kernel.org>
    Link: https://lore.kernel.org/r/87bkhzdn6g.ffs@tglx

    ---
    include/linux/sched/signal.h | 2 +-
    kernel/time/posix-timers.c | 31 ++++++++++++++++++-------------
    2 files changed, 19 insertions(+), 14 deletions(-)

    diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h
    index 2009926..669e8cf 100644
    --- a/include/linux/sched/signal.h
    +++ b/include/linux/sched/signal.h
    @@ -135,7 +135,7 @@ struct signal_struct {
    #ifdef CONFIG_POSIX_TIMERS

    /* POSIX.1b Interval Timers */
    - int posix_timer_id;
    + unsigned int next_posix_timer_id;
    struct list_head posix_timers;

    /* ITIMER_REAL timer for the process */
    diff --git a/kernel/time/posix-timers.c b/kernel/time/posix-timers.c
    index 2d835c2..2c5daeb 100644
    --- a/kernel/time/posix-timers.c
    +++ b/kernel/time/posix-timers.c
    @@ -140,25 +140,30 @@ static struct k_itimer *posix_timer_by_id(timer_t id)
    static int posix_timer_add(struct k_itimer *timer)
    {
    struct signal_struct *sig = current->signal;
    - int first_free_id = sig->posix_timer_id;
    struct hlist_head *head;
    - int ret = -ENOENT;
    + unsigned int cnt, id;

    - do {
    + /*
    + * FIXME: Replace this by a per signal struct xarray once there is
    + * a plan to handle the resulting CRIU regression gracefully.
    + */
    + for (cnt = 0; cnt <= INT_MAX; cnt++) {
    spin_lock(&hash_lock);
    - head = &posix_timers_hashtable[hash(sig, sig->posix_timer_id)];
    - if (!__posix_timers_find(head, sig, sig->posix_timer_id)) {
    + id = sig->next_posix_timer_id;
    +
    + /* Write the next ID back. Clamp it to the positive space */
    + sig->next_posix_timer_id = (id + 1) & INT_MAX;
    +
    + head = &posix_timers_hashtable[hash(sig, id)];
    + if (!__posix_timers_find(head, sig, id)) {
    hlist_add_head_rcu(&timer->t_hash, head);
    - ret = sig->posix_timer_id;
    + spin_unlock(&hash_lock);
    + return id;
    }
    - if (++sig->posix_timer_id < 0)
    - sig->posix_timer_id = 0;
    - if ((sig->posix_timer_id == first_free_id) && (ret == -ENOENT))
    - /* Loop over all possible ids completed */
    - ret = -EAGAIN;
    spin_unlock(&hash_lock);
    - } while (ret == -ENOENT);
    - return ret;
    + }
    + /* POSIX return code when no timer ID could be allocated */
    + return -EAGAIN;
    }

    static inline void unlock_timer(struct k_itimer *timr, unsigned long flags)
    \
     
     \ /
      Last update: 2023-06-05 17:11    [W:3.201 / U:0.012 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site