| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CANrsvRM7cwPj_6yziN-o20kg9UL=tfeykqSZLs1vn3j+Y5y_cA@mail.gmail.com>
Date: Wed, 30 Aug 2017 01:02:39 +0900
From: Byungchul Park <max.byungchul.park@...il.com>
To: Peter Zijlstra <peterz@...radead.org>
Cc: Byungchul Park <byungchul.park@....com>,
Ingo Molnar <mingo@...nel.org>, Tejun Heo <tj@...nel.org>,
Boqun Feng <boqun.feng@...il.com>, david@...morbit.com,
johannes@...solutions.net, oleg@...hat.com,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
kernel-team@....com
Subject: Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation
On Tue, Aug 29, 2017 at 5:59 PM, Peter Zijlstra <peterz@...radead.org> wrote:
> On Fri, Aug 25, 2017 at 10:11:14AM +0900, Byungchul Park wrote:
>> I meant, this seems to be led from your mis-understanding of
>> crossrelease_hist_{start, end}().
>
> I have, several times now, explained why PROC is special.
I rather have explained why it's not, more times than you did, and you
have not read my explanation. Anyway, I am seriously curious about
why. Of course, I remember you said "PROC is special", but not _why_.
I really want to know _why_ PROC(=each work) should be handled
differently from others. Please show me an example except wq case
where you just tried to avoid problems than fix them.
> You seem to still think it can be used like the soft/hard-irq ones, this
> is fundamentally not so.
I wonder why, seriously.
>
> Does something like so help?
>
> ---
> Subject: lockdep: Untangle xhlock history save/restore from task independence
>
> Where XHLOCK_{SOFT,HARD} are save/restore points in the xhlocks[] to
> ensure the temporal IRQ events don't interact with task state, the
> XHLOCK_PROC is a fundament different beast that just happens to share
> the interface.
>
> The purpose of XHLOCK_PROC is to annotate independent execution inside
> one task. For example workqueues, each work should appear to run in its
> own 'pristine' 'task'.
>
> Remove XHLOCK_PROC in favour of its own interface to avoid confusion.
>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@...radead.org>
> ---
> include/linux/irqflags.h | 4 +--
> include/linux/lockdep.h | 7 +++--
> kernel/locking/lockdep.c | 79 +++++++++++++++++++++++-------------------------
> kernel/workqueue.c | 9 +++---
> 4 files changed, 48 insertions(+), 51 deletions(-)
>
> diff --git a/include/linux/irqflags.h b/include/linux/irqflags.h
> index 9bc050bc81b2..5fdd93bb9300 100644
> --- a/include/linux/irqflags.h
> +++ b/include/linux/irqflags.h
> @@ -26,7 +26,7 @@
> # define trace_hardirq_enter() \
> do { \
> current->hardirq_context++; \
> - crossrelease_hist_start(XHLOCK_HARD, 0);\
> + crossrelease_hist_start(XHLOCK_HARD); \
> } while (0)
> # define trace_hardirq_exit() \
> do { \
> @@ -36,7 +36,7 @@ do { \
> # define lockdep_softirq_enter() \
> do { \
> current->softirq_context++; \
> - crossrelease_hist_start(XHLOCK_SOFT, 0);\
> + crossrelease_hist_start(XHLOCK_SOFT); \
> } while (0)
> # define lockdep_softirq_exit() \
> do { \
> diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
> index 78bb7133abed..bfa8e0b0d6f1 100644
> --- a/include/linux/lockdep.h
> +++ b/include/linux/lockdep.h
> @@ -551,7 +551,6 @@ struct pin_cookie { };
> enum xhlock_context_t {
> XHLOCK_HARD,
> XHLOCK_SOFT,
> - XHLOCK_PROC,
> XHLOCK_CTX_NR,
> };
>
> @@ -580,8 +579,9 @@ extern void lock_commit_crosslock(struct lockdep_map *lock);
> #define STATIC_LOCKDEP_MAP_INIT(_name, _key) \
> { .name = (_name), .key = (void *)(_key), .cross = 0, }
>
> -extern void crossrelease_hist_start(enum xhlock_context_t c, bool force);
> +extern void crossrelease_hist_start(enum xhlock_context_t c);
> extern void crossrelease_hist_end(enum xhlock_context_t c);
> +extern void lockdep_invariant_state(bool force);
> extern void lockdep_init_task(struct task_struct *task);
> extern void lockdep_free_task(struct task_struct *task);
> #else /* !CROSSRELEASE */
> @@ -593,8 +593,9 @@ extern void lockdep_free_task(struct task_struct *task);
> #define STATIC_LOCKDEP_MAP_INIT(_name, _key) \
> { .name = (_name), .key = (void *)(_key), }
>
> -static inline void crossrelease_hist_start(enum xhlock_context_t c, bool force) {}
> +static inline void crossrelease_hist_start(enum xhlock_context_t c) {}
> static inline void crossrelease_hist_end(enum xhlock_context_t c) {}
> +static inline void lockdep_invariant_state(bool force) {}
> static inline void lockdep_init_task(struct task_struct *task) {}
> static inline void lockdep_free_task(struct task_struct *task) {}
> #endif /* CROSSRELEASE */
> diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
> index f73ca595b81e..44c8d0d17170 100644
> --- a/kernel/locking/lockdep.c
> +++ b/kernel/locking/lockdep.c
> @@ -4623,13 +4623,8 @@ asmlinkage __visible void lockdep_sys_exit(void)
> /*
> * The lock history for each syscall should be independent. So wipe the
> * slate clean on return to userspace.
> - *
> - * crossrelease_hist_end() works well here even when getting here
> - * without starting (i.e. just after forking), because it rolls back
> - * the index to point to the last entry, which is already invalid.
> */
> - crossrelease_hist_end(XHLOCK_PROC);
> - crossrelease_hist_start(XHLOCK_PROC, false);
> + lockdep_invariant_state(false);
> }
>
> void lockdep_rcu_suspicious(const char *file, const int line, const char *s)
> @@ -4723,19 +4718,47 @@ static inline void invalidate_xhlock(struct hist_lock *xhlock)
> }
>
> /*
> - * Lock history stacks; we have 3 nested lock history stacks:
> + * Lock history stacks; we have 2 nested lock history stacks:
> *
> * HARD(IRQ)
> * SOFT(IRQ)
> - * PROC(ess)
> *
> * The thing is that once we complete a HARD/SOFT IRQ the future task locks
> * should not depend on any of the locks observed while running the IRQ. So
> * what we do is rewind the history buffer and erase all our knowledge of that
> * temporal event.
> - *
> - * The PROCess one is special though; it is used to annotate independence
> - * inside a task.
> + */
> +
> +void crossrelease_hist_start(enum xhlock_context_t c)
> +{
> + struct task_struct *cur = current;
> +
> + if (!cur->xhlocks)
> + return;
> +
> + cur->xhlock_idx_hist[c] = cur->xhlock_idx;
> + cur->hist_id_save[c] = cur->hist_id;
> +}
> +
> +void crossrelease_hist_end(enum xhlock_context_t c)
> +{
> + struct task_struct *cur = current;
> +
> + if (cur->xhlocks) {
> + unsigned int idx = cur->xhlock_idx_hist[c];
> + struct hist_lock *h = &xhlock(idx);
> +
> + cur->xhlock_idx = idx;
> +
> + /* Check if the ring was overwritten. */
> + if (h->hist_id != cur->hist_id_save[c])
> + invalidate_xhlock(h);
> + }
> +}
> +
> +/*
> + * lockdep_invariant_state() is used to annotate independence inside a task, to
> + * make one task look like multiple independent 'tasks'.
> *
> * Take for instance workqueues; each work is independent of the last. The
> * completion of a future work does not depend on the completion of a past work
> @@ -4758,40 +4781,14 @@ static inline void invalidate_xhlock(struct hist_lock *xhlock)
> * entry. Similarly, independence per-definition means it does not depend on
> * prior state.
> */
> -void crossrelease_hist_start(enum xhlock_context_t c, bool force)
> +void lockdep_invariant_state(bool force)
> {
> - struct task_struct *cur = current;
> -
> - if (!cur->xhlocks)
> - return;
> -
> /*
> * We call this at an invariant point, no current state, no history.
> + * Verify the former, enforce the latter.
> */
> - if (c == XHLOCK_PROC) {
> - /* verified the former, ensure the latter */
> - WARN_ON_ONCE(!force && cur->lockdep_depth);
> - invalidate_xhlock(&xhlock(cur->xhlock_idx));
> - }
> -
> - cur->xhlock_idx_hist[c] = cur->xhlock_idx;
> - cur->hist_id_save[c] = cur->hist_id;
> -}
> -
> -void crossrelease_hist_end(enum xhlock_context_t c)
> -{
> - struct task_struct *cur = current;
> -
> - if (cur->xhlocks) {
> - unsigned int idx = cur->xhlock_idx_hist[c];
> - struct hist_lock *h = &xhlock(idx);
> -
> - cur->xhlock_idx = idx;
> -
> - /* Check if the ring was overwritten. */
> - if (h->hist_id != cur->hist_id_save[c])
> - invalidate_xhlock(h);
> - }
> + WARN_ON_ONCE(!force && current->lockdep_depth);
> + invalidate_xhlock(&xhlock(current->xhlock_idx));
> }
>
> static int cross_lock(struct lockdep_map *lock)
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index c0331891dec1..ab3c0dc8c7ed 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -2094,8 +2094,8 @@ __acquires(&pool->lock)
> lock_map_acquire(&pwq->wq->lockdep_map);
> lock_map_acquire(&lockdep_map);
> /*
> - * Strictly speaking we should do start(PROC) without holding any
> - * locks, that is, before these two lock_map_acquire()'s.
> + * Strictly speaking we should mark the invariant state without holding
> + * any locks, that is, before these two lock_map_acquire()'s.
> *
> * However, that would result in:
> *
> @@ -2107,14 +2107,14 @@ __acquires(&pool->lock)
> * Which would create W1->C->W1 dependencies, even though there is no
> * actual deadlock possible. There are two solutions, using a
> * read-recursive acquire on the work(queue) 'locks', but this will then
> - * hit the lockdep limitation on recursive locks, or simly discard
> + * hit the lockdep limitation on recursive locks, or simply discard
> * these locks.
> *
> * AFAICT there is no possible deadlock scenario between the
> * flush_work() and complete() primitives (except for single-threaded
> * workqueues), so hiding them isn't a problem.
> */
> - crossrelease_hist_start(XHLOCK_PROC, true);
> + lockdep_invariant_state(true);
> trace_workqueue_execute_start(work);
> worker->current_func(work);
> /*
> @@ -2122,7 +2122,6 @@ __acquires(&pool->lock)
> * point will only record its address.
> */
> trace_workqueue_execute_end(work);
> - crossrelease_hist_end(XHLOCK_PROC);
> lock_map_release(&lockdep_map);
> lock_map_release(&pwq->wq->lockdep_map);
>
--
Thanks,
Byungchul
Powered by blists - more mailing lists