Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752413AbdHJL7T (ORCPT ); Thu, 10 Aug 2017 07:59:19 -0400 Received: from mail-pf0-f193.google.com ([209.85.192.193]:36081 "EHLO mail-pf0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752347AbdHJL7S (ORCPT ); Thu, 10 Aug 2017 07:59:18 -0400 Date: Thu, 10 Aug 2017 19:59:22 +0800 From: Boqun Feng To: Byungchul Park Cc: peterz@infradead.org, mingo@kernel.org, tglx@linutronix.de, walken@google.com, kirill@shutemov.name, linux-kernel@vger.kernel.org, linux-mm@kvack.org, akpm@linux-foundation.org, willy@infradead.org, npiggin@gmail.com, kernel-team@lge.com Subject: Re: [PATCH v8 06/14] lockdep: Detect and handle hist_lock ring buffer overwrite Message-ID: <20170810115922.kegrfeg6xz7mgpj4@tardis> References: <1502089981-21272-1-git-send-email-byungchul.park@lge.com> <1502089981-21272-7-git-send-email-byungchul.park@lge.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="ieps2idy3c3cac3n" Content-Disposition: inline In-Reply-To: <1502089981-21272-7-git-send-email-byungchul.park@lge.com> User-Agent: NeoMutt/20170609 (1.8.3) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 7032 Lines: 238 --ieps2idy3c3cac3n Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Aug 07, 2017 at 04:12:53PM +0900, Byungchul Park wrote: > The ring buffer can be overwritten by hardirq/softirq/work contexts. > That cases must be considered on rollback or commit. For example, >=20 > |<------ hist_lock ring buffer size ----->| > ppppppppppppiiiiiiiiiiiiiiiiiiiiiiiiiiiiiii > wrapped > iiiiiiiiiiiiiiiiiiiiiii.................... >=20 > where 'p' represents an acquisition in process context, > 'i' represents an acquisition in irq context. >=20 > On irq exit, crossrelease tries to rollback idx to original position, > but it should not because the entry already has been invalid by > overwriting 'i'. Avoid rollback or commit for entries overwritten. >=20 > Signed-off-by: Byungchul Park > --- > include/linux/lockdep.h | 20 +++++++++++++++++++ > include/linux/sched.h | 3 +++ > kernel/locking/lockdep.c | 52 ++++++++++++++++++++++++++++++++++++++++++= +----- > 3 files changed, 70 insertions(+), 5 deletions(-) >=20 > diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h > index 0c8a1b8..48c244c 100644 > --- a/include/linux/lockdep.h > +++ b/include/linux/lockdep.h > @@ -284,6 +284,26 @@ struct held_lock { > */ > struct hist_lock { > /* > + * Id for each entry in the ring buffer. This is used to > + * decide whether the ring buffer was overwritten or not. > + * > + * For example, > + * > + * |<----------- hist_lock ring buffer size ------->| > + * pppppppppppppppppppppiiiiiiiiiiiiiiiiiiiiiiiiiiiii > + * wrapped > iiiiiiiiiiiiiiiiiiiiiiiiiii....................... > + * > + * where 'p' represents an acquisition in process > + * context, 'i' represents an acquisition in irq > + * context. > + * > + * In this example, the ring buffer was overwritten by > + * acquisitions in irq context, that should be detected on > + * rollback or commit. > + */ > + unsigned int hist_id; > + > + /* > * Seperate stack_trace data. This will be used at commit step. > */ > struct stack_trace trace; > diff --git a/include/linux/sched.h b/include/linux/sched.h > index 5becef5..373466b 100644 > --- a/include/linux/sched.h > +++ b/include/linux/sched.h > @@ -855,6 +855,9 @@ struct task_struct { > unsigned int xhlock_idx; > /* For restoring at history boundaries */ > unsigned int xhlock_idx_hist[CONTEXT_NR]; > + unsigned int hist_id; > + /* For overwrite check at each context exit */ > + unsigned int hist_id_save[CONTEXT_NR]; > #endif > =20 > #ifdef CONFIG_UBSAN > diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c > index afd6e64..5168dac 100644 > --- a/kernel/locking/lockdep.c > +++ b/kernel/locking/lockdep.c > @@ -4742,6 +4742,17 @@ void lockdep_rcu_suspicious(const char *file, cons= t int line, const char *s) > static atomic_t cross_gen_id; /* Can be wrapped */ > =20 > /* > + * Make an entry of the ring buffer invalid. > + */ > +static inline void invalidate_xhlock(struct hist_lock *xhlock) > +{ > + /* > + * Normally, xhlock->hlock.instance must be !NULL. > + */ > + xhlock->hlock.instance =3D NULL; > +} > + > +/* > * Lock history stacks; we have 3 nested lock history stacks: > * > * Hard IRQ > @@ -4773,14 +4784,28 @@ void lockdep_rcu_suspicious(const char *file, con= st int line, const char *s) > */ > void crossrelease_hist_start(enum context_t c) > { > - if (current->xhlocks) > - current->xhlock_idx_hist[c] =3D current->xhlock_idx; > + struct task_struct *cur =3D current; > + > + if (cur->xhlocks) { > + cur->xhlock_idx_hist[c] =3D cur->xhlock_idx; > + cur->hist_id_save[c] =3D cur->hist_id; > + } > } > =20 > void crossrelease_hist_end(enum context_t c) > { > - if (current->xhlocks) > - current->xhlock_idx =3D current->xhlock_idx_hist[c]; > + struct task_struct *cur =3D current; > + > + if (cur->xhlocks) { > + unsigned int idx =3D cur->xhlock_idx_hist[c]; > + struct hist_lock *h =3D &xhlock(idx); > + > + cur->xhlock_idx =3D idx; > + > + /* Check if the ring was overwritten. */ > + if (h->hist_id !=3D cur->hist_id_save[c]) Could we use: if (h->hist_id !=3D idx) here, and > + invalidate_xhlock(h); > + } > } > =20 > static int cross_lock(struct lockdep_map *lock) > @@ -4826,6 +4851,7 @@ static inline int depend_after(struct held_lock *hl= ock) > * Check if the xhlock is valid, which would be false if, > * > * 1. Has not used after initializaion yet. > + * 2. Got invalidated. > * > * Remind hist_lock is implemented as a ring buffer. > */ > @@ -4857,6 +4883,7 @@ static void add_xhlock(struct held_lock *hlock) > =20 > /* Initialize hist_lock's members */ > xhlock->hlock =3D *hlock; > + xhlock->hist_id =3D current->hist_id++; use: xhlock->hist_id =3D idx; and, > =20 > xhlock->trace.nr_entries =3D 0; > xhlock->trace.max_entries =3D MAX_XHLOCK_TRACE_ENTRIES; > @@ -4995,6 +5022,7 @@ static int commit_xhlock(struct cross_lock *xlock, = struct hist_lock *xhlock) > static void commit_xhlocks(struct cross_lock *xlock) > { > unsigned int cur =3D current->xhlock_idx; > + unsigned int prev_hist_id =3D xhlock(cur).hist_id; use: unsigned int prev_hist_id =3D cur; here. Then we can get away with the added fields in task_struct at least. Thought? Regards, Boqun > unsigned int i; > =20 > if (!graph_lock()) > @@ -5013,6 +5041,17 @@ static void commit_xhlocks(struct cross_lock *xloc= k) > break; > =20 > /* > + * Filter out the cases that the ring buffer was > + * overwritten and the previous entry has a bigger > + * hist_id than the following one, which is impossible > + * otherwise. > + */ > + if (unlikely(before(xhlock->hist_id, prev_hist_id))) > + break; > + > + prev_hist_id =3D xhlock->hist_id; > + > + /* > * commit_xhlock() returns 0 with graph_lock already > * released if fail. > */ > @@ -5085,9 +5124,12 @@ void lockdep_init_task(struct task_struct *task) > int i; > =20 > task->xhlock_idx =3D UINT_MAX; > + task->hist_id =3D 0; > =20 > - for (i =3D 0; i < CONTEXT_NR; i++) > + for (i =3D 0; i < CONTEXT_NR; i++) { > task->xhlock_idx_hist[i] =3D UINT_MAX; > + task->hist_id_save[i] =3D 0; > + } > =20 > task->xhlocks =3D kzalloc(sizeof(struct hist_lock) * MAX_XHLOCKS_NR, > GFP_KERNEL); > --=20 > 1.9.1 >=20 --ieps2idy3c3cac3n Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQEzBAABCAAdFiEEj5IosQTPz8XU1wRHSXnow7UH+rgFAlmMSpcACgkQSXnow7UH +ri8Owf+MEgznLkEiujZoUhrUwJnoR0tY/UtfUIVIbsNE2zfTM7g9Tw0PrMcmxPb VLNaYTHfcAzv/AHPrBjqvj04sWpp69ZDhsM3M5VxW3xPo7Cx8sejpZSTfC1QEO+K nYQ7+IfmTp1R7XMTmFsxW/ZF1Zx6O53cYaLWYcPw5sKBVG2QQsE2jMKXLV2ivFiE S9Rw0Yu5DYMZkepduiVNC1LzLYOBNqrwehu+SIOHtorGDkzQMUc/f8EgSXVgjE2p dzkgsRRRr+vo0TdWYMA7/9b+vhi2qR9eQqAyZ6boWAKgsNUOEem8JTjFOVZUOjcH Lv/Xywwdn9PoLQsrerb130ayhZqQ9Q== =1bXY -----END PGP SIGNATURE----- --ieps2idy3c3cac3n--