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 == ? ((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 ">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 NULL derefernce?
But if the NULL derefernce is just right after the loop originally,
it will be masked by the *else* keyword。
> > > > 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?
>
> The whole business of having to save the pointer to the located item
> before breaking the loop, remembering to have set it to NULL earlier etc.
Ok, i see. And then you need pass a "item" to the list_for_each_entry macro
as a new argument.
>
> It is so much better if you can
Re: [Nouveau] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Wed, Mar 02, 2022 at 12:07:04PM -0800, Kees Cook wrote:
> On Wed, Mar 02, 2022 at 10:29:31AM +0100, Rasmus Villemoes wrote:
> > This won't help the current issue (because it doesn't exist and might
> > never), but just in case some compiler people are listening, I'd like to
> > have some sort of way to tell the compiler "treat this variable as
> > uninitialized from here on". So one could do
> >
> > #define kfree(p) do { __kfree(p); __magic_uninit(p); } while (0)
> >
> > with __magic_uninit being a magic no-op that doesn't affect the
> > semantics of the code, but could be used by the compiler's "[is/may be]
> > used uninitialized" machinery to flag e.g. double frees on some odd
> > error path etc. It would probably only work for local automatic
> > variables, but it should be possible to just ignore the hint if p is
> > some expression like foo->bar or has side effects. If we had that, the
> > end-of-loop test could include that to "uninitialize" the iterator.
>
> I've long wanted to change kfree() to explicitly set pointers to NULL on
> free. https://github.com/KSPP/linux/issues/87
You also need to be a bit careful with existing code because there are
places which do things like:
drivers/usb/host/r8a66597-hcd.c
424 kfree(dev);
^^^
425
426 for (port = 0; port < r8a66597->max_root_hub; port++) {
427 if (r8a66597->root_hub[port].dev == dev) {
^^^
428 r8a66597->root_hub[port].dev = NULL;
429 break;
430 }
431 }
Printing the freed pointer in debug code is another thing people do.
regards,
dan carpenter
Re: [Nouveau] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
From: Xiaomeng Tong
> Sent: 03 March 2022 02:27
>
> 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.
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 == ? ((item = NULL),0) : ((item =
list_item(iter),1));
iter = item->member->next) {
...
With a bit of casting you can use 'item' to hold 'iter'.
>
> > 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)
Re: [Nouveau] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
> On 3. Mar 2022, at 05:58, David Laight wrote:
>
> From: Xiaomeng Tong
>> Sent: 03 March 2022 02:27
>>
>> 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.
>
> 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 == ? ((item = NULL),0) : ((item =
> list_item(iter),1));
> iter = item->member->next) {
> ...
> With a bit of casting you can use 'item' to hold 'iter'.
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.
I believe all this does is get rid of the >member == (head) check
to terminate the list.
It alone will not fix any of the other issues that using the iterator
variable after the loop currently has.
AFAIK Adrián Moreno is working on doing something along those lines
for the list iterator in openvswitch (that was similar to the kernel
one before) [1].
I *think* they don't declare 'pos' within the loop which we *do want*
to avoid any uses of it after the loop.
(If pos is not declared in the initialization block, shadowing the
*outer* pos, it would just point to the last element of the list or stay
uninitialized if the list is empty).
[1] https://www.mail-archive.com/ovs-dev@openvswitch.org/msg63497.html
>
>>
>>> 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)
>
- Jakob
Re: [Nouveau] [PATCH 0/6] Remove usage of list iterator past the loop body
From: Dan Carpenter > Sent: 07 March 2022 15:01 > > Updating this API is risky because some places rely on the old behavior > and not all of them have been updated. Here are some additional places > you might want to change. I really can't help thinking that trying to merge this patch is actually impossible. It affects far too many different parts of the tree. Since (I believe) this is a doubly linked list with forwards and backwards pointers that point to a 'node' (not that there is a nice comment to that effect in the header - and there are lots of ways to do linked lists) the 'head' pretty much has to be a 'node'. I'd write the following new defines (but I might be using the old names here): list_first(head, field) First item, NULL if empty. list_last(head, field) Last item NULL if empty. list_next(head, item, field) Item after 'item', NULL if last. list_prev(head, item. field) Item before 'item', NULL if first. You get (something like): #define list_first(head, field) \ head->next == ? NULL : list_item(head->next, field) (probably needs typeof(item) from somewhere). The iterator loop is then just: #define loop_iterate(item, head, field) \ for (item = list_first(head, field); item; \ item = list_next(head, item, field) I'm not sure, but making the 'head' be a structure that contains a single member that is a 'node' might help type checking. Then all the code that uses the current defines can slowly be moved over (probably a couple of releases) before the existing defines are deleted. That should simplify all the open-coded search loops that are just as likely to be buggy (possibly more so). David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Re: [Nouveau] [PATCH 0/6] Remove usage of list iterator past the loop body
Updating this API is risky because some places rely on the old behavior and not all of them have been updated. Here are some additional places you might want to change. drivers/usb/host/uhci-q.c:466 link_async() warn: iterator used outside loop: 'pqh' drivers/infiniband/core/mad.c:968 ib_get_rmpp_segment() warn: iterator used outside loop: 'mad_send_wr->cur_seg' drivers/opp/debugfs.c:208 opp_migrate_dentry() warn: iterator used outside loop: 'new_dev' drivers/staging/greybus/audio_codec.c:602 gbcodec_mute_stream() warn: iterator used outside loop: 'module' drivers/staging/media/atomisp/pci/atomisp_acc.c:508 atomisp_acc_load_extensions() warn: iterator used outside loop: 'acc_fw' drivers/perf/thunderx2_pmu.c:814 tx2_uncore_pmu_init_dev() warn: iterator used outside loop: 'rentry' drivers/gpu/drm/nouveau/nvkm/engine/device/ctrl.c:111 nvkm_control_mthd_pstate_attr() warn: iterator used outside loop: 'pstate' drivers/gpu/drm/panfrost/panfrost_mmu.c:203 panfrost_mmu_as_get() warn: iterator used outside loop: 'lru_mmu' drivers/media/usb/uvc/uvc_v4l2.c:885 uvc_ioctl_enum_input() warn: iterator used outside loop: 'iterm' drivers/media/usb/uvc/uvc_v4l2.c:896 uvc_ioctl_enum_input() warn: iterator used outside loop: 'iterm' drivers/scsi/dc395x.c:3596 device_alloc() warn: iterator used outside loop: 'p' drivers/net/ethernet/mellanox/mlx4/alloc.c:379 __mlx4_alloc_from_zone() warn: iterator used outside loop: 'curr_node' fs/ocfs2/dlm/dlmdebug.c:573 lockres_seq_start() warn: iterator used outside loop: 'res' This patchset fixes 3 bugs. Initially when it's merged it's probably going to introduce some bugs because there are likely other places which rely on the old behavior. In an ideal world, with the new API the compiler would warn about uninitialized variables, but unfortunately that warning is disabled by default so we still have to rely on kbuild/Clang/Smatch to find the bugs. But hopefully the new API encourages people to write clearer code so it prevents bugs in the long run. regards, dan carpenter
Re: [Nouveau] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
From: Xiaomeng Tong > Sent: 02 March 2022 09:31 > > 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 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. It also doesn't need an extra variable defined in the for() statement so can be back-ported to older kernels without required declaration in the middle of blocks. 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. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
[Nouveau] [PATCH] drm/nouveau/bios: Rename prom_init() and friends functions
While working on powerpc headers, I ended up with the following error.
drivers/gpu/drm/nouveau/nvkm/subdev/bios/shadowrom.c:48:1: error:
conflicting types for 'prom_init'; have 'void *(struct nvkm_bios *, const char
*)'
make[5]: *** [scripts/Makefile.build:288:
drivers/gpu/drm/nouveau/nvkm/subdev/bios/shadowrom.o] Error 1
powerpc and a few other architectures have a prom_init() global function.
One day or another it will conflict with the one in shadowrom.c
Those being static, they can easily be renamed. Do it.
Signed-off-by: Christophe Leroy
---
drivers/gpu/drm/nouveau/nvkm/subdev/bios/shadowrom.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/bios/shadowrom.c
b/drivers/gpu/drm/nouveau/nvkm/subdev/bios/shadowrom.c
index ffa4b395220a..9c951e90e622 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/bios/shadowrom.c
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/bios/shadowrom.c
@@ -25,7 +25,7 @@
#include
static u32
-prom_read(void *data, u32 offset, u32 length, struct nvkm_bios *bios)
+nvbios_rom_read(void *data, u32 offset, u32 length, struct nvkm_bios *bios)
{
struct nvkm_device *device = data;
u32 i;
@@ -38,14 +38,14 @@ prom_read(void *data, u32 offset, u32 length, struct
nvkm_bios *bios)
}
static void
-prom_fini(void *data)
+nvbios_rom_fini(void *data)
{
struct nvkm_device *device = data;
nvkm_pci_rom_shadow(device->pci, true);
}
static void *
-prom_init(struct nvkm_bios *bios, const char *name)
+nvbios_rom_init(struct nvkm_bios *bios, const char *name)
{
struct nvkm_device *device = bios->subdev.device;
if (device->card_type == NV_40 && device->chipset >= 0x4c)
@@ -57,8 +57,8 @@ prom_init(struct nvkm_bios *bios, const char *name)
const struct nvbios_source
nvbios_rom = {
.name = "PROM",
- .init = prom_init,
- .fini = prom_fini,
- .read = prom_read,
+ .init = nvbios_rom_init,
+ .fini = nvbios_rom_fini,
+ .read = nvbios_rom_read,
.rw = false,
};
--
2.34.1
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] [Intel-gfx] [PATCH 6/6] treewide: remove check of list iterator against head past the loop body
On 28/02/2022 11:08, Jakob Koschel wrote:
When list_for_each_entry() completes the iteration over the whole list
without breaking the loop, the iterator value will be a bogus pointer
computed based on the head element.
While it is safe to use the pointer to determine if it was computed
based on the head element, either with list_entry_is_head() or
>member == head, using the iterator variable after the loop should
be avoided.
In preparation to limiting the scope of a list iterator to the list
traversal loop, use a dedicated pointer to point to the found element.
Signed-off-by: Jakob Koschel
[snip until i915 parts]
drivers/gpu/drm/i915/gem/i915_gem_context.c | 14 +++---
.../gpu/drm/i915/gem/i915_gem_execbuffer.c| 15 ---
drivers/gpu/drm/i915/gt/intel_ring.c | 15 ---
[snip]
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c
b/drivers/gpu/drm/i915/gem/i915_gem_context.c
index 00327b750fbb..80c79028901a 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -107,25 +107,27 @@ static void lut_close(struct i915_gem_context *ctx)
radix_tree_for_each_slot(slot, >handles_vma, , 0) {
struct i915_vma *vma = rcu_dereference_raw(*slot);
struct drm_i915_gem_object *obj = vma->obj;
- struct i915_lut_handle *lut;
+ struct i915_lut_handle *lut = NULL;
+ struct i915_lut_handle *tmp;
if (!kref_get_unless_zero(>base.refcount))
continue;
spin_lock(>lut_lock);
- list_for_each_entry(lut, >lut_list, obj_link) {
- if (lut->ctx != ctx)
+ list_for_each_entry(tmp, >lut_list, obj_link) {
+ if (tmp->ctx != ctx)
continue;
- if (lut->handle != iter.index)
+ if (tmp->handle != iter.index)
continue;
- list_del(>obj_link);
+ list_del(>obj_link);
+ lut = tmp;
break;
}
spin_unlock(>lut_lock);
- if (>obj_link != >lut_list) {
+ if (lut) {
i915_lut_handle_free(lut);
radix_tree_iter_delete(>handles_vma, , slot);
Looks okay although personally I would have left lut as is for a smaller
diff and introduced a new local like 'found' or 'unlinked'.
i915_vma_close(vma);
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
index 1736efa43339..fda9e3685ad2 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
@@ -2444,7 +2444,8 @@ static struct i915_request *eb_throttle(struct
i915_execbuffer *eb, struct intel
{
struct intel_ring *ring = ce->ring;
struct intel_timeline *tl = ce->timeline;
- struct i915_request *rq;
+ struct i915_request *rq = NULL;
+ struct i915_request *tmp;
/*
* Completely unscientific finger-in-the-air estimates for suitable
@@ -2460,15 +2461,17 @@ static struct i915_request *eb_throttle(struct
i915_execbuffer *eb, struct intel
* claiming our resources, but not so long that the ring completely
* drains before we can submit our next request.
*/
- list_for_each_entry(rq, >requests, link) {
- if (rq->ring != ring)
+ list_for_each_entry(tmp, >requests, link) {
+ if (tmp->ring != ring)
continue;
- if (__intel_ring_space(rq->postfix,
- ring->emit, ring->size) > ring->size / 2)
+ if (__intel_ring_space(tmp->postfix,
+ ring->emit, ring->size) > ring->size /
2) {
+ rq = tmp;
break;
+ }
}
- if (>link == >requests)
+ if (!rq)
return NULL; /* weird, we will check again later for real */
Alternatively, instead of break could simply do "return
i915_request_get(rq);" and replace the end of the function after the
loop with "return NULL;". A bit smaller diff, or at least less "spread
out" over the function, so might be easier to backport stuff touching
this area in the future. But looks correct as is.
return i915_request_get(rq);
diff --git a/drivers/gpu/drm/i915/gt/intel_ring.c
b/drivers/gpu/drm/i915/gt/intel_ring.c
index 2fdd52b62092..4881c4e0c407 100644
--- a/drivers/gpu/drm/i915/gt/intel_ring.c
+++ b/drivers/gpu/drm/i915/gt/intel_ring.c
@@ -191,24 +191,27 @@ wait_for_space(struct intel_ring *ring,
struct intel_timeline *tl,
unsigned int bytes)
{
- struct i915_request *target;
+ struct
Re: [Nouveau] [PATCH] drm/nouveau/bios: Rename prom_init() and friends functions
Le 04/03/2022 à 21:24, Lyude Paul a écrit :
> This mostly looks good to me. Just one question (and one comment down below
> that needs addressing). Is this with ppc32? (I ask because ppc64le doesn't
> seem to hit this compilation error).
That's with PPC64, see
http://kisskb.ellerman.id.au/kisskb/branch/chleroy/head/252ba609bea83234d2e35841c19ae84c67b43ec7/
But that's not (yet) with the mainline tree. That's work I'm doing to
cleanup our asm/asm-protoypes.h header.
Since commit 4efca4ed05cb ("kbuild: modversions for EXPORT_SYMBOL() for
asm") that file is dedicated to prototypes of functions defined in
assembly. Therefore I'm trying to dispatch C functions prototypes in
other headers. I wanted to move prom_init() prototype into asm/prom.h
and then I hit the problem.
In the beginning I was thinking about just changing the name of the
function in powerpc, but as I see that M68K, MIPS and SPARC also have a
prom_init() function, I thought it would be better to change the name in
shadowrom.c to avoid any future conflict like the one I got while
reworking the headers.
>> @@ -57,8 +57,8 @@ prom_init(struct nvkm_bios *bios, const char *name)
>> const struct nvbios_source
>> nvbios_rom = {
>> .name = "PROM",
>> - .init = prom_init,
>> - .fini = prom_fini,
>> - .read = prom_read,
>> + .init = nvbios_rom_init,
>> + .fini = nvbios_rom_fini,
>> + .read = nvbios_rom_read,
>
> Seeing as the source name is prom, I think using the naming convention
> nvbios_prom_* would be better then nvbios_rom_*.
>
Yes I wasn't sure about the best naming as the file name is shadowrom.c
and not shadowprom.c.
I will send v2 using nvbios_prom_* as a name.
Christophe
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 >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] [Kgdb-bugreport] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
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. 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. 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. > 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. 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. Daniel.
Re: [Nouveau] [PATCH] drm/nouveau/bios: Rename prom_init() and friends functions
Le 05/03/2022 à 08:38, Christophe Leroy a écrit :
Le 04/03/2022 à 21:24, Lyude Paul a écrit :
This mostly looks good to me. Just one question (and one comment down
below
that needs addressing). Is this with ppc32? (I ask because ppc64le
doesn't
seem to hit this compilation error).
That's with PPC64, see
http://kisskb.ellerman.id.au/kisskb/branch/chleroy/head/252ba609bea83234d2e35841c19ae84c67b43ec7/
But that's not (yet) with the mainline tree. That's work I'm doing to
cleanup our asm/asm-protoypes.h header.
Since commit 4efca4ed05cb ("kbuild: modversions for EXPORT_SYMBOL() for
asm") that file is dedicated to prototypes of functions defined in
assembly. Therefore I'm trying to dispatch C functions prototypes in
other headers. I wanted to move prom_init() prototype into asm/prom.h
and then I hit the problem.
In the beginning I was thinking about just changing the name of the
function in powerpc, but as I see that M68K, MIPS and SPARC also have a
prom_init() function, I thought it would be better to change the name in
shadowrom.c to avoid any future conflict like the one I got while
reworking the headers.
@@ -57,8 +57,8 @@ prom_init(struct nvkm_bios *bios, const char *name)
const struct nvbios_source
nvbios_rom = {
.name = "PROM",
- .init = prom_init,
- .fini = prom_fini,
- .read = prom_read,
+ .init = nvbios_rom_init,
+ .fini = nvbios_rom_fini,
+ .read = nvbios_rom_read,
Seeing as the source name is prom, I think using the naming convention
nvbios_prom_* would be better then nvbios_rom_*.
Yes I wasn't sure about the best naming as the file name is shadowrom.c
and not shadowprom.c.
I will send v2 using nvbios_prom_* as a name.
While preparing v2 I remembered that in fact, I called the functions
nvbios_rom_* because the name of the nvbios_source struct is nvbios_rom,
so for me it made sense to use the name of the struct as a prefix for
the functions.
So I'm OK to change it to nvbios_prom_* but it looks less logical to me.
Please confirm you still prefer nvbios_prom as prefix to the function names.
Christophe
Re: [Nouveau] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Wed, Mar 02, 2022 at 10:29:31AM +0100, Rasmus Villemoes wrote:
> This won't help the current issue (because it doesn't exist and might
> never), but just in case some compiler people are listening, I'd like to
> have some sort of way to tell the compiler "treat this variable as
> uninitialized from here on". So one could do
>
> #define kfree(p) do { __kfree(p); __magic_uninit(p); } while (0)
>
> with __magic_uninit being a magic no-op that doesn't affect the
> semantics of the code, but could be used by the compiler's "[is/may be]
> used uninitialized" machinery to flag e.g. double frees on some odd
> error path etc. It would probably only work for local automatic
> variables, but it should be possible to just ignore the hint if p is
> some expression like foo->bar or has side effects. If we had that, the
> end-of-loop test could include that to "uninitialize" the iterator.
I've long wanted to change kfree() to explicitly set pointers to NULL on
free. https://github.com/KSPP/linux/issues/87
The thing stopping a trivial transformation of kfree() is:
kfree(get_some_pointer());
I would argue, though, that the above is poor form: the thing holding
the pointer should be the thing freeing it, so these cases should be
refactored and kfree() could do the NULLing by default.
Quoting myself in the above issue:
Without doing massive tree-wide changes, I think we need compiler
support. If we had something like __builtin_is_lvalue(), we could
distinguish function returns from lvalues. For example, right now a
common case are things like:
kfree(get_some_ptr());
But if we could at least gain coverage of the lvalue cases, and detect
them statically at compile-time, we could do:
#define __kfree_and_null(x) do { __kfree(*x); *x = NULL; } while (0)
#define kfree(x) __builtin_choose_expr(__builtin_is_lvalue(x),
__kfree_and_null(&(x)), __kfree(x))
Alternatively, we could do a tree-wide change of the former case (findable
with Coccinelle) and change them into something like kfree_no_null()
and redefine kfree() itself:
#define kfree_no_null(x) do { void *__ptr = (x); __kfree(__ptr); } while (0)
#define kfree(x) do { __kfree(x); x = NULL; } while (0)
--
Kees Cook
Re: [Nouveau] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Wed, Mar 02, 2022 at 12:18:45PM -0800, Linus Torvalds wrote: > On Wed, Mar 2, 2022 at 12:07 PM Kees Cook wrote: > > > > I've long wanted to change kfree() to explicitly set pointers to NULL on > > free. https://github.com/KSPP/linux/issues/87 > > We've had this discussion with the gcc people in the past, and gcc > actually has some support for it, but it's sadly tied to the actual > function name (ie gcc has some special-casing for "free()") > > See > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94527 > > for some of that discussion. > > Oh, and I see some patch actually got merged since I looked there last > so that you can mark "deallocator" functions, but I think it's only > for the context matching, not for actually killing accesses to the > pointer afterwards. Ah! I missed that getting added in GCC 11. But yes, there it is: https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-malloc-function-attribute Hah, now we may need to split __malloc from __alloc_size. ;) I'd still like the NULL assignment behavior, though, since some things can easily avoid static analysis. -- Kees Cook
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 == ? ((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 Wed, Mar 2, 2022 at 12:07 PM Kees Cook wrote: > > I've long wanted to change kfree() to explicitly set pointers to NULL on > free. https://github.com/KSPP/linux/issues/87 We've had this discussion with the gcc people in the past, and gcc actually has some support for it, but it's sadly tied to the actual function name (ie gcc has some special-casing for "free()") See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94527 for some of that discussion. Oh, and I see some patch actually got merged since I looked there last so that you can mark "deallocator" functions, but I think it's only for the context matching, not for actually killing accesses to the pointer afterwards. Linus
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
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.
Because it is a NULL dereference you find out pretty quickly.
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 == ? ((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.
> 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.
> > > 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?
The whole business of having to save the pointer to the located item
before breaking the loop, remembering to have set it to NULL earlier etc.
It is so much better if you can just do:
if (found)
break;
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT,
UK
Registration No: 1397386 (Wales)
Re: [Nouveau] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On 02/03/2022 00.55, Linus Torvalds wrote:
> On Tue, Mar 1, 2022 at 3:19 PM David Laight wrote:
>>
> With the "don't use iterator outside the loop" approach, the exact
> same code works in both the old world order and the new world order,
> and you don't have the semantic confusion. And *if* you try to use the
> iterator outside the loop, you'll _mostly_ (*) get a compiler warning
> about it not being initialized.
>
> Linus
>
> (*) Unless somebody initializes the iterator pointer pointlessly.
> Which clearly does happen. Thus the "mostly". It's not perfect, and
> that's most definitely not nice - but it should at least hopefully
> make it that much harder to mess up.
This won't help the current issue (because it doesn't exist and might
never), but just in case some compiler people are listening, I'd like to
have some sort of way to tell the compiler "treat this variable as
uninitialized from here on". So one could do
#define kfree(p) do { __kfree(p); __magic_uninit(p); } while (0)
with __magic_uninit being a magic no-op that doesn't affect the
semantics of the code, but could be used by the compiler's "[is/may be]
used uninitialized" machinery to flag e.g. double frees on some odd
error path etc. It would probably only work for local automatic
variables, but it should be possible to just ignore the hint if p is
some expression like foo->bar or has side effects. If we had that, the
end-of-loop test could include that to "uninitialize" the iterator.
Maybe sparse/smatch or some other static analyzer could implement such a
magic thing? Maybe it's better as a function attribute
[__attribute__((uninitializes(1)))] to avoid having to macrofy all
functions that release resources.
Rasmus
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 (>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 (>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 ) 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
Re: [Nouveau] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Wed, Mar 02, 2022 at 10:29:31AM +0100, Rasmus Villemoes wrote:
> This won't help the current issue (because it doesn't exist and might
> never), but just in case some compiler people are listening, I'd like to
> have some sort of way to tell the compiler "treat this variable as
> uninitialized from here on". So one could do
>
> #define kfree(p) do { __kfree(p); __magic_uninit(p); } while (0)
>
I think this is a good idea.
Smatch can already find all the iterator used outside the loop bugs that
Jakob did with a manageably small number of false positives. The
problems are that:
1) It would be better to find it in the compile stage instead of later.
2) I hadn't published that check. Will do shortly.
3) A couple weeks back I noticed that the list_for_each_entry() check
was no longer working. Fixed now.
4) Smatch was only looking at cases which dereferenced the iterator and
not checks for NULL. I will test the fix for that tonight.
5) Smatch is broken on PowerPC.
Coccinelle also has checks for iterator used outside the loop.
Coccinelle had these checks before Smatch did. I copied Julia's idea.
If your annotation was added to GCC it would solve all those problems.
But it's kind of awkward that we can't annotate kfree() directly
instead of creating the kfree() macro. And there are lots of other
functions which free things so you'd have to create a ton of macros
like:
#define gr_free_dma_desc(a, b) do { __gr_free_dma_desc(a, b);
__magic_uninit(b); } while (0)
And then there are functions which free a struct member:
void free_bar(struct foo *p) { kfree(p->bar); }
Or functions which free a container_of().
Smatch is more evolved than designed but what I do these days is use $0,
$1, $2 to represent the parameters. So you can say a function frees
$0->bar. For container_of() then is "(168<~$0)->bar" which means 168
bytes from $0. Returns are parameter -1 so I guess it would be $(-1),
but as I said Smatch evolved so right now places that talk about
returned values use a different format.
What you could do is just make a parseable table next to the function
definition with all the information. Then you would use a Perl script
to automatically generate a Coccinelle check to warn about use after
frees.
diff --git a/mm/slab.c b/mm/slab.c
index ddf5737c63d9..c9dffa5c40a2 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -3771,6 +3771,9 @@ EXPORT_SYMBOL(kmem_cache_free_bulk);
*
* Don't free memory not originally allocated by kmalloc()
* or you will run into trouble.
+ *
+ * CHECKER information
+ * frees: $0
*/
void kfree(const void *objp)
{
regards,
dan carpenter
