From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752722AbdJ3JGn (ORCPT ); Mon, 30 Oct 2017 05:06:43 -0400 Received: from mx2.suse.de ([195.135.220.15]:43805 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751445AbdJ3JGm (ORCPT ); Mon, 30 Oct 2017 05:06:42 -0400 Date: Mon, 30 Oct 2017 10:06:40 +0100 From: Jan Kara To: Waiman Long Cc: 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 , Boqun Feng , Davidlohr Bueso Subject: Re: [PATCH v7 10/10] lib/dlock-list: Fix use-after-unlock problem in dlist_for_each_entry_safe() Message-ID: <20171030090640.GC23278@quack2.suse.cz> References: <1507229008-20569-1-git-send-email-longman@redhat.com> <1509135053-19214-1-git-send-email-longman@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1509135053-19214-1-git-send-email-longman@redhat.com> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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. > > 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. > > Reported-by: Boqun Feng > Signed-off-by: Waiman Long Looks good to me. You can add: Reviewed-by: Jan Kara Honza > --- > include/linux/dlock-list.h | 28 +++++++++++++++++----------- > 1 file changed, 17 insertions(+), 11 deletions(-) > > 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, > } > > /** > - * dlock_list_first_entry - get the first element from a list > + * dlock_list_next_list_entry - get first element from next list in iterator > * @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 iterated. > + * Return : Pointer to first entry or NULL if all the lists are iterated. > */ > -#define dlock_list_first_entry(iter, type, member) \ > +#define dlock_list_next_list_entry(iter, pos, member) \ > ({ \ > struct dlock_list_node *_n; \ > _n = __dlock_list_next_entry(NULL, iter); \ > - _n ? list_entry(_n, type, member) : NULL; \ > + _n ? list_entry(_n, typeof(*pos), member) : NULL; \ > }) > > /** > @@ -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 = dlock_list_first_entry(iter, typeof(*(pos)), member);\ > + for (pos = dlock_list_next_list_entry(iter, pos, member); \ > pos != NULL; \ > pos = dlock_list_next_entry(pos, iter, member)) > > @@ -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 after the > * current one. > + * > + * The call to __dlock_list_next_list() is deferred until the next entry > + * is being iterated to avoid use-after-unlock problem. > */ > #define dlist_for_each_entry_safe(pos, n, iter, member) \ > - for (pos = dlock_list_first_entry(iter, typeof(*(pos)), member);\ > + for (pos = NULL; \ > ({ \ > - bool _b = (pos != NULL); \ > - if (_b) \ > - n = dlock_list_next_entry(pos, iter, member); \ > - _b; \ > + if (!pos || \ > + (&(pos)->member.list == &(iter)->entry->list)) \ > + pos = dlock_list_next_list_entry(iter, pos, \ > + member); \ > + if (pos) \ > + n = list_next_entry(pos, member.list); \ > + pos; \ > }); \ > pos = n) > > -- > 1.8.3.1 > > -- Jan Kara SUSE Labs, CR