Re: [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: [PATCH 6/6] treewide: remove check of list iterator against head past the loop body
So looking at this patch, I really reacted to the fact that quite
often the "use outside the loop" case is all kinds of just plain
unnecessary, but _used_ to be a convenience feature.
I'll just quote the first chunk in it's entirely as an example - not
because I think this chunk is particularly important, but because it's
a good example:
On Mon, Feb 28, 2022 at 3:09 AM Jakob Koschel wrote:
>
> diff --git a/arch/arm/mach-mmp/sram.c b/arch/arm/mach-mmp/sram.c
> index 6794e2db1ad5..fc47c107059b 100644
> --- a/arch/arm/mach-mmp/sram.c
> +++ b/arch/arm/mach-mmp/sram.c
> @@ -39,19 +39,22 @@ static LIST_HEAD(sram_bank_list);
> struct gen_pool *sram_get_gpool(char *pool_name)
> {
> struct sram_bank_info *info = NULL;
> + struct sram_bank_info *tmp;
>
> if (!pool_name)
> return NULL;
>
> mutex_lock(_lock);
>
> - list_for_each_entry(info, _bank_list, node)
> - if (!strcmp(pool_name, info->pool_name))
> + list_for_each_entry(tmp, _bank_list, node)
> + if (!strcmp(pool_name, tmp->pool_name)) {
> + info = tmp;
> break;
> + }
>
> mutex_unlock(_lock);
>
> - if (>node == _bank_list)
> + if (!info)
> return NULL;
>
> return info->gpool;
I realize this was probably at least auto-generated with coccinelle,
but maybe that script could be taught to do avoid the "use after loop"
by simply moving the code _into_ the loop.
IOW, this all would be cleaner and clear written as
if (!pool_name)
return NULL;
mutex_lock(_lock);
list_for_each_entry(info, _bank_list, node) {
if (!strcmp(pool_name, info->pool_name)) {
mutex_unlock(_lock);
return info;
}
}
mutex_unlock(_lock);
return NULL;
Ta-daa - no use outside the loop, no need for new variables, just a
simple "just do it inside the loop". Yes, we end up having that lock
thing twice, but it looks worth it from a "make the code obvious"
standpoint.
Would it be even cleaner if the locking was done in the caller, and
the loop was some simple helper function? It probably would. But that
would require a bit more smarts than probably a simple coccinelle
script would do.
Linus
Re: [PATCH 6/6] treewide: remove check of list iterator against head past the loop body
On Mon, Feb 28, 2022 at 12:08:22PM +0100, Jakob Koschel wrote:
> diff --git a/drivers/infiniband/hw/hfi1/tid_rdma.c
> b/drivers/infiniband/hw/hfi1/tid_rdma.c
> index 2a7abf7a1f7f..a069847b56aa 100644
> --- a/drivers/infiniband/hw/hfi1/tid_rdma.c
> +++ b/drivers/infiniband/hw/hfi1/tid_rdma.c
> @@ -1239,7 +1239,7 @@ static int kern_alloc_tids(struct tid_rdma_flow *flow)
> struct hfi1_ctxtdata *rcd = flow->req->rcd;
> struct hfi1_devdata *dd = rcd->dd;
> u32 ngroups, pageidx = 0;
> - struct tid_group *group = NULL, *used;
> + struct tid_group *group = NULL, *used, *tmp;
> u8 use;
>
> flow->tnode_cnt = 0;
> @@ -1248,13 +1248,15 @@ static int kern_alloc_tids(struct tid_rdma_flow *flow)
> goto used_list;
>
> /* First look at complete groups */
> - list_for_each_entry(group, >tid_group_list.list, list) {
> - kern_add_tid_node(flow, rcd, "complete groups", group,
> - group->size);
> + list_for_each_entry(tmp, >tid_group_list.list, list) {
> + kern_add_tid_node(flow, rcd, "complete groups", tmp,
> + tmp->size);
>
> - pageidx += group->size;
> - if (!--ngroups)
> + pageidx += tmp->size;
> + if (!--ngroups) {
> + group = tmp;
> break;
> + }
> }
>
> if (pageidx >= flow->npagesets)
> @@ -1277,7 +1279,7 @@ static int kern_alloc_tids(struct tid_rdma_flow *flow)
>* However, if we are at the head, we have reached the end of the
^
>* complete groups list from the first loop above
^^
>*/
Originally this code tested for an open code list_is_head() so the
comment made sense, but it's out of date now. Just delete it.
> - if (group && >list == >tid_group_list.list)
> + if (!group)
> goto bail_eagain;
> group = list_prepare_entry(group, >tid_group_list.list,
> list);
regards,
dan carpenter
[PATCH 6/6] treewide: remove check of list iterator against head past the loop body
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
---
arch/arm/mach-mmp/sram.c | 9 ++--
arch/arm/plat-pxa/ssp.c | 28 +---
drivers/block/drbd/drbd_req.c | 45 ---
drivers/counter/counter-chrdev.c | 26 ++-
drivers/crypto/cavium/nitrox/nitrox_main.c| 11 +++--
drivers/dma/ppc4xx/adma.c | 11 +++--
drivers/firewire/core-transaction.c | 32 +++--
drivers/firewire/sbp2.c | 14 +++---
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c| 19 +---
drivers/gpu/drm/drm_memory.c | 15 ---
drivers/gpu/drm/drm_mm.c | 17 ---
drivers/gpu/drm/drm_vm.c | 13 +++---
drivers/gpu/drm/gma500/oaktrail_lvds.c| 9 ++--
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 ---
.../gpu/drm/nouveau/nvkm/subdev/fb/ramgk104.c | 13 +++---
drivers/gpu/drm/scheduler/sched_main.c| 14 +++---
drivers/gpu/drm/ttm/ttm_bo.c | 19
drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 22 +
drivers/infiniband/hw/hfi1/tid_rdma.c | 16 ---
drivers/infiniband/hw/mlx4/main.c | 12 ++---
drivers/media/dvb-frontends/mxl5xx.c | 11 +++--
drivers/media/v4l2-core/v4l2-ctrls-api.c | 31 +++--
drivers/misc/mei/interrupt.c | 12 ++---
.../net/ethernet/qlogic/qede/qede_filter.c| 11 +++--
.../net/wireless/intel/ipw2x00/libipw_rx.c| 15 ---
drivers/power/supply/cpcap-battery.c | 11 +++--
drivers/scsi/lpfc/lpfc_bsg.c | 16 ---
drivers/staging/rtl8192e/rtl819x_TSProc.c | 17 +++
drivers/staging/rtl8192e/rtllib_rx.c | 17 ---
.../staging/rtl8192u/ieee80211/ieee80211_rx.c | 15 ---
.../rtl8192u/ieee80211/rtl819x_TSProc.c | 19
drivers/usb/gadget/composite.c| 9 ++--
fs/cifs/smb2misc.c| 10 +++--
fs/proc/kcore.c | 13 +++---
kernel/debug/kdb/kdb_main.c | 36 +--
kernel/power/snapshot.c | 10 +++--
kernel/trace/ftrace.c | 22 +
kernel/trace/trace_eprobe.c | 15 ---
kernel/trace/trace_events.c | 11 ++---
net/9p/trans_xen.c| 11 +++--
net/ipv4/udp_tunnel_nic.c | 10 +++--
net/tipc/name_table.c | 11 +++--
net/tipc/socket.c | 11 +++--
net/xfrm/xfrm_ipcomp.c| 11 +++--
sound/soc/intel/catpt/pcm.c | 13 +++---
sound/soc/sprd/sprd-mcdt.c| 13 +++---
48 files changed, 455 insertions(+), 315 deletions(-)
diff --git a/arch/arm/mach-mmp/sram.c b/arch/arm/mach-mmp/sram.c
index 6794e2db1ad5..fc47c107059b 100644
--- a/arch/arm/mach-mmp/sram.c
+++ b/arch/arm/mach-mmp/sram.c
@@ -39,19 +39,22 @@ static LIST_HEAD(sram_bank_list);
struct gen_pool *sram_get_gpool(char *pool_name)
{
struct sram_bank_info *info = NULL;
+ struct sram_bank_info *tmp;
if (!pool_name)
return NULL;
mutex_lock(_lock);
- list_for_each_entry(info, _bank_list, node)
- if (!strcmp(pool_name, info->pool_name))
+ list_for_each_entry(tmp, _bank_list, node)
+ if (!strcmp(pool_name, tmp->pool_name)) {
+ info = tmp;
break;
+ }
mutex_unlock(_lock);
- if (>node == _bank_list)
+ if (!info)
return NULL;
return info->gpool;
diff --git a/arch/arm/plat-pxa/ssp.c b/arch/arm/plat-pxa/ssp.c
index 563440315acd..4884a8c0c89b 100644
--- a/arch/arm/plat-pxa/ssp.c
+++ b/arch/arm/plat-pxa/ssp.c
@@ -38,22 +38,20 @@ static LIST_HEAD(ssp_list);
struct ssp_device *pxa_ssp_request(int port, const char *label)
{
struct ssp_device *ssp = NULL;
+ struct ssp_device *tmp;
mutex_lock(_lock);
- list_for_each_entry(ssp, _list, node) {
- if (ssp->port_id == port && ssp->use_count == 0) {
- ssp->use_count++;
- ssp->label = label;
+ list_for_each_entry(tmp,
Re: [PATCH 6/6] treewide: remove check of list iterator against head past the loop body
This is a bit more work (and a lot more noise), but I'd prefer if this were split into as many patches as there are components. I'm not going to review the parts of the patches that don't concern me, and if something turns out to be a problem later one (it shouldn't but one never knows) it'll be much easier to revert or put the blame on an individual smaller commit than on this... With that being said, ultimately I don't care that much and will leave that to people who do :) Jakob Koschel wrote on Mon, Feb 28, 2022 at 12:08:22PM +0100: > net/9p/trans_xen.c| 11 +++-- This 9p change looks good to me. Reviewed-by: Dominique Martinet # 9p -- Dominique
