This is the first patch removing several categories of use cases of
the list iterator variable past the loop.
This is follow up to the discussion in:
https://lore.kernel.org/all/[email protected]/
As concluded in:
https://lore.kernel.org/all/[email protected]/
the correct use should be using a separate variable after the loop
and using a 'tmp' variable as the list iterator.
The list iterator will not point to a valid structure after the loop
if no break/goto was hit. Invalid uses of the list iterator variable
can be avoided altogether by simply using a separate pointer to
iterate the list.
Linus and Greg agreed on the following pattern:
- struct gr_request *req;
+ struct gr_request *req = NULL;
+ struct gr_request *tmp;
struct gr_ep *ep;
int ret = 0;
- list_for_each_entry(req, &ep->queue, queue) {
- if (&req->req == _req)
+ list_for_each_entry(tmp, &ep->queue, queue) {
+ if (&tmp->req == _req) {
+ req = tmp;
break;
+ }
}
- if (&req->req != _req) {
+ if (!req) {
ret = -EINVAL;
goto out;
}
With gnu89 the list iterator variable cannot yet be declared
within the for loop of the list iterator.
Moving to a more modern version of C would allow defining
the list iterator variable within the macro, limiting
the scope to the loop.
This avoids any incorrect usage past the loop altogether.
This are around 30% of the cases where the iterator
variable is used past the loop (identified with a slightly
modified version of use_after_iter.cocci).
I've decided to split it into at a few patches separated
by similar use cases.
Because the output of get_maintainer.pl was too big,
I included all the found lists and everyone from the
previous discussion.
Jakob Koschel (6):
drivers: usb: remove usage of list iterator past the loop body
treewide: remove using list iterator after loop body as a ptr
treewide: fix incorrect use to determine if list is empty
drivers: remove unnecessary use of list iterator variable
treewide: remove dereference of list iterator after loop body
treewide: remove check of list iterator against head past the loop
body
arch/arm/mach-mmp/sram.c | 9 ++--
arch/arm/plat-pxa/ssp.c | 28 +++++-------
arch/powerpc/sysdev/fsl_gtm.c | 4 +-
arch/x86/kernel/cpu/sgx/encl.c | 6 ++-
drivers/block/drbd/drbd_req.c | 45 ++++++++++++-------
drivers/counter/counter-chrdev.c | 26 ++++++-----
drivers/crypto/cavium/nitrox/nitrox_main.c | 11 +++--
drivers/dma/dw-edma/dw-edma-core.c | 4 +-
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/clk/base.c | 11 +++--
.../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/pci/saa7134/saa7134-alsa.c | 4 +-
drivers/media/v4l2-core/v4l2-ctrls-api.c | 31 +++++++------
drivers/misc/mei/interrupt.c | 12 ++---
.../net/ethernet/intel/i40e/i40e_ethtool.c | 3 +-
.../net/ethernet/qlogic/qede/qede_filter.c | 11 +++--
drivers/net/wireless/ath/ath6kl/htc_mbox.c | 2 +-
.../net/wireless/intel/ipw2x00/libipw_rx.c | 15 ++++---
drivers/perf/xgene_pmu.c | 13 +++---
drivers/power/supply/cpcap-battery.c | 11 +++--
drivers/scsi/lpfc/lpfc_bsg.c | 16 ++++---
drivers/scsi/scsi_transport_sas.c | 17 ++++---
drivers/scsi/wd719x.c | 12 +++--
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/thermal/thermal_core.c | 38 ++++++++++------
drivers/usb/gadget/composite.c | 9 ++--
drivers/usb/gadget/configfs.c | 22 +++++----
drivers/usb/gadget/udc/aspeed-vhub/epn.c | 11 +++--
drivers/usb/gadget/udc/at91_udc.c | 26 ++++++-----
drivers/usb/gadget/udc/atmel_usba_udc.c | 11 +++--
drivers/usb/gadget/udc/bdc/bdc_ep.c | 11 +++--
drivers/usb/gadget/udc/fsl_qe_udc.c | 11 +++--
drivers/usb/gadget/udc/fsl_udc_core.c | 11 +++--
drivers/usb/gadget/udc/goku_udc.c | 11 +++--
drivers/usb/gadget/udc/gr_udc.c | 11 +++--
drivers/usb/gadget/udc/lpc32xx_udc.c | 11 +++--
drivers/usb/gadget/udc/max3420_udc.c | 11 +++--
drivers/usb/gadget/udc/mv_u3d_core.c | 11 +++--
drivers/usb/gadget/udc/mv_udc_core.c | 11 +++--
drivers/usb/gadget/udc/net2272.c | 12 ++---
drivers/usb/gadget/udc/net2280.c | 11 +++--
drivers/usb/gadget/udc/omap_udc.c | 11 +++--
drivers/usb/gadget/udc/pxa25x_udc.c | 11 +++--
drivers/usb/gadget/udc/s3c-hsudc.c | 11 +++--
drivers/usb/gadget/udc/tegra-xudc.c | 11 +++--
drivers/usb/gadget/udc/udc-xilinx.c | 11 +++--
drivers/usb/mtu3/mtu3_gadget.c | 11 +++--
drivers/usb/musb/musb_gadget.c | 11 +++--
drivers/vfio/mdev/mdev_core.c | 11 +++--
fs/cifs/smb2misc.c | 10 +++--
fs/f2fs/segment.c | 9 ++--
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 +++---
83 files changed, 708 insertions(+), 465 deletions(-)
base-commit: 7ee022567bf9e2e0b3cd92461a2f4986ecc99673
--
2.25.1
The list iterator variable will be a bogus pointer if no break was hit.
Dereferencing it could load *any* out-of-bounds/undefined value
making it unsafe to use that in the comparision to determine if the
specific element was found.
This is fixed by using a separate list iterator variable for the loop
and only setting the original variable if a suitable element was found.
Then determing if the element was found is simply checking if the
variable is set.
Signed-off-by: Jakob Koschel <[email protected]>
---
drivers/gpu/drm/nouveau/nvkm/subdev/clk/base.c | 11 +++++++----
drivers/scsi/wd719x.c | 12 ++++++++----
fs/f2fs/segment.c | 9 ++++++---
3 files changed, 21 insertions(+), 11 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..c56cd9e59a66 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/clk/base.c
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/clk/base.c
@@ -471,20 +471,23 @@ nvkm_pstate_new(struct nvkm_clk *clk, int idx)
static int
nvkm_clk_ustate_update(struct nvkm_clk *clk, int req)
{
- struct nvkm_pstate *pstate;
+ struct nvkm_pstate *pstate = NULL;
+ struct nvkm_pstate *tmp;
int i = 0;
if (!clk->allow_reclock)
return -ENOSYS;
if (req != -1 && req != -2) {
- list_for_each_entry(pstate, &clk->states, head) {
- if (pstate->pstate == req)
+ list_for_each_entry(tmp, &clk->states, head) {
+ if (tmp->pstate == req) {
+ pstate = tmp;
break;
+ }
i++;
}
- if (pstate->pstate != req)
+ if (!pstate)
return -EINVAL;
req = i;
}
diff --git a/drivers/scsi/wd719x.c b/drivers/scsi/wd719x.c
index 1a7947554581..be270ed8e00d 100644
--- a/drivers/scsi/wd719x.c
+++ b/drivers/scsi/wd719x.c
@@ -684,11 +684,15 @@ static irqreturn_t wd719x_interrupt(int irq, void *dev_id)
case WD719X_INT_SPIDERFAILED:
/* was the cmd completed a direct or SCB command? */
if (regs.bytes.OPC == WD719X_CMD_PROCESS_SCB) {
- struct wd719x_scb *scb;
- list_for_each_entry(scb, &wd->active_scbs, list)
- if (SCB_out == scb->phys)
+ struct wd719x_scb *scb = NULL;
+ struct wd719x_scb *tmp;
+
+ list_for_each_entry(tmp, &wd->active_scbs, list)
+ if (SCB_out == tmp->phys) {
+ scb = tmp;
break;
- if (SCB_out == scb->phys)
+ }
+ if (scb)
wd719x_interrupt_SCB(wd, regs, scb);
else
dev_err(&wd->pdev->dev, "card returned invalid SCB pointer\n");
diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index 1dabc8244083..a3684385e04a 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -356,16 +356,19 @@ void f2fs_drop_inmem_page(struct inode *inode, struct page *page)
struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
struct list_head *head = &fi->inmem_pages;
struct inmem_pages *cur = NULL;
+ struct inmem_pages *tmp;
f2fs_bug_on(sbi, !page_private_atomic(page));
mutex_lock(&fi->inmem_lock);
- list_for_each_entry(cur, head, list) {
- if (cur->page == page)
+ list_for_each_entry(tmp, head, list) {
+ if (tmp->page == page) {
+ cur = tmp;
break;
+ }
}
- f2fs_bug_on(sbi, list_empty(head) || cur->page != page);
+ f2fs_bug_on(sbi, !cur);
list_del(&cur->list);
mutex_unlock(&fi->inmem_lock);
--
2.25.1
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