From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752465AbdJ3OFJ (ORCPT ); Mon, 30 Oct 2017 10:05:09 -0400 Received: from mail-oi0-f67.google.com ([209.85.218.67]:49435 "EHLO mail-oi0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751572AbdJ3OFI (ORCPT ); Mon, 30 Oct 2017 10:05:08 -0400 X-Google-Smtp-Source: ABhQp+QhyTfqs2P0DxOVqxzYKmIT+FrkpB84ChIC4iwhfzRKtMDOMVR2DvQEeejhWzH8rMdQ1Wd9Cw== X-ME-Sender: Date: Mon, 30 Oct 2017 22:06:33 +0800 From: Boqun Feng To: Jan Kara Cc: Waiman Long , Alexander Viro , Jan Kara , Jeff Layton , "J. Bruce Fields" , Tejun Heo , Christoph Lameter , linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, Ingo Molnar , Peter Zijlstra , Andi Kleen , Dave Chinner , Davidlohr Bueso Subject: Re: [PATCH v7 10/10] lib/dlock-list: Fix use-after-unlock problem in dlist_for_each_entry_safe() Message-ID: <20171030140633.GB29040@tardis> References: <1507229008-20569-1-git-send-email-longman@redhat.com> <1509135053-19214-1-git-send-email-longman@redhat.com> <20171030090640.GC23278@quack2.suse.cz> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="Y7xTucakfITjPcLV" Content-Disposition: inline In-Reply-To: <20171030090640.GC23278@quack2.suse.cz> User-Agent: Mutt/1.9.1 (2017-09-22) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --Y7xTucakfITjPcLV Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Oct 30, 2017 at 10:06:40AM +0100, Jan Kara wrote: > On Fri 27-10-17 16:10:53, Waiman Long wrote: > > The dlist_for_each_entry_safe() macro in include/linux/dlock-list has > > a use-after-unlock problem where racing condition can happen because > > of a lack of spinlock protection. Fortunately, this macro is not > > currently being used in the kernel. > >=20 > > This patch changes the dlist_for_each_entry_safe() macro so that the > > call to __dlock_list_next_list() is deferred until the next entry is > > being used. That should eliminate the use-after-unlock problem. > >=20 Better than what I proposed, thanks for looking into this! > > Reported-by: Boqun Feng > > Signed-off-by: Waiman Long >=20 > Looks good to me. You can add: >=20 > Reviewed-by: Jan Kara >=20 Also mine FWIW: Reviewed-by: Boqun Feng Regards, Boqun > Honza >=20 >=20 > > --- > > include/linux/dlock-list.h | 28 +++++++++++++++++----------- > > 1 file changed, 17 insertions(+), 11 deletions(-) > >=20 > > diff --git a/include/linux/dlock-list.h b/include/linux/dlock-list.h > > index 02c5f4d..f4b7657 100644 > > --- a/include/linux/dlock-list.h > > +++ b/include/linux/dlock-list.h > > @@ -191,17 +191,17 @@ extern void dlock_list_add(struct dlock_list_node= *node, > > } > > =20 > > /** > > - * dlock_list_first_entry - get the first element from a list > > + * dlock_list_next_list_entry - get first element from next list in it= erator > > * @iter : The dlock list iterator. > > - * @type : The type of the struct this is embedded in. > > + * @pos : A variable of the struct that is embedded in. > > * @member: The name of the dlock_list_node within the struct. > > - * Return : Pointer to the next entry or NULL if all the entries are i= terated. > > + * Return : Pointer to first entry or NULL if all the lists are iterat= ed. > > */ > > -#define dlock_list_first_entry(iter, type, member) \ > > +#define dlock_list_next_list_entry(iter, pos, member) \ > > ({ \ > > struct dlock_list_node *_n; \ > > _n =3D __dlock_list_next_entry(NULL, iter); \ > > - _n ? list_entry(_n, type, member) : NULL; \ > > + _n ? list_entry(_n, typeof(*pos), member) : NULL; \ > > }) > > =20 > > /** > > @@ -231,7 +231,7 @@ extern void dlock_list_add(struct dlock_list_node *= node, > > * This iteration function is designed to be used in a while loop. > > */ > > #define dlist_for_each_entry(pos, iter, member) \ > > - for (pos =3D dlock_list_first_entry(iter, typeof(*(pos)), member);\ > > + for (pos =3D dlock_list_next_list_entry(iter, pos, member); \ > > pos !=3D NULL; \ > > pos =3D dlock_list_next_entry(pos, iter, member)) > > =20 > > @@ -245,14 +245,20 @@ extern void dlock_list_add(struct dlock_list_node= *node, > > * This iteration macro is safe with respect to list entry removal. > > * However, it cannot correctly iterate newly added entries right afte= r the > > * current one. > > + * > > + * The call to __dlock_list_next_list() is deferred until the next ent= ry > > + * is being iterated to avoid use-after-unlock problem. > > */ > > #define dlist_for_each_entry_safe(pos, n, iter, member) \ > > - for (pos =3D dlock_list_first_entry(iter, typeof(*(pos)), member);\ > > + for (pos =3D NULL; \ > > ({ \ > > - bool _b =3D (pos !=3D NULL); \ > > - if (_b) \ > > - n =3D dlock_list_next_entry(pos, iter, member); \ > > - _b; \ > > + if (!pos || \ > > + (&(pos)->member.list =3D=3D &(iter)->entry->list)) \ > > + pos =3D dlock_list_next_list_entry(iter, pos, \ > > + member); \ > > + if (pos) \ > > + n =3D list_next_entry(pos, member.list); \ > > + pos; \ > > }); \ > > pos =3D n) > > =20 > > --=20 > > 1.8.3.1 > >=20 > >=20 > --=20 > Jan Kara > SUSE Labs, CR --Y7xTucakfITjPcLV Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQEzBAABCAAdFiEEj5IosQTPz8XU1wRHSXnow7UH+rgFAln3MeUACgkQSXnow7UH +rhZ0Af6AuFTeQqigtPCohyf/upW3ddSTMak1erwtZn/pNj8p3fRPCaBIv8WBQep k1xsYAyTHvKr1a8XoRBIDUSfx04hpdS58P7zYgBALQ/VcWOELFaoPTY1e2y2OlUC LSS4NS8SxWt2NHhU+RVK/xlVkLgusU1G5oyvKABWv/E/WjKQBSix7zQTq6k9UUFg UG0AhdhGLl4XCrjkc2jZ5xyAzgmmHGlxKDowqInrCx+g8JXFYTqu6l5OnB3W2g7r rg+cv3cUhCl+3kAoiBTirHdMDGDBfb9xRa5kDfzFIli+E+OMpYkAJpbAgW4QozPD 3gmK0Wy1Hilz2C7/RN2ENo42SPWDCQ== =8ijz -----END PGP SIGNATURE----- --Y7xTucakfITjPcLV--