From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx2.suse.de ([195.135.220.15]:55014 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752465AbdJ3OMs (ORCPT ); Mon, 30 Oct 2017 10:12:48 -0400 Date: Mon, 30 Oct 2017 07:11:48 -0700 From: Davidlohr Bueso 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 Subject: Re: [PATCH v7 10/10] lib/dlock-list: Fix use-after-unlock problem in dlist_for_each_entry_safe() Message-ID: <20171030141147.leqcsaxebwiq6dq6@linux-n805> 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; format=flowed Content-Disposition: inline In-Reply-To: <1509135053-19214-1-git-send-email-longman@redhat.com> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Fri, 27 Oct 2017, 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 Reviewed-by: Davidlohr Bueso But would it not be better to merge this patch (among others) into 1/N? Specifically the newer patches 7-10 should be in the original dlock implementation instead of adding fixes to incorrect code in the original commit. Also less of a pita for backporting. Thanks, Davidlohr >--- > 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 >