Thread (85 messages) 85 messages, 22 authors, 2022-03-07

RE: [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr

From: David Laight <hidden>
Date: 2022-03-03 04:58:36
Also in: alsa-devel, amd-gfx, dmaengine, dri-devel, intel-gfx, intel-wired-lan, kvm, linux-arch, linux-aspeed, linux-block, linux-cifs, linux-crypto, linux-f2fs-devel, linux-fsdevel, linux-iio, linux-media, linux-mediatek, linux-pm, linux-rdma, linux-scsi, linux-staging, linux-tegra, linux-usb, linux-wireless, lkml, netdev, nouveau

From: Xiaomeng Tong
Sent: 03 March 2022 02:27

On Wed, 2 Mar 2022 14:04:06 +0000, David Laight
[off-list ref] wrote:
quoted
I think that it would be better to make any alternate loop macro
just set the variable to NULL on the loop exit.
That is easier to code for and the compiler might be persuaded to
not redo the test.
No, that would lead to a NULL dereference.
Why, it would make it b ethe same as the 'easy to use':
	for (item = head; item; item = item->next) {
		...
		if (...)
			break;
		...
	}
	if (!item)
		return;
 
The problem is the mis-use of iterator outside the loop on exit, and
the iterator will be the HEAD's container_of pointer which pointers
to a type-confused struct. Sidenote: The *mis-use* here refers to
mistakely access to other members of the struct, instead of the
list_head member which acutally is the valid HEAD.
The problem is that the HEAD's container_of pointer should never
be calculated at all.
This is what is fundamentally broken about the current definition.
IOW, you would dereference a (NULL + offset_of_member) address here.
Where?
Please remind me if i missed something, thanks.

Can you share your "alternative definitions" details? thanks!
The loop should probably use as extra variable that points
to the 'list node' in the next structure.
Something like:
	for (xxx *iter = head->next;
		iter == &head ? ((item = NULL),0) : ((item = list_item(iter),1));
		iter = item->member->next) {
	   ...
With a bit of casting you can use 'item' to hold 'iter'.
quoted
OTOH there may be alternative definitions that can be used to get
the compiler (or other compiler-like tools) to detect broken code.
Even if the definition can't possibly generate a working kerrnel.
The "list_for_each_entry_inside(pos, type, head, member)" way makes
the iterator invisiable outside the loop, and would be catched by
compiler if use-after-loop things happened.
It is also a compete PITA for anything doing a search.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help