Re: [Nouveau] [PATCH] dispnv50: atom: fix an incorrect NULL check on list iterator
on Sun, 27 Mar 2022 16:59:28 +0100, Emil Velikov wrote: > On Sun, 27 Mar 2022 at 08:39, Xiaomeng Tong wrote: > > > > The bug is here: > > return encoder; > > > > The list iterator value 'encoder' will *always* be set and non-NULL > > by drm_for_each_encoder_mask(), so it is incorrect to assume that the > > iterator value will be NULL if the list is empty or no element found. > > Otherwise it will bypass some NULL checks and lead to invalid memory > > access passing the check. > > > > To fix this bug, just return 'encoder' when found, otherwise return > > NULL. > > > > Isn't this covered by the upcoming list* iterator rework [1] or is > this another iterator glitch? Actually, it is a part of the upcoming work. > IMHO we should be looking at fixing the implementation and not the > hundreds of users through the kernel. > > HTH > -Emil > [1] https://lwn.net/Articles/887097/ Yes, you are right. This has also been taken into account by the upcoming list iterator rework to avoid a lot uesr' changes as much as possible. However, this patch is fixing a potential bug caused by incorrect use of list iterator outside the loop, which can not be fixed by the implementation itself. -- Xiaomeng Tong
[Nouveau] [PATCH] clk: base: fix an incorrect NULL check on list iterator
The bug is here: if (nvkm_cstate_valid(clk, cstate, max_volt, clk->temp)) return cstate; The list iterator value 'cstate' will *always* be set and non-NULL by list_for_each_entry_from_reverse(), so it is incorrect to assume that the iterator value will be unchanged if the list is empty or no element is found (In fact, it will be a bogus pointer to an invalid structure object containing the HEAD). Also it missed a NULL check at callsite and may lead to invalid memory access after that. To fix this bug, just return 'encoder' when found, otherwise return NULL. And add the NULL check. Cc: sta...@vger.kernel.org Fixes: 1f7f3d91ad38a ("drm/nouveau/clk: Respect voltage limits in nvkm_cstate_prog") Signed-off-by: Xiaomeng Tong --- drivers/gpu/drm/nouveau/nvkm/subdev/clk/base.c | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/clk/base.c b/drivers/gpu/drm/nouveau/nvkm/subdev/clk/base.c index 57199be082fd..c2b5cc5f97ed 100644 --- a/drivers/gpu/drm/nouveau/nvkm/subdev/clk/base.c +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/clk/base.c @@ -135,10 +135,10 @@ nvkm_cstate_find_best(struct nvkm_clk *clk, struct nvkm_pstate *pstate, list_for_each_entry_from_reverse(cstate, &pstate->list, head) { if (nvkm_cstate_valid(clk, cstate, max_volt, clk->temp)) - break; + return cstate; } - return cstate; + return NULL; } static struct nvkm_cstate * @@ -169,6 +169,8 @@ nvkm_cstate_prog(struct nvkm_clk *clk, struct nvkm_pstate *pstate, int cstatei) if (!list_empty(&pstate->list)) { cstate = nvkm_cstate_get(clk, pstate, cstatei); cstate = nvkm_cstate_find_best(clk, pstate, cstate); + if (!cstate) + return -EINVAL; } else { cstate = &pstate->base; } -- 2.17.1
[Nouveau] [PATCH] dispnv50: atom: fix an incorrect NULL check on list iterator
The bug is here: return encoder; The list iterator value 'encoder' will *always* be set and non-NULL by drm_for_each_encoder_mask(), so it is incorrect to assume that the iterator value will be NULL if the list is empty or no element found. Otherwise it will bypass some NULL checks and lead to invalid memory access passing the check. To fix this bug, just return 'encoder' when found, otherwise return NULL. Cc: sta...@vger.kernel.org Fixes: 12885ecbfe62d ("drm/nouveau/kms/nvd9-: Add CRC support") Signed-off-by: Xiaomeng Tong --- drivers/gpu/drm/nouveau/dispnv50/atom.h | 6 +++--- drivers/gpu/drm/nouveau/dispnv50/crc.c | 27 - 2 files changed, 25 insertions(+), 8 deletions(-) diff --git a/drivers/gpu/drm/nouveau/dispnv50/atom.h b/drivers/gpu/drm/nouveau/dispnv50/atom.h index 3d82b3c67dec..93f8f4f64578 100644 --- a/drivers/gpu/drm/nouveau/dispnv50/atom.h +++ b/drivers/gpu/drm/nouveau/dispnv50/atom.h @@ -160,14 +160,14 @@ nv50_head_atom_get(struct drm_atomic_state *state, struct drm_crtc *crtc) static inline struct drm_encoder * nv50_head_atom_get_encoder(struct nv50_head_atom *atom) { - struct drm_encoder *encoder = NULL; + struct drm_encoder *encoder; /* We only ever have a single encoder */ drm_for_each_encoder_mask(encoder, atom->state.crtc->dev, atom->state.encoder_mask) - break; + return encoder; - return encoder; + return NULL; } #define nv50_wndw_atom(p) container_of((p), struct nv50_wndw_atom, state) diff --git a/drivers/gpu/drm/nouveau/dispnv50/crc.c b/drivers/gpu/drm/nouveau/dispnv50/crc.c index 29428e770f14..b834e8a9ae77 100644 --- a/drivers/gpu/drm/nouveau/dispnv50/crc.c +++ b/drivers/gpu/drm/nouveau/dispnv50/crc.c @@ -390,9 +390,18 @@ void nv50_crc_atomic_check_outp(struct nv50_atom *atom) struct nv50_head_atom *armh = nv50_head_atom(old_crtc_state); struct nv50_head_atom *asyh = nv50_head_atom(new_crtc_state); struct nv50_outp_atom *outp_atom; - struct nouveau_encoder *outp = - nv50_real_outp(nv50_head_atom_get_encoder(armh)); - struct drm_encoder *encoder = &outp->base.base; + struct nouveau_encoder *outp; + struct drm_encoder *encoder, *enc; + + enc = nv50_head_atom_get_encoder(armh); + if (!enc) + continue; + + outp = nv50_real_outp(enc); + if (!outp) + continue; + + encoder = &outp->base.base; if (!asyh->clr.crc) continue; @@ -443,8 +452,16 @@ void nv50_crc_atomic_set(struct nv50_head *head, struct drm_device *dev = crtc->dev; struct nv50_crc *crc = &head->crc; const struct nv50_crc_func *func = nv50_disp(dev)->core->func->crc; - struct nouveau_encoder *outp = - nv50_real_outp(nv50_head_atom_get_encoder(asyh)); + struct nouveau_encoder *outp; + struct drm_encoder *encoder; + + encoder = nv50_head_atom_get_encoder(asyh); + if (!encoder) + return; + + outp = nv50_real_outp(encoder); + if (!outp) + return; func->set_src(head, outp->or, nv50_crc_source_type(outp, asyh->crc.src), &crc->ctx[crc->ctx_idx]); -- 2.17.1
[Nouveau] [PATCH] device: fix missing check on list iterator
The bug is here: lo = pstate->base.domain[domain->name]; The list iterator 'pstate' will point to a bogus position containing HEAD if the list is empty or no element is found. This case should be checked before any use of the iterator, otherwise it will lead to a invalid memory access. To fix this bug, add an check. Use a new value 'iter' as the list iterator, while use the old value 'pstate' as a dedicated variable to point to the found element. Cc: sta...@vger.kernel.org Fixes: 9838366c1597d ("drm/nouveau/device: initial control object class, with pstate control methods") Signed-off-by: Xiaomeng Tong --- drivers/gpu/drm/nouveau/nvkm/engine/device/ctrl.c | 11 --- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/device/ctrl.c b/drivers/gpu/drm/nouveau/nvkm/engine/device/ctrl.c index ce774579c89d..6b768635e8ba 100644 --- a/drivers/gpu/drm/nouveau/nvkm/engine/device/ctrl.c +++ b/drivers/gpu/drm/nouveau/nvkm/engine/device/ctrl.c @@ -72,7 +72,7 @@ nvkm_control_mthd_pstate_attr(struct nvkm_control *ctrl, void *data, u32 size) } *args = data; struct nvkm_clk *clk = ctrl->device->clk; const struct nvkm_domain *domain; - struct nvkm_pstate *pstate; + struct nvkm_pstate *pstate = NULL, *iter; struct nvkm_cstate *cstate; int i = 0, j = -1; u32 lo, hi; @@ -103,11 +103,16 @@ nvkm_control_mthd_pstate_attr(struct nvkm_control *ctrl, void *data, u32 size) return -EINVAL; if (args->v0.state != NVIF_CONTROL_PSTATE_ATTR_V0_STATE_CURRENT) { - list_for_each_entry(pstate, &clk->states, head) { - if (i++ == args->v0.state) + list_for_each_entry(iter, &clk->states, head) { + if (i++ == args->v0.state) { + pstate = iter; break; + } } + if (!pstate) + return -EINVAL; + lo = pstate->base.domain[domain->name]; hi = lo; list_for_each_entry(cstate, &pstate->list, head) { -- 2.17.1
Re: [Nouveau] [PATCH] device: fix missing check on list iterator
On Sat, 26 Mar 2022 22:38:05 -0700, Guenter Roeck wrote: > > @@ -103,11 +103,16 @@ nvkm_control_mthd_pstate_attr(struct nvkm_control > > *ctrl, void *data, u32 size) > > return -EINVAL; > > > > if (args->v0.state != NVIF_CONTROL_PSTATE_ATTR_V0_STATE_CURRENT) { > > - list_for_each_entry(pstate, &clk->states, head) { > > - if (i++ == args->v0.state) > > + list_for_each_entry(iter, &clk->states, head) { > > + if (i++ == args->v0.state) { > > + pstate = iter; > > Is iter and the assignment really necessary ? Unless I am missing something, > list_for_each_entry() always assigns pos (pstate/iter), even if the list is > empty. If nothing is found, pstate would be NULL at the end, so the pstate will not be NULL at the end! so the assignment is necessary! #define list_for_each_entry(pos, head, member) \ for (pos = __container_of((head)->next, pos, member); \ &pos->member != (head); \ pos = __container_of(pos->member.next, pos, member)) -- Xiaomeng Tong
Re: [Nouveau] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
> From: Xiaomeng Tong > > Sent: 03 March 2022 07:27 > > > > On Thu, 3 Mar 2022 04:58:23 +, David Laight wrote: > > > on 3 Mar 2022 10:27:29 +0800, Xiaomeng Tong wrote: > > > > 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. > > > > Yes, the rule is "the HEAD's container_of pointer should never be > > calculated at all outside the loop", but how do you make sure everyone > > follows this rule? > > Everyone makes mistakes, but we can eliminate them all from the beginning > > with the help of compiler which can catch such use-after-loop things. > > > > > > IOW, you would dereference a (NULL + offset_of_member) address here. > > > > > >Where? > > > > In the case where a developer do not follows the above rule, and mistakely > > access a non-list-head member of the HEAD's container_of pointer outside > > the loop. For example: > > struct req{ > > int a; > > struct list_head h; > > } > > struct req *r; > > list_for_each_entry(r, HEAD, h) { > > if (r->a == 0x10) > > break; > > } > > // the developer made a mistake: he didn't take this situation into > > // account where all entries in the list are *r->a != 0x10*, and now > > // the r is the HEAD's container_of pointer. > > r->a = 0x20; > > Thus the "r->a = 0x20" would dereference a (NULL + offset_of_member) > > address here. > > That is just a bug. > No different to failing to check anything else might 'return' > a NULL pointer. Yes, but it‘s a mistake everyone has made and will make, we should avoid this at the beginning with the help of compiler. > Because it is a NULL dereference you find out pretty quickly. AFAIK,NULL dereference is a undefined bahavior, can compiler quickly catch it? Or it can only be applied to some simple/restricted cases. > The existing loop leaves you with a valid pointer to something > that isn't a list item. > > > > > 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'. > > > > you still can not make sure everyone follows this rule: > > "do not use iterator outside the loop" without the help of compiler, > > because item is declared outside the loop. > > That one has 'iter' defined in the loop. Oh, sorry, I misunderstood. Then this is the same way with my way of list_for_each_entry_inside(pos, type, head, member), which declare the iterator inside the loop. You go further and make things like "&pos->member == (head)" goes away to avoid calculate the HEAD's container_of pointer, although the calculation is harmless. > > > BTW, to avoid ambiguity,the "alternative definitions" here i asked is > > something from you in this context: > > "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." > > I was thinking of something like: > if ((pos = list_first)), 1) pos = NULL else > so that unchecked dereferences after the loop will be detectable > as NULL pointer offsets - but that in itself isn't enough to avoid > other warnings. > Do you mean put this right after the loop (I changed somthing that i do not understand, please correct me if i am worng, thanks): if (pos == list_first) pos = NULL; else and compiler can detect the following NUL
Re: [Nouveau] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
correct for typo: -for (struct list_head *list = head->next, cond = (struct list_head *)-1; cond == (struct list_head *)-1; cond = NULL) \ +for (struct list_head *list = head->next, *cond = (struct list_head *)-1; cond == (struct list_head *)-1; cond = NULL) \ -- Xiaomeng Tong
Re: [Nouveau] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Wed, 2 Mar 2022 14:04:06 +, David Laight wrote: > 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. 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. IOW, you would dereference a (NULL + offset_of_member) address here. Please remind me if i missed something, thanks. > 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. Can you share your "alternative definitions" details? thanks! -- Xiaomeng Tong
Re: [Nouveau] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
> I think this would make sense, it would mean you only assign the containing > element on valid elements. > > I was thinking something along the lines of: > > #define list_for_each_entry(pos, head, member) > \ > for (struct list_head *list = head->next, typeof(pos) pos; \ >list == head ? 0 : (( pos = list_entry(pos, list, member), 1)); > \ >list = list->next) > > Although the initialization block of the for loop is not valid C, I'm > not sure there is any way to declare two variables of a different type > in the initialization part of the loop. It can be done using a *nested loop*, like this: #define list_for_each_entry(pos, head, member) \ for (struct list_head *list = head->next, cond = (struct list_head *)-1; cond == (struct list_head *)-1; cond = NULL) \ for (typeof(pos) pos; \ list == head ? 0 : (( pos = list_entry(pos, list, member), 1)); \ list = list->next) > > I believe all this does is get rid of the &pos->member == (head) check > to terminate the list. Indeed, although the original way is harmless. > It alone will not fix any of the other issues that using the iterator > variable after the loop currently has. Yes, but I stick with the list_for_each_entry_inside(pos, type, head, member) way to make the iterator invisiable outside the loop (before and after the loop). It is maintainable longer-term than "type(pos) pos" one and perfect. see my explain: https://lore.kernel.org/lkml/20220302093106.8402-1-xiam0nd.t...@gmail.com/ and list_for_each_entry_inside(pos, type, head, member) patch here: https://lore.kernel.org/lkml/20220301075839.4156-3-xiam0nd.t...@gmail.com/ -- Xiaomeng Tong
Re: [Nouveau] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Thu, 3 Mar 2022 04:58:23 +, David Laight wrote: > on 3 Mar 2022 10:27:29 +0800, Xiaomeng Tong wrote: > > 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. Yes, the rule is "the HEAD's container_of pointer should never be calculated at all outside the loop", but how do you make sure everyone follows this rule? Everyone makes mistakes, but we can eliminate them all from the beginning with the help of compiler which can catch such use-after-loop things. > > IOW, you would dereference a (NULL + offset_of_member) address here. > >Where? In the case where a developer do not follows the above rule, and mistakely access a non-list-head member of the HEAD's container_of pointer outside the loop. For example: struct req{ int a; struct list_head h; } struct req *r; list_for_each_entry(r, HEAD, h) { if (r->a == 0x10) break; } // the developer made a mistake: he didn't take this situation into // account where all entries in the list are *r->a != 0x10*, and now // the r is the HEAD's container_of pointer. r->a = 0x20; Thus the "r->a = 0x20" would dereference a (NULL + offset_of_member) address here. > > 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'. you still can not make sure everyone follows this rule: "do not use iterator outside the loop" without the help of compiler, because item is declared outside the loop. BTW, to avoid ambiguity,the "alternative definitions" here i asked is something from you in this context: "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." > > > > > 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. You mean it would be a burden on search? can you show me some examples? Or you mean it is too long the list_for_each_entry_inside* string to live in one single line, and should spilt into two line? If it is the case, there are 2 way to mitigate it. 1. pass a shorter t as struct type to the macro 2. after all list_for_each_entry macros be replaced with list_for_each_entry_inside, remove the list_for_each_entry implementations and rename all list_for_each_entry_inside use back to the origin list_for_each_entry in a single patch. For me, it is acceptable and not a such big problem. -- Xiaomeng Tong
Re: [Nouveau] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Mon, 28 Feb 2022 16:41:04 -0800, Linus Torvalds wrote: > > But basically to _me_, the important part is that the end result is > maintainable longer-term. I couldn't agree more. And because of that, I stick with the following approach because it's maintainable longer-term than "type(pos) pos" one: Implements a new macro for each list_for_each_entry* with _inside suffix. #define list_for_each_entry_inside(pos, type, head, member) I have posted a patch series here to demonstrate this approach: https://lore.kernel.org/lkml/20220301075839.4156-3-xiam0nd.t...@gmail.com/ Although we need replace all the use of list_for_each_entry* (15000+) with list_for_each_entry*_inside, the work can be done gradually rather than all at once. We can incrementally replace these callers until all these in the kernel are completely updated with *_inside* one. At that time, we can just remove the implements of origin macros and rename the *_inside* macro back to the origin name just in one single patch. And the "type(pos) pos" approach need teach developers to "not initialize the iterator variable, otherwise the use-after-loop will not be reported by compiler", which is unreasonable and impossible for all developers. And it will mess up the following code logic and no warnning reported by compiler, even without initializing "ext" at the beginning: void foo(struct mem_extent *arg) { struct mem_extent *ext; // used both for iterator and normal ptr ... ext = arg; // this assignment can alse be done in another bar() func ... list_for_each_entry(ext, head, member) { if (found(ext)) break; } ... // use ext after the loop ret = ext; } If the loop hit the break, the last "ret" will be the found ext iterator. However, if the "type(pos) pos" approach applied, the last "ret" will be "arg" which is not the intention of the developers, because the "ext" is two different variables inside and outside the loop. Thus, my idea is *better a finger off than always aching*, let's choose the "list_for_each_entry_inside(pos, type, head, member)" approach. > It turns out that just syntactically, it's really nice to give the > type of the iterator from outside the way we do now. Yeah, it may be a > bit odd, and maybe it's partly because I'm so used to the > "list_for_each_list_entry()" syntax, but moving the type into the loop > construct really made it nasty - either one very complex line, or > having to split it over two lines which was even worse. > > Maybe the place I looked at just happened to have a long typename, but > it's basically always going to be a struct, so it's never a _simple_ > type. And it just looked very odd adn unnatural to have the type as > one of the "arguments" to that list_for_each_entry() macro. we can pass a shorter type name to list_for_each_entry_inside, thus no need to split it over two lines. Actually it is not a big problem. + #define t struct sram_bank_info - list_for_each_entry(pos, head, member) { + list_for_each_entry_inside(pos, t, head, member) { I put the type at the second argument not the first to avoid messing up the pattern match in some coccinelle scripts. > (b) gives us a nice warning for any normal use-after-loop case > (unless you explicitly initialized it like that > sgx_mmu_notifier_release() function did for no good reason sometimes developers can be confused by the reported warnning: "used without having been initialized", and can not figure out immediately that "oh, now i am using another different variable but with the same name of the loop iterator variable", which has changed the programming habits of developers. > (c) also guarantees that even if you don't get a warning, > non-converted (or newly written) bad code won't actually _work_ > > so you end up getting the new rules without any ambiguity or mistaken It will lead to a wrong/NULL pointer dereference if the pointer is used anywhere else, depend on which value is used to initialized with. Best regard, -- Xiaomeng Tong
Re: [Nouveau] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Thu, 3 Mar 2022 12:18:24 +, Daniel Thompson wrote: > On Thu, Mar 03, 2022 at 03:26:57PM +0800, Xiaomeng Tong wrote: > > On Thu, 3 Mar 2022 04:58:23 +, David Laight wrote: > > > on 3 Mar 2022 10:27:29 +0800, Xiaomeng Tong wrote: > > > > 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. > > > > Yes, the rule is "the HEAD's container_of pointer should never be > > calculated at all outside the loop", but how do you make sure everyone > > follows this rule? > > Your formulation of the rule is correct: never run container_of() on HEAD > pointer. Actually, it is not my rule. My rule is that never access other members of the struct except for the list_head member after the loop, because this is a invalid member after loop exit, but valid for the list_head member which just is HEAD and the lately caculation (&pos->head) seems harmless. I have considered the case that the HEAD's container "pos" is layouted across the max and the min address boundary, which means the address of HEAD is likely 0x60, and the address of pos is likely 0xffe0. It seems ok to caculate pos with: ((type *)(__mptr - offsetof(type, member))); and it seems ok to caculate head outside the loop with: if (&pos->head == &HEAD) return NULL; The only case I can think of with the rule "never run container_of() on HEAD" must be followed is when the first argument (which is &HEAD) passing to container_of() is NULL + some offset, it may lead to the resulting "pos->member" access being a NULL dereference. But maybe the caller can take the responsibility to check if it is NULL, not container_of() itself. Please remind me if i missed somthing, thanks. > > However the rule that is introduced by list_for_each_entry_inside() is > *not* this rule. The rule it introduces is: never access the iterator > variable outside the loop. Sorry for the confusion, indeed, that is two *different* rule. > > Making the iterator NULL on loop exit does follow the rule you proposed > but using a different technique: do not allow HEAD to be stored in the > iterator variable after loop exit. This also makes it impossible to run > container_of() on the HEAD pointer. > It does not. My rule is: never access the iterator variable outside the loop. The "Making the iterator NULL on loop exit" way still leak the pos with NULL outside the loop, may lead to a NULL deference. > > > Everyone makes mistakes, but we can eliminate them all from the beginning > > with the help of compiler which can catch such use-after-loop things. > > Indeed but if we introduce new interfaces then we don't have to worry > about existing usages and silent regressions. Code will have been > written knowing the loop can exit with the iterator set to NULL. Yes, it is more simple and compatible with existing interfaces. Howerver, you should make every developers to remember that "pos will be set NULL on loop exit", which is unreasonable and impossible for *every* single person. Otherwise the mis-use-after-loop will lead to a NULL dereference. But we can kill this problem by declaring iterator inside the loop and the complier will catch it if somebody mis-use-after-loop. > > Sure it is still possible for programmers to make mistakes and > dereference the NULL pointer but C programmers are well training w.r.t. > NULL pointer checking so such mistakes are much less likely than with > the current list_for_each_entry() macro. This risk must be offset > against the way a NULLify approach can lead to more elegant code when we > are doing a list search. > Yes, the NULLify approach is better than the current list_for_each_entry() macro, but i stick with that the list_for_each_entry_inside() way is best and perfect _technically_. Thus, my idea is *better a finger off than always aching*, let's settle this damn problem once and for all, with list_for_each_entry_inside(). -- Xiaomeng Tong