Christian König
2022-Feb-28 12:19 UTC
[Nouveau] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
Am 28.02.22 um 12:08 schrieb Jakob Koschel:> If the list does not contain the expected element, the value of > list_for_each_entry() iterator will not point to a valid structure. > To avoid type confusion in such case, the list iterator > scope will be limited to list_for_each_entry() loop.We explicitly have the list_entry_is_head() macro to test after a loop if the element pointer points to the head of the list instead of a valid list entry. So at least from my side I absolutely don't think that this is a good idea.> In preparation to limiting scope of a list iterator to the list traversal > loop, use a dedicated pointer to point to the found element. > Determining if an element was found is then simply checking if > the pointer is != NULL.Since when do we actually want to do this? Take this code here as an example:> diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c > index 48afe96ae0f0..6c916416decc 100644 > --- a/arch/x86/kernel/cpu/sgx/encl.c > +++ b/arch/x86/kernel/cpu/sgx/encl.c > @@ -450,7 +450,8 @@ static void sgx_mmu_notifier_release(struct mmu_notifier *mn, > struct mm_struct *mm) > { > struct sgx_encl_mm *encl_mm = container_of(mn, struct sgx_encl_mm, mmu_notifier); > - struct sgx_encl_mm *tmp = NULL; > + struct sgx_encl_mm *found_encl_mm = NULL; > + struct sgx_encl_mm *tmp; > > /* > * The enclave itself can remove encl_mm. Note, objects can't be moved > @@ -460,12 +461,13 @@ static void sgx_mmu_notifier_release(struct mmu_notifier *mn, > list_for_each_entry(tmp, &encl_mm->encl->mm_list, list) { > if (tmp == encl_mm) { > list_del_rcu(&encl_mm->list); > + found_encl_mm = tmp; > break; > } > } > spin_unlock(&encl_mm->encl->mm_lock); > > - if (tmp == encl_mm) { > + if (found_encl_mm) { > synchronize_srcu(&encl_mm->encl->srcu); > mmu_notifier_put(mn); > }I don't think that using the extra variable makes the code in any way more reliable or easier to read. Regards, Christian.
Linus Torvalds
2022-Feb-28 19:56 UTC
[Nouveau] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Mon, Feb 28, 2022 at 4:19 AM Christian K?nig <christian.koenig at amd.com> wrote:> > I don't think that using the extra variable makes the code in any way > more reliable or easier to read.So I think the next step is to do the attached patch (which requires that "-std=gnu11" that was discussed in the original thread). That will guarantee that the 'pos' parameter of list_for_each_entry() is only updated INSIDE the for_each_list_entry() loop, and can never point to the (wrongly typed) head entry. And I would actually hope that it should actually cause compiler warnings about possibly uninitialized variables if people then use the 'pos' pointer outside the loop. Except (a) that code in sgx/encl.c currently initializes 'tmp' to NULL for inexplicable reasons - possibly because it already expected this behavior (b) when I remove that NULL initializer, I still don't get a warning, because we've disabled -Wno-maybe-uninitialized since it results in so many false positives. Oh well. Anyway, give this patch a look, and at least if it's expanded to do "(pos) = NULL" in the entry statement for the for-loop, it will avoid the HEAD type confusion that Jakob is working on. And I think in a cleaner way than the horrid games he plays. (But it won't avoid possible CPU speculation of such type confusion. That, in my opinion, is a completely different issue) I do wish we could actually poison the 'pos' value after the loop somehow - but clearly the "might be uninitialized" I was hoping for isn't the way to do it. Anybody have any ideas? Linus -------------- next part -------------- A non-text attachment was scrubbed... Name: p Type: application/octet-stream Size: 881 bytes Desc: not available URL: <https://lists.freedesktop.org/archives/nouveau/attachments/20220228/b8f33f5b/attachment-0001.obj>
David Laight
2022-Mar-01 02:15 UTC
[Nouveau] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
From: Linus Torvalds> Sent: 28 February 2022 19:56 > On Mon, Feb 28, 2022 at 4:19 AM Christian K?nig > <christian.koenig at amd.com> wrote: > > > > I don't think that using the extra variable makes the code in any way > > more reliable or easier to read. > > So I think the next step is to do the attached patch (which requires > that "-std=gnu11" that was discussed in the original thread). > > That will guarantee that the 'pos' parameter of list_for_each_entry() > is only updated INSIDE the for_each_list_entry() loop, and can never > point to the (wrongly typed) head entry. > > And I would actually hope that it should actually cause compiler > warnings about possibly uninitialized variables if people then use the > 'pos' pointer outside the loop. Except > > (a) that code in sgx/encl.c currently initializes 'tmp' to NULL for > inexplicable reasons - possibly because it already expected this > behavior > > (b) when I remove that NULL initializer, I still don't get a warning, > because we've disabled -Wno-maybe-uninitialized since it results in so > many false positives. > > Oh well. > > Anyway, give this patch a look, and at least if it's expanded to do > "(pos) = NULL" in the entry statement for the for-loop, it will avoid > the HEAD type confusion that Jakob is working on. And I think in a > cleaner way than the horrid games he plays. > > (But it won't avoid possible CPU speculation of such type confusion. > That, in my opinion, is a completely different issue) > > I do wish we could actually poison the 'pos' value after the loop > somehow - but clearly the "might be uninitialized" I was hoping for > isn't the way to do it. > > Anybody have any ideas? > > Linusdiff --git a/include/linux/list.h b/include/linux/list.h index dd6c2041d09c..bab995596aaa 100644 --- a/include/linux/list.h +++ b/include/linux/list.h @@ -634,10 +634,10 @@ static inline void list_splice_tail_init(struct list_head *list, * @head: the head for your list. * @member: the name of the list_head within the struct. */ -#define list_for_each_entry(pos, head, member) \ - for (pos = list_first_entry(head, typeof(*pos), member); \ - !list_entry_is_head(pos, head, member); \ - pos = list_next_entry(pos, member)) +#define list_for_each_entry(pos, head, member) \ + for (typeof(pos) __iter = list_first_entry(head, typeof(*pos), member); \ + !list_entry_is_head(__iter, head, member) && (((pos)=__iter),1); \ + __iter = list_next_entry(__iter, member)) /** * list_for_each_entry_reverse - iterate backwards over list of given type. I think you actually want: !list_entry_is_head(__iter, head, member) ? (((pos)=__iter),1) : (((pos) = NULL),0); Which can be done in the original by: !list_entry_is_head(pos, head, member) ? 1 : (((pos) = NULL), 0); Although it would be safer to have (without looking up the actual name): for (item *__item = head; \ __item ? (((pos) = list_item(__item, member)), 1) : (((pos) = NULL), 0); __item = (pos)->member) The local does need 'fixing' to avoid shadowing for nested loops. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)