From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751218AbdBUAqm (ORCPT ); Mon, 20 Feb 2017 19:46:42 -0500 Received: from shadbolt.e.decadent.org.uk ([88.96.1.126]:60808 "EHLO shadbolt.e.decadent.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750948AbdBUAqk (ORCPT ); Mon, 20 Feb 2017 19:46:40 -0500 Message-ID: <1487637984.2885.8.camel@decadent.org.uk> Subject: Re: [PATCH 3.2 114/126] perf/core: Fix concurrent sys_perf_event_open() vs. 'move_group' race From: Ben Hutchings To: linux-kernel@vger.kernel.org, stable@vger.kernel.org Cc: akpm@linux-foundation.org, John Dias , Di@decadent.org.uk, Thomas Gleixner , Vince Weaver , Linus Torvalds , Jiri Olsa , Stephane Eranian , Arnaldo Carvalho de Melo , Kees Cook , Ingo Molnar , Peter Zijlstra , Arnaldo Carvalho de Melo , Min Chong , Alexander Shishkin Date: Tue, 21 Feb 2017 00:46:24 +0000 In-Reply-To: References: Content-Type: multipart/signed; micalg="pgp-sha512"; protocol="application/pgp-signature"; boundary="=-0DoE2X9AFD9itsNMZU/z" X-Mailer: Evolution 3.22.4-1 Mime-Version: 1.0 X-SA-Exim-Connect-IP: 188.29.164.30 X-SA-Exim-Mail-From: ben@decadent.org.uk X-SA-Exim-Scanned: No (on shadbolt.decadent.org.uk); SAEximRunCond expanded to false Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --=-0DoE2X9AFD9itsNMZU/z Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Wed, 2017-02-15 at 22:41 +0000, Ben Hutchings wrote: > 3.2.85-rc1 review patch.=C2=A0=C2=A0If anyone has any objections, please = let me know. >=20 > ------------------ >=20 > From: Peter Zijlstra >=20 > commit 321027c1fe77f892f4ea07846aeae08cefbbb290 upstream. >=20 > Di Shen reported a race between two concurrent sys_perf_event_open() > calls where both try and move the same pre-existing software group > into a hardware context. >=20 > The problem is exactly that described in commit: >=20 > =C2=A0 f63a8daa5812 ("perf: Fix event->ctx locking") >=20 > ... where, while we wait for a ctx->mutex acquisition, the event->ctx > relation can have changed under us. >=20 > That very same commit failed to recognise sys_perf_event_context() as an > external access vector to the events and thereby didn't apply the > established locking rules correctly. >=20 > So while one sys_perf_event_open() call is stuck waiting on > mutex_lock_double(), the other (which owns said locks) moves the group > about. So by the time the former sys_perf_event_open() acquires the > locks, the context we've acquired is stale (and possibly dead). >=20 > Apply the established locking rules as per perf_event_ctx_lock_nested() > to the mutex_lock_double() for the 'move_group' case. This obviously mean= s > we need to validate state after we acquire the locks. [...] > =C2=A0 /* > =C2=A0 =C2=A0* See perf_event_ctx_lock() for comments on the details > =C2=A0 =C2=A0* of swizzling perf_event::ctx. > =C2=A0 =C2=A0*/ > - mutex_lock_double(&gctx->mutex, &ctx->mutex); > - > =C2=A0 perf_remove_from_context(group_leader, false); > =C2=A0 > =C2=A0 /* > @@ -6709,10 +6757,8 @@ SYSCALL_DEFINE5(perf_event_open, > =C2=A0 ++ctx->generation; > =C2=A0 perf_unpin_context(ctx); > =C2=A0 > - if (move_group) { > - mutex_unlock(&gctx->mutex); > - put_ctx(gctx); > - } > + if (move_group) > + perf_event_ctx_unlock(group_leader, gctx); > =C2=A0 mutex_unlock(&ctx->mutex); > =C2=A0 > =C2=A0 event->owner =3D current; [...] Peter has clarified that the last call to put_ctx(gctx) corresponds to the reference cleared by perf_remove_from_context(group_leader, false) above. So although perf_event_ctx_unlock() also calls put_ctx(gctx), we really do want to drop two references here now and should keep the direct call. I made the same error when backporting to 3.16, and will fix that as well. Ben. --=20 Ben Hutchings 73.46% of all statistics are made up. --=-0DoE2X9AFD9itsNMZU/z Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part -----BEGIN PGP SIGNATURE----- iQIzBAABCgAdFiEErCspvTSmr92z9o8157/I7JWGEQkFAlirjeAACgkQ57/I7JWG EQlQrw//V16f7zVlNVwEnXHcmFIvB+lT8rQb45vrels0fVYuTXeNAtmUBY6iCguq mDrwnxb4RqsnNW9jd1vvyflUPotxTNjLV31CE0VTArxy9II0IQOKqx815JEu/wFr 1GZPZ5Ym2wj+C9qp7dDonkK4VBIa3TMuTxN5Mv1Z/yKD9Er5hr90hSWqz2poRJC/ 1Lqk1kqq9tzPup651oTTd288cGCCkLAIgDsfBoQ/sfVU8hcDc7c9OVqrd+bpxMZZ mzLO3yFh+u97pPfcy4xYwJEHMSibfd+yJaly257KHSLzauKnSkjGyyFYVGglLQWx Xg+1ABXRj+Aeq0rWfc8gUtXMSR5MpR+HsqxzCEDIsnbtNREELSe0jqOJc4qtDeJK lLuiwOIIBqRuUn3TrXeH+xmewncFXOY25DDJs0MbjZqUkTEbaX9MPrUXSTN+eu2V HF8a++GGg/BTZf+gpuIe6fKX9oDWj4acmk+npB9fDnux6raB1dgS6XY6k5bZZkBb j912zVv425FRfchZyyUJ60fomYxrV3EoUHEMGEFEuTgNDtJO2+t34EvOjM+zEzm5 iB1K4u7vUJWr69eivWPNszHsO0sJj2I0o8K82pH5+lLCm6m4v5tOjx+sObNoWk4a g0dPrUUtzgk8SeuuQbKpuD3iBDx2enCnKcCbHdbtIFEXH0a+4Uw= =X3JM -----END PGP SIGNATURE----- --=-0DoE2X9AFD9itsNMZU/z--