[PATCH 0/6] Remove usage of list iterator past the loop body

STALE1608d

85 messages, 22 authors, 2022-03-07 · open the first message on its own page

[PATCH 0/6] Remove usage of list iterator past the loop body

From: Jakob Koschel <hidden>
Date: 2022-02-28 11:09:04

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/20220217184829.1991035-1-jakobkoschel@gmail.com/

As concluded in:
https://lore.kernel.org/all/YhdfEIwI4EdtHdym@kroah.com/
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

[PATCH 1/6] drivers: usb: remove usage of list iterator past the loop body

From: Jakob Koschel <hidden>
Date: 2022-02-28 11:09:05

If the list representing the request queue does not contain the expected
request, the value of list_for_each_entry() iterator will not point to a
valid structure. To avoid type confusion in such case, the list iterator
scope will be limited to list_for_each_entry() loop.

In preparation to limiting scope of a list iterator to the list traversal
loop, use a dedicated pointer to point to the found request object.

Signed-off-by: Jakob Koschel <redacted>
---
 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/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/udc-xilinx.c      | 11 ++++++----
 17 files changed, 128 insertions(+), 75 deletions(-)
diff --git a/drivers/usb/gadget/udc/aspeed-vhub/epn.c b/drivers/usb/gadget/udc/aspeed-vhub/epn.c
index 917892ca8753..cad874ee4472 100644
--- a/drivers/usb/gadget/udc/aspeed-vhub/epn.c
+++ b/drivers/usb/gadget/udc/aspeed-vhub/epn.c
@@ -466,19 +466,22 @@ static int ast_vhub_epn_dequeue(struct usb_ep* u_ep, struct usb_request *u_req)
 {
 	struct ast_vhub_ep *ep = to_ast_ep(u_ep);
 	struct ast_vhub *vhub = ep->vhub;
-	struct ast_vhub_req *req;
+	struct ast_vhub_req *req = NULL;
+	struct ast_vhub_req *tmp;
 	unsigned long flags;
 	int rc = -EINVAL;

 	spin_lock_irqsave(&vhub->lock, flags);

 	/* Make sure it's actually queued on this endpoint */
-	list_for_each_entry (req, &ep->queue, queue) {
-		if (&req->req == u_req)
+	list_for_each_entry(tmp, &ep->queue, queue) {
+		if (&tmp->req == u_req) {
+			req = tmp;
 			break;
+		}
 	}

-	if (&req->req == u_req) {
+	if (req) {
 		EPVDBG(ep, "dequeue req @%p active=%d\n",
 		       req, req->active);
 		if (req->active)
diff --git a/drivers/usb/gadget/udc/at91_udc.c b/drivers/usb/gadget/udc/at91_udc.c
index 9040a0561466..0fd0307bc07b 100644
--- a/drivers/usb/gadget/udc/at91_udc.c
+++ b/drivers/usb/gadget/udc/at91_udc.c
@@ -150,13 +150,14 @@ static void proc_ep_show(struct seq_file *s, struct at91_ep *ep)
 	if (list_empty (&ep->queue))
 		seq_printf(s, "\t(queue empty)\n");

-	else list_for_each_entry (req, &ep->queue, queue) {
-		unsigned	length = req->req.actual;
+	else
+		list_for_each_entry(req, &ep->queue, queue) {
+			unsigned int	length = req->req.actual;

-		seq_printf(s, "\treq %p len %d/%d buf %p\n",
-				&req->req, length,
-				req->req.length, req->req.buf);
-	}
+			seq_printf(s, "\treq %p len %d/%d buf %p\n",
+					&req->req, length,
+					req->req.length, req->req.buf);
+		}
 	spin_unlock_irqrestore(&udc->lock, flags);
 }
@@ -226,7 +227,7 @@ static int proc_udc_show(struct seq_file *s, void *unused)

 	if (udc->enabled && udc->vbus) {
 		proc_ep_show(s, &udc->ep[0]);
-		list_for_each_entry (ep, &udc->gadget.ep_list, ep.ep_list) {
+		list_for_each_entry(ep, &udc->gadget.ep_list, ep.ep_list) {
 			if (ep->ep.desc)
 				proc_ep_show(s, ep);
 		}
@@ -704,7 +705,8 @@ static int at91_ep_queue(struct usb_ep *_ep,
 static int at91_ep_dequeue(struct usb_ep *_ep, struct usb_request *_req)
 {
 	struct at91_ep		*ep;
-	struct at91_request	*req;
+	struct at91_request	*req = NULL;
+	struct at91_request	*tmp;
 	unsigned long		flags;
 	struct at91_udc		*udc;
@@ -717,11 +719,13 @@ static int at91_ep_dequeue(struct usb_ep *_ep, struct usb_request *_req)
 	spin_lock_irqsave(&udc->lock, flags);

 	/* make sure it's actually queued on this endpoint */
-	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) {
 		spin_unlock_irqrestore(&udc->lock, flags);
 		return -EINVAL;
 	}
diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c b/drivers/usb/gadget/udc/atmel_usba_udc.c
index 2b893bceea45..8e393e14f137 100644
--- a/drivers/usb/gadget/udc/atmel_usba_udc.c
+++ b/drivers/usb/gadget/udc/atmel_usba_udc.c
@@ -860,7 +860,8 @@ static int usba_ep_dequeue(struct usb_ep *_ep, struct usb_request *_req)
 {
 	struct usba_ep *ep = to_usba_ep(_ep);
 	struct usba_udc *udc = ep->udc;
-	struct usba_request *req;
+	struct usba_request *req = NULL;
+	struct usba_request *tmp;
 	unsigned long flags;
 	u32 status;
@@ -869,12 +870,14 @@ static int usba_ep_dequeue(struct usb_ep *_ep, struct usb_request *_req)

 	spin_lock_irqsave(&udc->lock, flags);

-	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) {
 		spin_unlock_irqrestore(&udc->lock, flags);
 		return -EINVAL;
 	}
diff --git a/drivers/usb/gadget/udc/bdc/bdc_ep.c b/drivers/usb/gadget/udc/bdc/bdc_ep.c
index 8e2f20b12519..829e96791d0a 100644
--- a/drivers/usb/gadget/udc/bdc/bdc_ep.c
+++ b/drivers/usb/gadget/udc/bdc/bdc_ep.c
@@ -1757,6 +1757,7 @@ static int bdc_gadget_ep_dequeue(struct usb_ep *_ep,
 				  struct usb_request *_req)
 {
 	struct bdc_req *req;
+	struct bdc_req *tmp;
 	unsigned long flags;
 	struct bdc_ep *ep;
 	struct bdc *bdc;
@@ -1771,12 +1772,16 @@ static int bdc_gadget_ep_dequeue(struct usb_ep *_ep,
 	dev_dbg(bdc->dev, "%s ep:%s req:%p\n", __func__, ep->name, req);
 	bdc_dbg_bd_list(bdc, ep);
 	spin_lock_irqsave(&bdc->lock, flags);
+
+	req = NULL;
 	/* make sure it's still queued on this endpoint */
-	list_for_each_entry(req, &ep->queue, queue) {
-		if (&req->usb_req == _req)
+	list_for_each_entry(tmp, &ep->queue, queue) {
+		if (&tmp->usb_req == _req) {
+			req = tmp;
 			break;
+		}
 	}
-	if (&req->usb_req != _req) {
+	if (!req) {
 		spin_unlock_irqrestore(&bdc->lock, flags);
 		dev_err(bdc->dev, "usb_req !=req n");
 		return -EINVAL;
diff --git a/drivers/usb/gadget/udc/fsl_qe_udc.c b/drivers/usb/gadget/udc/fsl_qe_udc.c
index 15db7a3868fe..3979a2825e3c 100644
--- a/drivers/usb/gadget/udc/fsl_qe_udc.c
+++ b/drivers/usb/gadget/udc/fsl_qe_udc.c
@@ -1776,7 +1776,8 @@ static int qe_ep_queue(struct usb_ep *_ep, struct usb_request *_req,
 static int qe_ep_dequeue(struct usb_ep *_ep, struct usb_request *_req)
 {
 	struct qe_ep *ep = container_of(_ep, struct qe_ep, ep);
-	struct qe_req *req;
+	struct qe_req *req = NULL;
+	struct qe_req *tmp;
 	unsigned long flags;

 	if (!_ep || !_req)
@@ -1785,12 +1786,14 @@ static int qe_ep_dequeue(struct usb_ep *_ep, struct usb_request *_req)
 	spin_lock_irqsave(&ep->udc->lock, flags);

 	/* make sure it's actually queued on this endpoint */
-	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) {
 		spin_unlock_irqrestore(&ep->udc->lock, flags);
 		return -EINVAL;
 	}
diff --git a/drivers/usb/gadget/udc/fsl_udc_core.c b/drivers/usb/gadget/udc/fsl_udc_core.c
index 29fcb9b461d7..23d670fae12c 100644
--- a/drivers/usb/gadget/udc/fsl_udc_core.c
+++ b/drivers/usb/gadget/udc/fsl_udc_core.c
@@ -918,7 +918,8 @@ fsl_ep_queue(struct usb_ep *_ep, struct usb_request *_req, gfp_t gfp_flags)
 static int fsl_ep_dequeue(struct usb_ep *_ep, struct usb_request *_req)
 {
 	struct fsl_ep *ep = container_of(_ep, struct fsl_ep, ep);
-	struct fsl_req *req;
+	struct fsl_req *req = NULL;
+	struct fsl_req *tmp;
 	unsigned long flags;
 	int ep_num, stopped, ret = 0;
 	u32 epctrl;
@@ -940,11 +941,13 @@ static int fsl_ep_dequeue(struct usb_ep *_ep, struct usb_request *_req)
 	fsl_writel(epctrl, &dr_regs->endptctrl[ep_num]);

 	/* make sure it's actually queued on this endpoint */
-	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;
 	}
diff --git a/drivers/usb/gadget/udc/goku_udc.c b/drivers/usb/gadget/udc/goku_udc.c
index 3757a772a55e..62f0b7d794ec 100644
--- a/drivers/usb/gadget/udc/goku_udc.c
+++ b/drivers/usb/gadget/udc/goku_udc.c
@@ -809,7 +809,8 @@ static void nuke(struct goku_ep *ep, int status)
 /* dequeue JUST ONE request */
 static int goku_dequeue(struct usb_ep *_ep, struct usb_request *_req)
 {
-	struct goku_request	*req;
+	struct goku_request	*req = NULL;
+	struct goku_request	*tmp;
 	struct goku_ep		*ep;
 	struct goku_udc		*dev;
 	unsigned long		flags;
@@ -833,11 +834,13 @@ static int goku_dequeue(struct usb_ep *_ep, struct usb_request *_req)
 	spin_lock_irqsave(&dev->lock, flags);

 	/* make sure it's actually queued on this endpoint */
-	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) {
 		spin_unlock_irqrestore (&dev->lock, flags);
 		return -EINVAL;
 	}
diff --git a/drivers/usb/gadget/udc/gr_udc.c b/drivers/usb/gadget/udc/gr_udc.c
index 4b35739d3695..5d65d8ad5281 100644
--- a/drivers/usb/gadget/udc/gr_udc.c
+++ b/drivers/usb/gadget/udc/gr_udc.c
@@ -1690,7 +1690,8 @@ static int gr_queue_ext(struct usb_ep *_ep, struct usb_request *_req,
 /* Dequeue JUST ONE request */
 static int gr_dequeue(struct usb_ep *_ep, struct usb_request *_req)
 {
-	struct gr_request *req;
+	struct gr_request *req = NULL;
+	struct gr_request *tmp;
 	struct gr_ep *ep;
 	struct gr_udc *dev;
 	int ret = 0;
@@ -1710,11 +1711,13 @@ static int gr_dequeue(struct usb_ep *_ep, struct usb_request *_req)
 	spin_lock_irqsave(&dev->lock, flags);

 	/* Make sure it's actually queued on this endpoint */
-	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;
 	}
diff --git a/drivers/usb/gadget/udc/lpc32xx_udc.c b/drivers/usb/gadget/udc/lpc32xx_udc.c
index a25d01c89564..024b646638fb 100644
--- a/drivers/usb/gadget/udc/lpc32xx_udc.c
+++ b/drivers/usb/gadget/udc/lpc32xx_udc.c
@@ -1830,7 +1830,8 @@ static int lpc32xx_ep_queue(struct usb_ep *_ep,
 static int lpc32xx_ep_dequeue(struct usb_ep *_ep, struct usb_request *_req)
 {
 	struct lpc32xx_ep *ep;
-	struct lpc32xx_request *req;
+	struct lpc32xx_request *req = NULL;
+	struct lpc32xx_request *tmp;
 	unsigned long flags;

 	ep = container_of(_ep, struct lpc32xx_ep, ep);
@@ -1840,11 +1841,13 @@ static int lpc32xx_ep_dequeue(struct usb_ep *_ep, struct usb_request *_req)
 	spin_lock_irqsave(&ep->udc->lock, flags);

 	/* make sure it's actually queued on this endpoint */
-	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) {
 		spin_unlock_irqrestore(&ep->udc->lock, flags);
 		return -EINVAL;
 	}
diff --git a/drivers/usb/gadget/udc/mv_u3d_core.c b/drivers/usb/gadget/udc/mv_u3d_core.c
index a1057ddfbda3..39bd0aeb58d1 100644
--- a/drivers/usb/gadget/udc/mv_u3d_core.c
+++ b/drivers/usb/gadget/udc/mv_u3d_core.c
@@ -844,7 +844,8 @@ mv_u3d_ep_queue(struct usb_ep *_ep, struct usb_request *_req, gfp_t gfp_flags)
 static int mv_u3d_ep_dequeue(struct usb_ep *_ep, struct usb_request *_req)
 {
 	struct mv_u3d_ep *ep;
-	struct mv_u3d_req *req;
+	struct mv_u3d_req *req = NULL;
+	struct mv_u3d_req *tmp;
 	struct mv_u3d *u3d;
 	struct mv_u3d_ep_context *ep_context;
 	struct mv_u3d_req *next_req;
@@ -861,11 +862,13 @@ static int mv_u3d_ep_dequeue(struct usb_ep *_ep, struct usb_request *_req)
 	spin_lock_irqsave(&ep->u3d->lock, flags);

 	/* make sure it's actually queued on this endpoint */
-	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;
 	}
diff --git a/drivers/usb/gadget/udc/mv_udc_core.c b/drivers/usb/gadget/udc/mv_udc_core.c
index b6d34dda028b..9d708ce49c50 100644
--- a/drivers/usb/gadget/udc/mv_udc_core.c
+++ b/drivers/usb/gadget/udc/mv_udc_core.c
@@ -771,7 +771,8 @@ static void mv_prime_ep(struct mv_ep *ep, struct mv_req *req)
 static int mv_ep_dequeue(struct usb_ep *_ep, struct usb_request *_req)
 {
 	struct mv_ep *ep = container_of(_ep, struct mv_ep, ep);
-	struct mv_req *req;
+	struct mv_req *req = NULL;
+	struct mv_req *tmp;
 	struct mv_udc *udc = ep->udc;
 	unsigned long flags;
 	int stopped, ret = 0;
@@ -793,11 +794,13 @@ static int mv_ep_dequeue(struct usb_ep *_ep, struct usb_request *_req)
 	writel(epctrlx, &udc->op_regs->epctrlx[ep->ep_num]);

 	/* make sure it's actually queued on this endpoint */
-	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;
 	}
diff --git a/drivers/usb/gadget/udc/net2272.c b/drivers/usb/gadget/udc/net2272.c
index 7c38057dcb4a..bb59200f1596 100644
--- a/drivers/usb/gadget/udc/net2272.c
+++ b/drivers/usb/gadget/udc/net2272.c
@@ -926,7 +926,8 @@ static int
 net2272_dequeue(struct usb_ep *_ep, struct usb_request *_req)
 {
 	struct net2272_ep *ep;
-	struct net2272_request *req;
+	struct net2272_request *req = NULL;
+	struct net2272_request *tmp;
 	unsigned long flags;
 	int stopped;
@@ -939,11 +940,13 @@ net2272_dequeue(struct usb_ep *_ep, struct usb_request *_req)
 	ep->stopped = 1;

 	/* make sure it's still queued on this endpoint */
-	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) {
 		ep->stopped = stopped;
 		spin_unlock_irqrestore(&ep->dev->lock, flags);
 		return -EINVAL;
@@ -954,7 +957,6 @@ net2272_dequeue(struct usb_ep *_ep, struct usb_request *_req)
 		dev_dbg(ep->dev->dev, "unlink (%s) pio\n", _ep->name);
 		net2272_done(ep, req, -ECONNRESET);
 	}
-	req = NULL;
 	ep->stopped = stopped;

 	spin_unlock_irqrestore(&ep->dev->lock, flags);
diff --git a/drivers/usb/gadget/udc/net2280.c b/drivers/usb/gadget/udc/net2280.c
index 16e7d2db6411..dbf5592dbcf0 100644
--- a/drivers/usb/gadget/udc/net2280.c
+++ b/drivers/usb/gadget/udc/net2280.c
@@ -1240,7 +1240,8 @@ static void nuke(struct net2280_ep *ep)
 static int net2280_dequeue(struct usb_ep *_ep, struct usb_request *_req)
 {
 	struct net2280_ep	*ep;
-	struct net2280_request	*req;
+	struct net2280_request	*req = NULL;
+	struct net2280_request	*tmp;
 	unsigned long		flags;
 	u32			dmactl;
 	int			stopped;
@@ -1266,11 +1267,13 @@ static int net2280_dequeue(struct usb_ep *_ep, struct usb_request *_req)
 	}

 	/* make sure it's still queued on this endpoint */
-	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) {
 		ep->stopped = stopped;
 		spin_unlock_irqrestore(&ep->dev->lock, flags);
 		ep_dbg(ep->dev, "%s: Request mismatch\n", __func__);
diff --git a/drivers/usb/gadget/udc/omap_udc.c b/drivers/usb/gadget/udc/omap_udc.c
index 494da00398d7..c0f6e066ccc2 100644
--- a/drivers/usb/gadget/udc/omap_udc.c
+++ b/drivers/usb/gadget/udc/omap_udc.c
@@ -1003,7 +1003,8 @@ omap_ep_queue(struct usb_ep *_ep, struct usb_request *_req, gfp_t gfp_flags)
 static int omap_ep_dequeue(struct usb_ep *_ep, struct usb_request *_req)
 {
 	struct omap_ep	*ep = container_of(_ep, struct omap_ep, ep);
-	struct omap_req	*req;
+	struct omap_req	*req = NULL;
+	struct omap_req	*tmp;
 	unsigned long	flags;

 	if (!_ep || !_req)
@@ -1012,11 +1013,13 @@ static int omap_ep_dequeue(struct usb_ep *_ep, struct usb_request *_req)
 	spin_lock_irqsave(&ep->udc->lock, flags);

 	/* make sure it's actually queued on this endpoint */
-	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) {
 		spin_unlock_irqrestore(&ep->udc->lock, flags);
 		return -EINVAL;
 	}
diff --git a/drivers/usb/gadget/udc/pxa25x_udc.c b/drivers/usb/gadget/udc/pxa25x_udc.c
index b38747fd3bb0..889ea52bbe0a 100644
--- a/drivers/usb/gadget/udc/pxa25x_udc.c
+++ b/drivers/usb/gadget/udc/pxa25x_udc.c
@@ -966,7 +966,8 @@ static void nuke(struct pxa25x_ep *ep, int status)
 static int pxa25x_ep_dequeue(struct usb_ep *_ep, struct usb_request *_req)
 {
 	struct pxa25x_ep	*ep;
-	struct pxa25x_request	*req;
+	struct pxa25x_request	*req = NULL;
+	struct pxa25x_request	*tmp;
 	unsigned long		flags;

 	ep = container_of(_ep, struct pxa25x_ep, ep);
@@ -976,11 +977,13 @@ static int pxa25x_ep_dequeue(struct usb_ep *_ep, struct usb_request *_req)
 	local_irq_save(flags);

 	/* make sure it's actually queued on this endpoint */
-	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) {
 		local_irq_restore(flags);
 		return -EINVAL;
 	}
diff --git a/drivers/usb/gadget/udc/s3c-hsudc.c b/drivers/usb/gadget/udc/s3c-hsudc.c
index 89f1f8c9f02e..5006c9ebbac6 100644
--- a/drivers/usb/gadget/udc/s3c-hsudc.c
+++ b/drivers/usb/gadget/udc/s3c-hsudc.c
@@ -877,7 +877,8 @@ static int s3c_hsudc_dequeue(struct usb_ep *_ep, struct usb_request *_req)
 {
 	struct s3c_hsudc_ep *hsep = our_ep(_ep);
 	struct s3c_hsudc *hsudc = hsep->dev;
-	struct s3c_hsudc_req *hsreq;
+	struct s3c_hsudc_req *hsreq = NULL;
+	struct s3c_hsudc_req *tmp;
 	unsigned long flags;

 	hsep = our_ep(_ep);
@@ -886,11 +887,13 @@ static int s3c_hsudc_dequeue(struct usb_ep *_ep, struct usb_request *_req)

 	spin_lock_irqsave(&hsudc->lock, flags);

-	list_for_each_entry(hsreq, &hsep->queue, queue) {
-		if (&hsreq->req == _req)
+	list_for_each_entry(tmp, &hsep->queue, queue) {
+		if (&tmp->req == _req) {
+			hsreq = tmp;
 			break;
+		}
 	}
-	if (&hsreq->req != _req) {
+	if (!hsreq) {
 		spin_unlock_irqrestore(&hsudc->lock, flags);
 		return -EINVAL;
 	}
diff --git a/drivers/usb/gadget/udc/udc-xilinx.c b/drivers/usb/gadget/udc/udc-xilinx.c
index 6ce886fb7bfe..6812824cc823 100644
--- a/drivers/usb/gadget/udc/udc-xilinx.c
+++ b/drivers/usb/gadget/udc/udc-xilinx.c
@@ -1136,17 +1136,20 @@ static int xudc_ep_queue(struct usb_ep *_ep, struct usb_request *_req,
 static int xudc_ep_dequeue(struct usb_ep *_ep, struct usb_request *_req)
 {
 	struct xusb_ep *ep	= to_xusb_ep(_ep);
-	struct xusb_req *req	= to_xusb_req(_req);
+	struct xusb_req *req	= NULL;
+	struct xusb_req *tmp;
 	struct xusb_udc *udc	= ep->udc;
 	unsigned long flags;

 	spin_lock_irqsave(&udc->lock, flags);
 	/* Make sure it's actually queued on this endpoint */
-	list_for_each_entry(req, &ep->queue, queue) {
-		if (&req->usb_req == _req)
+	list_for_each_entry(tmp, &ep->queue, queue) {
+		if (&tmp->usb_req == _req) {
+			req = tmp;
 			break;
+		}
 	}
-	if (&req->usb_req != _req) {
+	if (!req) {
 		spin_unlock_irqrestore(&udc->lock, flags);
 		return -EINVAL;
 	}
--
2.25.1

Re: [PATCH 1/6] drivers: usb: remove usage of list iterator past the loop body

From: Dan Carpenter <hidden>
Date: 2022-02-28 11:28:13

On Mon, Feb 28, 2022 at 12:08:17PM +0100, Jakob Koschel wrote:
quoted hunk
diff --git a/drivers/usb/gadget/udc/at91_udc.c b/drivers/usb/gadget/udc/at91_udc.c
index 9040a0561466..0fd0307bc07b 100644
--- a/drivers/usb/gadget/udc/at91_udc.c
+++ b/drivers/usb/gadget/udc/at91_udc.c
@@ -150,13 +150,14 @@ static void proc_ep_show(struct seq_file *s, struct at91_ep *ep)
 	if (list_empty (&ep->queue))
 		seq_printf(s, "\t(queue empty)\n");

-	else list_for_each_entry (req, &ep->queue, queue) {
-		unsigned	length = req->req.actual;
+	else
+		list_for_each_entry(req, &ep->queue, queue) {
+			unsigned int	length = req->req.actual;

-		seq_printf(s, "\treq %p len %d/%d buf %p\n",
-				&req->req, length,
-				req->req.length, req->req.buf);
-	}
+			seq_printf(s, "\treq %p len %d/%d buf %p\n",
+					&req->req, length,
+					req->req.length, req->req.buf);
+		}
Don't make unrelated white space changes.  It just makes the patch
harder to review.  As you're writing the patch make note of any
additional changes and do them later in a separate patch.

Also a multi-line indent gets curly braces for readability even though
it's not required by C.  And then both sides would get curly braces.
quoted hunk
 	spin_unlock_irqrestore(&udc->lock, flags);
 }
@@ -226,7 +227,7 @@ static int proc_udc_show(struct seq_file *s, void *unused)

 	if (udc->enabled && udc->vbus) {
 		proc_ep_show(s, &udc->ep[0]);
-		list_for_each_entry (ep, &udc->gadget.ep_list, ep.ep_list) {
+		list_for_each_entry(ep, &udc->gadget.ep_list, ep.ep_list) {
Another unrelated change.
 			if (ep->ep.desc)
 				proc_ep_show(s, ep);
 		}

[ snip ]
quoted hunk
diff --git a/drivers/usb/gadget/udc/net2272.c b/drivers/usb/gadget/udc/net2272.c
index 7c38057dcb4a..bb59200f1596 100644
--- a/drivers/usb/gadget/udc/net2272.c
+++ b/drivers/usb/gadget/udc/net2272.c
@@ -926,7 +926,8 @@ static int
 net2272_dequeue(struct usb_ep *_ep, struct usb_request *_req)
 {
 	struct net2272_ep *ep;
-	struct net2272_request *req;
+	struct net2272_request *req = NULL;
+	struct net2272_request *tmp;
 	unsigned long flags;
 	int stopped;
@@ -939,11 +940,13 @@ net2272_dequeue(struct usb_ep *_ep, struct usb_request *_req)
 	ep->stopped = 1;

 	/* make sure it's still queued on this endpoint */
-	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) {
 		ep->stopped = stopped;
 		spin_unlock_irqrestore(&ep->dev->lock, flags);
 		return -EINVAL;
@@ -954,7 +957,6 @@ net2272_dequeue(struct usb_ep *_ep, struct usb_request *_req)
 		dev_dbg(ep->dev->dev, "unlink (%s) pio\n", _ep->name);
 		net2272_done(ep, req, -ECONNRESET);
 	}
-	req = NULL;
Another unrelated change.  These are all good changes but send them as
separate patches.
 	ep->stopped = stopped;

 	spin_unlock_irqrestore(&ep->dev->lock, flags);
regards,
dan carpenter

Re: [PATCH 1/6] drivers: usb: remove usage of list iterator past the loop body

From: Jakob Koschel <hidden>
Date: 2022-02-28 12:03:48

On 28. Feb 2022, at 12:24, Dan Carpenter [off-list ref] wrote:

On Mon, Feb 28, 2022 at 12:08:17PM +0100, Jakob Koschel wrote:
quoted
diff --git a/drivers/usb/gadget/udc/at91_udc.c b/drivers/usb/gadget/udc/at91_udc.c
index 9040a0561466..0fd0307bc07b 100644
--- a/drivers/usb/gadget/udc/at91_udc.c
+++ b/drivers/usb/gadget/udc/at91_udc.c
@@ -150,13 +150,14 @@ static void proc_ep_show(struct seq_file *s, struct at91_ep *ep)
	if (list_empty (&ep->queue))
		seq_printf(s, "\t(queue empty)\n");

-	else list_for_each_entry (req, &ep->queue, queue) {
-		unsigned	length = req->req.actual;
+	else
+		list_for_each_entry(req, &ep->queue, queue) {
+			unsigned int	length = req->req.actual;

-		seq_printf(s, "\treq %p len %d/%d buf %p\n",
-				&req->req, length,
-				req->req.length, req->req.buf);
-	}
+			seq_printf(s, "\treq %p len %d/%d buf %p\n",
+					&req->req, length,
+					req->req.length, req->req.buf);
+		}
Don't make unrelated white space changes.  It just makes the patch
harder to review.  As you're writing the patch make note of any
additional changes and do them later in a separate patch.

Also a multi-line indent gets curly braces for readability even though
it's not required by C.  And then both sides would get curly braces.
quoted
	spin_unlock_irqrestore(&udc->lock, flags);
}
@@ -226,7 +227,7 @@ static int proc_udc_show(struct seq_file *s, void *unused)
	if (udc->enabled && udc->vbus) {
		proc_ep_show(s, &udc->ep[0]);
-		list_for_each_entry (ep, &udc->gadget.ep_list, ep.ep_list) {
+		list_for_each_entry(ep, &udc->gadget.ep_list, ep.ep_list) {
Another unrelated change.
quoted
			if (ep->ep.desc)
				proc_ep_show(s, ep);
		}

[ snip ]
Thanks for pointing out, I'll remove the changes here and note them down
to send them separately.
quoted
diff --git a/drivers/usb/gadget/udc/net2272.c b/drivers/usb/gadget/udc/net2272.c
index 7c38057dcb4a..bb59200f1596 100644
--- a/drivers/usb/gadget/udc/net2272.c
+++ b/drivers/usb/gadget/udc/net2272.c
@@ -926,7 +926,8 @@ static int
net2272_dequeue(struct usb_ep *_ep, struct usb_request *_req)
{
	struct net2272_ep *ep;
-	struct net2272_request *req;
+	struct net2272_request *req = NULL;
+	struct net2272_request *tmp;
	unsigned long flags;
	int stopped;
@@ -939,11 +940,13 @@ net2272_dequeue(struct usb_ep *_ep, struct usb_request *_req)
	ep->stopped = 1;

	/* make sure it's still queued on this endpoint */
-	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) {
		ep->stopped = stopped;
		spin_unlock_irqrestore(&ep->dev->lock, flags);
		return -EINVAL;
@@ -954,7 +957,6 @@ net2272_dequeue(struct usb_ep *_ep, struct usb_request *_req)
		dev_dbg(ep->dev->dev, "unlink (%s) pio\n", _ep->name);
		net2272_done(ep, req, -ECONNRESET);
	}
-	req = NULL;
Another unrelated change.  These are all good changes but send them as
separate patches.
You are referring to the req = NULL, right?

I've changed the use of 'req' in the same function and assumed that I can
just remove the unnecessary statement. But if it's better to do separately
I'll do that.
quoted
	ep->stopped = stopped;

	spin_unlock_irqrestore(&ep->dev->lock, flags);
regards,
dan carpenter
thanks,
Jakob Koschel

Re: [PATCH 1/6] drivers: usb: remove usage of list iterator past the loop body

From: Dan Carpenter <hidden>
Date: 2022-02-28 13:20:39

On Mon, Feb 28, 2022 at 01:03:36PM +0100, Jakob Koschel wrote:
quoted
quoted
@@ -954,7 +957,6 @@ net2272_dequeue(struct usb_ep *_ep, struct usb_request *_req)
		dev_dbg(ep->dev->dev, "unlink (%s) pio\n", _ep->name);
		net2272_done(ep, req, -ECONNRESET);
	}
-	req = NULL;
Another unrelated change.  These are all good changes but send them as
separate patches.
You are referring to the req = NULL, right?
Yes.
I've changed the use of 'req' in the same function and assumed that I can
just remove the unnecessary statement. But if it's better to do separately
I'll do that.
These are all changes which made me pause during my review to figure out
why they were necessary.  The line between what is a related part of a
patch is a bit vague and some maintainers will ask you to add or subtract
from a patch depending on their individual tastes.  I don't really have
an exact answer, but I felt like this patch needs to be subtracted from.

Especially if there is a whole chunk of the patch which can be removed,
then to me, that obviously should be in a different patch.

regards,
dan carpenter

Re: [PATCH 1/6] drivers: usb: remove usage of list iterator past the loop body

From: Joe Perches <joe@perches.com>
Date: 2022-02-28 18:41:51

On Mon, 2022-02-28 at 14:24 +0300, Dan Carpenter wrote:
a multi-line indent gets curly braces for readability even though
it's not required by C.  And then both sides would get curly braces.
That's more your personal preference than a coding style guideline.

Re: [PATCH 1/6] drivers: usb: remove usage of list iterator past the loop body

From: Dan Carpenter <hidden>
Date: 2022-03-01 05:54:33

On Mon, Feb 28, 2022 at 10:20:28AM -0800, Joe Perches wrote:
On Mon, 2022-02-28 at 14:24 +0300, Dan Carpenter wrote:
quoted
a multi-line indent gets curly braces for readability even though
it's not required by C.  And then both sides would get curly braces.
That's more your personal preference than a coding style guideline.
It's a USB patch.  I thought Greg prefered it that way.  Greg has some
specific style things which he likes and I have adopted and some I
pretend not to see.

regards,
dan carpenter

[PATCH 3/6] treewide: fix incorrect use to determine if list is empty

From: Jakob Koschel <hidden>
Date: 2022-02-28 11:09:07

The list iterator value will *always* be set by list_for_each_entry().
It is incorrect to assume that the iterator value will be NULL if the
list is empty.

Instead of checking the pointer it should be checked if
the list is empty.
In acpi_get_pmu_hw_inf() instead of setting the pointer to NULL
on the break, it is set to the correct value and leaving it
NULL if no element was found.

Signed-off-by: Jakob Koschel <redacted>
---
 arch/powerpc/sysdev/fsl_gtm.c            |  4 ++--
 drivers/media/pci/saa7134/saa7134-alsa.c |  4 ++--
 drivers/perf/xgene_pmu.c                 | 13 +++++++------
 3 files changed, 11 insertions(+), 10 deletions(-)
diff --git a/arch/powerpc/sysdev/fsl_gtm.c b/arch/powerpc/sysdev/fsl_gtm.c
index 8963eaffb1b7..39186ad6b3c3 100644
--- a/arch/powerpc/sysdev/fsl_gtm.c
+++ b/arch/powerpc/sysdev/fsl_gtm.c
@@ -86,7 +86,7 @@ static LIST_HEAD(gtms);
  */
 struct gtm_timer *gtm_get_timer16(void)
 {
-	struct gtm *gtm = NULL;
+	struct gtm *gtm;
 	int i;

 	list_for_each_entry(gtm, &gtms, list_node) {
@@ -103,7 +103,7 @@ struct gtm_timer *gtm_get_timer16(void)
 		spin_unlock_irq(&gtm->lock);
 	}

-	if (gtm)
+	if (!list_empty(&gtms))
 		return ERR_PTR(-EBUSY);
 	return ERR_PTR(-ENODEV);
 }
diff --git a/drivers/media/pci/saa7134/saa7134-alsa.c b/drivers/media/pci/saa7134/saa7134-alsa.c
index fb24d2ed3621..d3cde05a6eba 100644
--- a/drivers/media/pci/saa7134/saa7134-alsa.c
+++ b/drivers/media/pci/saa7134/saa7134-alsa.c
@@ -1214,7 +1214,7 @@ static int alsa_device_exit(struct saa7134_dev *dev)

 static int saa7134_alsa_init(void)
 {
-	struct saa7134_dev *dev = NULL;
+	struct saa7134_dev *dev;

 	saa7134_dmasound_init = alsa_device_init;
 	saa7134_dmasound_exit = alsa_device_exit;
@@ -1229,7 +1229,7 @@ static int saa7134_alsa_init(void)
 			alsa_device_init(dev);
 	}

-	if (dev == NULL)
+	if (list_empty(&saa7134_devlist))
 		pr_info("saa7134 ALSA: no saa7134 cards found\n");

 	return 0;
diff --git a/drivers/perf/xgene_pmu.c b/drivers/perf/xgene_pmu.c
index 2b6d476bd213..e255f9e665d1 100644
--- a/drivers/perf/xgene_pmu.c
+++ b/drivers/perf/xgene_pmu.c
@@ -1460,7 +1460,8 @@ xgene_pmu_dev_ctx *acpi_get_pmu_hw_inf(struct xgene_pmu *xgene_pmu,
 	struct hw_pmu_info *inf;
 	void __iomem *dev_csr;
 	struct resource res;
-	struct resource_entry *rentry;
+	struct resource_entry *rentry = NULL;
+	struct resource_entry *tmp;
 	int enable_bit;
 	int rc;
@@ -1475,16 +1476,16 @@ xgene_pmu_dev_ctx *acpi_get_pmu_hw_inf(struct xgene_pmu *xgene_pmu,
 		return NULL;
 	}

-	list_for_each_entry(rentry, &resource_list, node) {
-		if (resource_type(rentry->res) == IORESOURCE_MEM) {
-			res = *rentry->res;
-			rentry = NULL;
+	list_for_each_entry(tmp, &resource_list, node) {
+		if (resource_type(tmp->res) == IORESOURCE_MEM) {
+			res = *tmp->res;
+			rentry = tmp;
 			break;
 		}
 	}
 	acpi_dev_free_resource_list(&resource_list);

-	if (rentry) {
+	if (!rentry) {
 		dev_err(dev, "PMU type %d: No memory resource found\n", type);
 		return NULL;
 	}
--
2.25.1

Re: [PATCH 3/6] treewide: fix incorrect use to determine if list is empty

From: Dan Carpenter <hidden>
Date: 2022-02-28 11:40:45

On Mon, Feb 28, 2022 at 12:08:19PM +0100, Jakob Koschel wrote:
The list iterator value will *always* be set by list_for_each_entry().
It is incorrect to assume that the iterator value will be NULL if the
list is empty.

Instead of checking the pointer it should be checked if
the list is empty.
In acpi_get_pmu_hw_inf() instead of setting the pointer to NULL
on the break, it is set to the correct value and leaving it
NULL if no element was found.

Signed-off-by: Jakob Koschel <redacted>
---
 arch/powerpc/sysdev/fsl_gtm.c            |  4 ++--
 drivers/media/pci/saa7134/saa7134-alsa.c |  4 ++--
 drivers/perf/xgene_pmu.c                 | 13 +++++++------
 3 files changed, 11 insertions(+), 10 deletions(-)
These are all bug fixes.

1) Send them as 3 separate patches.
2) Add Fixes tags.

regards,
dan carpenter

[PATCH 4/6] drivers: remove unnecessary use of list iterator variable

From: Jakob Koschel <hidden>
Date: 2022-02-28 11:09:09

When list_for_each_entry() completes the iteration over the whole list
without breaking the loop, the iterator value will *always* be a bogus
pointer computed based on the head element.

To avoid type confusion use the actual list head directly instead of last
iterator value.

Signed-off-by: Jakob Koschel <redacted>
---
 drivers/dma/dw-edma/dw-edma-core.c             | 4 ++--
 drivers/net/ethernet/intel/i40e/i40e_ethtool.c | 3 ++-
 drivers/net/wireless/ath/ath6kl/htc_mbox.c     | 2 +-
 3 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
index 468d1097a1ec..7883c4831857 100644
--- a/drivers/dma/dw-edma/dw-edma-core.c
+++ b/drivers/dma/dw-edma/dw-edma-core.c
@@ -136,7 +136,7 @@ static void dw_edma_free_burst(struct dw_edma_chunk *chunk)
 	}

 	/* Remove the list head */
-	kfree(child);
+	kfree(chunk->burst);
 	chunk->burst = NULL;
 }
@@ -156,7 +156,7 @@ static void dw_edma_free_chunk(struct dw_edma_desc *desc)
 	}

 	/* Remove the list head */
-	kfree(child);
+	kfree(desc->chunk);
 	desc->chunk = NULL;
 }
diff --git a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
index 091f36adbbe1..c0ea9dbc4ff6 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
@@ -3963,7 +3963,8 @@ static void __i40e_reprogram_flex_pit(struct i40e_pf *pf,
 	 * correctly, the hardware will disable flexible field parsing.
 	 */
 	if (!list_empty(flex_pit_list))
-		last_offset = list_prev_entry(entry, list)->src_offset + 1;
+		last_offset = list_entry(flex_pit_list->prev,
+					 struct i40e_flex_pit, list)->src_offset + 1;

 	for (; i < 3; i++, last_offset++) {
 		i40e_write_rx_ctl(&pf->hw,
diff --git a/drivers/net/wireless/ath/ath6kl/htc_mbox.c b/drivers/net/wireless/ath/ath6kl/htc_mbox.c
index e3874421c4c0..cf5b05860799 100644
--- a/drivers/net/wireless/ath/ath6kl/htc_mbox.c
+++ b/drivers/net/wireless/ath/ath6kl/htc_mbox.c
@@ -104,7 +104,7 @@ static void ath6kl_credit_init(struct ath6kl_htc_credit_info *cred_info,
 	 * it use list_for_each_entry_reverse to walk around the whole ep list.
 	 * Therefore assign this lowestpri_ep_dist after walk around the ep_list
 	 */
-	cred_info->lowestpri_ep_dist = cur_ep_dist->list;
+	cred_info->lowestpri_ep_dist = *ep_list;

 	WARN_ON(cred_info->cur_free_credits <= 0);

--
2.25.1

[PATCH 6/6] treewide: remove check of list iterator against head past the loop body

From: Jakob Koschel <hidden>
Date: 2022-02-28 11:09:45

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
&pos->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 <redacted>
---
 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(&sram_lock);

-	list_for_each_entry(info, &sram_bank_list, node)
-		if (!strcmp(pool_name, info->pool_name))
+	list_for_each_entry(tmp, &sram_bank_list, node)
+		if (!strcmp(pool_name, tmp->pool_name)) {
+			info = tmp;
 			break;
+		}

 	mutex_unlock(&sram_lock);

-	if (&info->node == &sram_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(&ssp_lock);

-	list_for_each_entry(ssp, &ssp_list, node) {
-		if (ssp->port_id == port && ssp->use_count == 0) {
-			ssp->use_count++;
-			ssp->label = label;
+	list_for_each_entry(tmp, &ssp_list, node) {
+		if (tmp->port_id == port && tmp->use_count == 0) {
+			tmp->use_count++;
+			tmp->label = label;
+			ssp = tmp;
 			break;
 		}
 	}

 	mutex_unlock(&ssp_lock);
-
-	if (&ssp->node == &ssp_list)
-		return NULL;
-
 	return ssp;
 }
 EXPORT_SYMBOL(pxa_ssp_request);
@@ -62,22 +60,20 @@ struct ssp_device *pxa_ssp_request_of(const struct device_node *of_node,
 				      const char *label)
 {
 	struct ssp_device *ssp = NULL;
+	struct ssp_device *tmp;

 	mutex_lock(&ssp_lock);

-	list_for_each_entry(ssp, &ssp_list, node) {
-		if (ssp->of_node == of_node && ssp->use_count == 0) {
-			ssp->use_count++;
-			ssp->label = label;
+	list_for_each_entry(tmp, &ssp_list, node) {
+		if (tmp->of_node == of_node && tmp->use_count == 0) {
+			tmp->use_count++;
+			tmp->label = label;
+			ssp = tmp;
 			break;
 		}
 	}

 	mutex_unlock(&ssp_lock);
-
-	if (&ssp->node == &ssp_list)
-		return NULL;
-
 	return ssp;
 }
 EXPORT_SYMBOL(pxa_ssp_request_of);
diff --git a/drivers/block/drbd/drbd_req.c b/drivers/block/drbd/drbd_req.c
index 3235532ae077..ee7ee8b657bd 100644
--- a/drivers/block/drbd/drbd_req.c
+++ b/drivers/block/drbd/drbd_req.c
@@ -332,17 +332,22 @@ static void set_if_null_req_next(struct drbd_peer_device *peer_device, struct dr
 static void advance_conn_req_next(struct drbd_peer_device *peer_device, struct drbd_request *req)
 {
 	struct drbd_connection *connection = peer_device ? peer_device->connection : NULL;
+	struct drbd_request *tmp;
 	if (!connection)
 		return;
 	if (connection->req_next != req)
 		return;
-	list_for_each_entry_continue(req, &connection->transfer_log, tl_requests) {
-		const unsigned s = req->rq_state;
-		if (s & RQ_NET_QUEUED)
+
+	tmp = req;
+	req = NULL;
+	list_for_each_entry_continue(tmp, &connection->transfer_log, tl_requests) {
+		const unsigned int s = tmp->rq_state;
+
+		if (s & RQ_NET_QUEUED) {
+			req = tmp;
 			break;
+		}
 	}
-	if (&req->tl_requests == &connection->transfer_log)
-		req = NULL;
 	connection->req_next = req;
 }
@@ -358,17 +363,22 @@ static void set_if_null_req_ack_pending(struct drbd_peer_device *peer_device, st
 static void advance_conn_req_ack_pending(struct drbd_peer_device *peer_device, struct drbd_request *req)
 {
 	struct drbd_connection *connection = peer_device ? peer_device->connection : NULL;
+	struct drbd_request *tmp;
 	if (!connection)
 		return;
 	if (connection->req_ack_pending != req)
 		return;
-	list_for_each_entry_continue(req, &connection->transfer_log, tl_requests) {
-		const unsigned s = req->rq_state;
-		if ((s & RQ_NET_SENT) && (s & RQ_NET_PENDING))
+
+	tmp = req;
+	req = NULL;
+	list_for_each_entry_continue(tmp, &connection->transfer_log, tl_requests) {
+		const unsigned int s = tmp->rq_state;
+
+		if ((s & RQ_NET_SENT) && (s & RQ_NET_PENDING)) {
+			req = tmp;
 			break;
+		}
 	}
-	if (&req->tl_requests == &connection->transfer_log)
-		req = NULL;
 	connection->req_ack_pending = req;
 }
@@ -384,17 +394,22 @@ static void set_if_null_req_not_net_done(struct drbd_peer_device *peer_device, s
 static void advance_conn_req_not_net_done(struct drbd_peer_device *peer_device, struct drbd_request *req)
 {
 	struct drbd_connection *connection = peer_device ? peer_device->connection : NULL;
+	struct drbd_request *tmp;
 	if (!connection)
 		return;
 	if (connection->req_not_net_done != req)
 		return;
-	list_for_each_entry_continue(req, &connection->transfer_log, tl_requests) {
-		const unsigned s = req->rq_state;
-		if ((s & RQ_NET_SENT) && !(s & RQ_NET_DONE))
+
+	tmp = req;
+	req = NULL;
+	list_for_each_entry_continue(tmp, &connection->transfer_log, tl_requests) {
+		const unsigned int s = tmp->rq_state;
+
+		if ((s & RQ_NET_SENT) && !(s & RQ_NET_DONE)) {
+			req = tmp;
 			break;
+		}
 	}
-	if (&req->tl_requests == &connection->transfer_log)
-		req = NULL;
 	connection->req_not_net_done = req;
 }
diff --git a/drivers/counter/counter-chrdev.c b/drivers/counter/counter-chrdev.c
index b7c62f957a6a..6548dd9f02ac 100644
--- a/drivers/counter/counter-chrdev.c
+++ b/drivers/counter/counter-chrdev.c
@@ -131,18 +131,21 @@ static int counter_set_event_node(struct counter_device *const counter,
 				  struct counter_watch *const watch,
 				  const struct counter_comp_node *const cfg)
 {
-	struct counter_event_node *event_node;
+	struct counter_event_node *event_node = NULL;
+	struct counter_event_node *tmp;
 	int err = 0;
 	struct counter_comp_node *comp_node;

 	/* Search for event in the list */
-	list_for_each_entry(event_node, &counter->next_events_list, l)
-		if (event_node->event == watch->event &&
-		    event_node->channel == watch->channel)
+	list_for_each_entry(tmp, &counter->next_events_list, l)
+		if (tmp->event == watch->event &&
+		    tmp->channel == watch->channel) {
+			event_node = tmp;
 			break;
+		}

 	/* If event is not already in the list */
-	if (&event_node->l == &counter->next_events_list) {
+	if (!event_node) {
 		/* Allocate new event node */
 		event_node = kmalloc(sizeof(*event_node), GFP_KERNEL);
 		if (!event_node)
@@ -535,7 +538,8 @@ void counter_push_event(struct counter_device *const counter, const u8 event,
 	struct counter_event ev;
 	unsigned int copied = 0;
 	unsigned long flags;
-	struct counter_event_node *event_node;
+	struct counter_event_node *event_node = NULL;
+	struct counter_event_node *tmp;
 	struct counter_comp_node *comp_node;

 	ev.timestamp = ktime_get_ns();
@@ -546,13 +550,15 @@ void counter_push_event(struct counter_device *const counter, const u8 event,
 	spin_lock_irqsave(&counter->events_list_lock, flags);

 	/* Search for event in the list */
-	list_for_each_entry(event_node, &counter->events_list, l)
-		if (event_node->event == event &&
-		    event_node->channel == channel)
+	list_for_each_entry(tmp, &counter->events_list, l)
+		if (tmp->event == event &&
+		    tmp->channel == channel) {
+			event_node = tmp;
 			break;
+		}

 	/* If event is not in the list */
-	if (&event_node->l == &counter->events_list)
+	if (!event_node)
 		goto exit_early;

 	/* Read and queue relevant comp for userspace */
diff --git a/drivers/crypto/cavium/nitrox/nitrox_main.c b/drivers/crypto/cavium/nitrox/nitrox_main.c
index 6c61817996a3..a003659bf66f 100644
--- a/drivers/crypto/cavium/nitrox/nitrox_main.c
+++ b/drivers/crypto/cavium/nitrox/nitrox_main.c
@@ -269,15 +269,18 @@ static void nitrox_remove_from_devlist(struct nitrox_device *ndev)

 struct nitrox_device *nitrox_get_first_device(void)
 {
-	struct nitrox_device *ndev;
+	struct nitrox_device *ndev = NULL;
+	struct nitrox_device *tmp;

 	mutex_lock(&devlist_lock);
-	list_for_each_entry(ndev, &ndevlist, list) {
-		if (nitrox_ready(ndev))
+	list_for_each_entry(tmp, &ndevlist, list) {
+		if (nitrox_ready(tmp)) {
+			ndev = tmp;
 			break;
+		}
 	}
 	mutex_unlock(&devlist_lock);
-	if (&ndev->list == &ndevlist)
+	if (!ndev)
 		return NULL;

 	refcount_inc(&ndev->refcnt);
diff --git a/drivers/dma/ppc4xx/adma.c b/drivers/dma/ppc4xx/adma.c
index 5e46e347e28b..542286e1f0cf 100644
--- a/drivers/dma/ppc4xx/adma.c
+++ b/drivers/dma/ppc4xx/adma.c
@@ -935,23 +935,26 @@ static void ppc440spe_adma_device_clear_eot_status(
 			if (rv & DMA_CDB_STATUS_MSK) {
 				/* ZeroSum check failed
 				 */
-				struct ppc440spe_adma_desc_slot *iter;
+				struct ppc440spe_adma_desc_slot *iter = NULL;
+				struct ppc440spe_adma_desc_slot *tmp;
 				dma_addr_t phys = rv & ~DMA_CDB_MSK;

 				/*
 				 * Update the status of corresponding
 				 * descriptor.
 				 */
-				list_for_each_entry(iter, &chan->chain,
+				list_for_each_entry(tmp, &chan->chain,
 				    chain_node) {
-					if (iter->phys == phys)
+					if (tmp->phys == phys) {
+						iter = tmp;
 						break;
+					}
 				}
 				/*
 				 * if cannot find the corresponding
 				 * slot it's a bug
 				 */
-				BUG_ON(&iter->chain_node == &chan->chain);
+				BUG_ON(!iter);

 				if (iter->xor_check_result) {
 					if (test_bit(PPC440SPE_DESC_PCHECK,
diff --git a/drivers/firewire/core-transaction.c b/drivers/firewire/core-transaction.c
index ac487c96bb71..870cbfb84f4f 100644
--- a/drivers/firewire/core-transaction.c
+++ b/drivers/firewire/core-transaction.c
@@ -73,24 +73,26 @@ static int try_cancel_split_timeout(struct fw_transaction *t)
 static int close_transaction(struct fw_transaction *transaction,
 			     struct fw_card *card, int rcode)
 {
-	struct fw_transaction *t;
+	struct fw_transaction *t = NULL;
+	struct fw_transaction *tmp;
 	unsigned long flags;

 	spin_lock_irqsave(&card->lock, flags);
-	list_for_each_entry(t, &card->transaction_list, link) {
-		if (t == transaction) {
-			if (!try_cancel_split_timeout(t)) {
+	list_for_each_entry(tmp, &card->transaction_list, link) {
+		if (tmp == transaction) {
+			if (!try_cancel_split_timeout(tmp)) {
 				spin_unlock_irqrestore(&card->lock, flags);
 				goto timed_out;
 			}
-			list_del_init(&t->link);
-			card->tlabel_mask &= ~(1ULL << t->tlabel);
+			list_del_init(&tmp->link);
+			card->tlabel_mask &= ~(1ULL << tmp->tlabel);
+			t = tmp;
 			break;
 		}
 	}
 	spin_unlock_irqrestore(&card->lock, flags);

-	if (&t->link != &card->transaction_list) {
+	if (t) {
 		t->callback(card, rcode, NULL, 0, t->callback_data);
 		return 0;
 	}
@@ -935,7 +937,8 @@ EXPORT_SYMBOL(fw_core_handle_request);

 void fw_core_handle_response(struct fw_card *card, struct fw_packet *p)
 {
-	struct fw_transaction *t;
+	struct fw_transaction *t = NULL;
+	struct fw_transaction *tmp;
 	unsigned long flags;
 	u32 *data;
 	size_t data_length;
@@ -947,20 +950,21 @@ void fw_core_handle_response(struct fw_card *card, struct fw_packet *p)
 	rcode	= HEADER_GET_RCODE(p->header[1]);

 	spin_lock_irqsave(&card->lock, flags);
-	list_for_each_entry(t, &card->transaction_list, link) {
-		if (t->node_id == source && t->tlabel == tlabel) {
-			if (!try_cancel_split_timeout(t)) {
+	list_for_each_entry(tmp, &card->transaction_list, link) {
+		if (tmp->node_id == source && tmp->tlabel == tlabel) {
+			if (!try_cancel_split_timeout(tmp)) {
 				spin_unlock_irqrestore(&card->lock, flags);
 				goto timed_out;
 			}
-			list_del_init(&t->link);
-			card->tlabel_mask &= ~(1ULL << t->tlabel);
+			list_del_init(&tmp->link);
+			card->tlabel_mask &= ~(1ULL << tmp->tlabel);
+			t = tmp;
 			break;
 		}
 	}
 	spin_unlock_irqrestore(&card->lock, flags);

-	if (&t->link == &card->transaction_list) {
+	if (!t) {
  timed_out:
 		fw_notice(card, "unsolicited response (source %x, tlabel %x)\n",
 			  source, tlabel);
diff --git a/drivers/firewire/sbp2.c b/drivers/firewire/sbp2.c
index 85cd379fd383..d01aabda7cad 100644
--- a/drivers/firewire/sbp2.c
+++ b/drivers/firewire/sbp2.c
@@ -408,7 +408,8 @@ static void sbp2_status_write(struct fw_card *card, struct fw_request *request,
 			      void *payload, size_t length, void *callback_data)
 {
 	struct sbp2_logical_unit *lu = callback_data;
-	struct sbp2_orb *orb;
+	struct sbp2_orb *orb = NULL;
+	struct sbp2_orb *tmp;
 	struct sbp2_status status;
 	unsigned long flags;
@@ -433,17 +434,18 @@ static void sbp2_status_write(struct fw_card *card, struct fw_request *request,

 	/* Lookup the orb corresponding to this status write. */
 	spin_lock_irqsave(&lu->tgt->lock, flags);
-	list_for_each_entry(orb, &lu->orb_list, link) {
+	list_for_each_entry(tmp, &lu->orb_list, link) {
 		if (STATUS_GET_ORB_HIGH(status) == 0 &&
-		    STATUS_GET_ORB_LOW(status) == orb->request_bus) {
-			orb->rcode = RCODE_COMPLETE;
-			list_del(&orb->link);
+		    STATUS_GET_ORB_LOW(status) == tmp->request_bus) {
+			tmp->rcode = RCODE_COMPLETE;
+			list_del(&tmp->link);
+			orb = tmp;
 			break;
 		}
 	}
 	spin_unlock_irqrestore(&lu->tgt->lock, flags);

-	if (&orb->link != &lu->orb_list) {
+	if (orb) {
 		orb->callback(orb, &status);
 		kref_put(&orb->kref, free_orb); /* orb callback reference */
 	} else {
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index b37fc7d7d2c7..8b38e2fb0674 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -2444,26 +2444,31 @@ int amdgpu_vm_bo_unmap(struct amdgpu_device *adev,
 		       struct amdgpu_bo_va *bo_va,
 		       uint64_t saddr)
 {
-	struct amdgpu_bo_va_mapping *mapping;
+	struct amdgpu_bo_va_mapping *mapping = NULL;
+	struct amdgpu_bo_va_mapping *tmp;
 	struct amdgpu_vm *vm = bo_va->base.vm;
 	bool valid = true;

 	saddr /= AMDGPU_GPU_PAGE_SIZE;

-	list_for_each_entry(mapping, &bo_va->valids, list) {
-		if (mapping->start == saddr)
+	list_for_each_entry(tmp, &bo_va->valids, list) {
+		if (tmp->start == saddr) {
+			mapping = tmp;
 			break;
+		}
 	}

-	if (&mapping->list == &bo_va->valids) {
+	if (!mapping) {
 		valid = false;

-		list_for_each_entry(mapping, &bo_va->invalids, list) {
-			if (mapping->start == saddr)
+		list_for_each_entry(tmp, &bo_va->invalids, list) {
+			if (tmp->start == saddr) {
+				mapping = tmp;
 				break;
+			}
 		}

-		if (&mapping->list == &bo_va->invalids)
+		if (!mapping)
 			return -ENOENT;
 	}
diff --git a/drivers/gpu/drm/drm_memory.c b/drivers/gpu/drm/drm_memory.c
index d2e1dccd8113..99ddb7ad9eb7 100644
--- a/drivers/gpu/drm/drm_memory.c
+++ b/drivers/gpu/drm/drm_memory.c
@@ -60,7 +60,8 @@ static void *agp_remap(unsigned long offset, unsigned long size,
 {
 	unsigned long i, num_pages =
 	    PAGE_ALIGN(size) / PAGE_SIZE;
-	struct drm_agp_mem *agpmem;
+	struct drm_agp_mem *agpmem = NULL;
+	struct drm_agp_mem *tmp;
 	struct page **page_map;
 	struct page **phys_page_map;
 	void *addr;
@@ -71,12 +72,14 @@ static void *agp_remap(unsigned long offset, unsigned long size,
 	offset -= dev->hose->mem_space->start;
 #endif

-	list_for_each_entry(agpmem, &dev->agp->memory, head)
-		if (agpmem->bound <= offset
-		    && (agpmem->bound + (agpmem->pages << PAGE_SHIFT)) >=
-		    (offset + size))
+	list_for_each_entry(tmp, &dev->agp->memory, head)
+		if (tmp->bound <= offset
+		    && (tmp->bound + (tmp->pages << PAGE_SHIFT)) >=
+		    (offset + size)) {
+			agpmem = tmp;
 			break;
-	if (&agpmem->head == &dev->agp->memory)
+		}
+	if (!agpmem)
 		return NULL;

 	/*
diff --git a/drivers/gpu/drm/drm_mm.c b/drivers/gpu/drm/drm_mm.c
index 8257f9d4f619..0124e8dfa134 100644
--- a/drivers/gpu/drm/drm_mm.c
+++ b/drivers/gpu/drm/drm_mm.c
@@ -912,7 +912,8 @@ EXPORT_SYMBOL(drm_mm_scan_remove_block);
 struct drm_mm_node *drm_mm_scan_color_evict(struct drm_mm_scan *scan)
 {
 	struct drm_mm *mm = scan->mm;
-	struct drm_mm_node *hole;
+	struct drm_mm_node *hole = NULL;
+	struct drm_mm_node *tmp;
 	u64 hole_start, hole_end;

 	DRM_MM_BUG_ON(list_empty(&mm->hole_stack));
@@ -925,18 +926,20 @@ struct drm_mm_node *drm_mm_scan_color_evict(struct drm_mm_scan *scan)
 	 * in the hole_stack list, but due to side-effects in the driver it
 	 * may not be.
 	 */
-	list_for_each_entry(hole, &mm->hole_stack, hole_stack) {
-		hole_start = __drm_mm_hole_node_start(hole);
-		hole_end = hole_start + hole->hole_size;
+	list_for_each_entry(tmp, &mm->hole_stack, hole_stack) {
+		hole_start = __drm_mm_hole_node_start(tmp);
+		hole_end = hole_start + tmp->hole_size;

 		if (hole_start <= scan->hit_start &&
-		    hole_end >= scan->hit_end)
+		    hole_end >= scan->hit_end) {
+			hole = tmp;
 			break;
+		}
 	}

 	/* We should only be called after we found the hole previously */
-	DRM_MM_BUG_ON(&hole->hole_stack == &mm->hole_stack);
-	if (unlikely(&hole->hole_stack == &mm->hole_stack))
+	DRM_MM_BUG_ON(!hole);
+	if (unlikely(!hole))
 		return NULL;

 	DRM_MM_BUG_ON(hole_start > scan->hit_start);
diff --git a/drivers/gpu/drm/drm_vm.c b/drivers/gpu/drm/drm_vm.c
index e957d4851dc0..630b2bbd172e 100644
--- a/drivers/gpu/drm/drm_vm.c
+++ b/drivers/gpu/drm/drm_vm.c
@@ -138,7 +138,8 @@ static vm_fault_t drm_vm_fault(struct vm_fault *vmf)
 		 */
 		resource_size_t offset = vmf->address - vma->vm_start;
 		resource_size_t baddr = map->offset + offset;
-		struct drm_agp_mem *agpmem;
+		struct drm_agp_mem *agpmem = NULL;
+		struct drm_agp_mem *tmp;
 		struct page *page;

 #ifdef __alpha__
@@ -151,13 +152,15 @@ static vm_fault_t drm_vm_fault(struct vm_fault *vmf)
 		/*
 		 * It's AGP memory - find the real physical page to map
 		 */
-		list_for_each_entry(agpmem, &dev->agp->memory, head) {
-			if (agpmem->bound <= baddr &&
-			    agpmem->bound + agpmem->pages * PAGE_SIZE > baddr)
+		list_for_each_entry(tmp, &dev->agp->memory, head) {
+			if (tmp->bound <= baddr &&
+			    tmp->bound + tmp->pages * PAGE_SIZE > baddr) {
+				agpmem = tmp;
 				break;
+			}
 		}

-		if (&agpmem->head == &dev->agp->memory)
+		if (!agpmem)
 			goto vm_fault_error;

 		/*
diff --git a/drivers/gpu/drm/gma500/oaktrail_lvds.c b/drivers/gpu/drm/gma500/oaktrail_lvds.c
index 28b995ef2844..2df1cbef0965 100644
--- a/drivers/gpu/drm/gma500/oaktrail_lvds.c
+++ b/drivers/gpu/drm/gma500/oaktrail_lvds.c
@@ -87,6 +87,7 @@ static void oaktrail_lvds_mode_set(struct drm_encoder *encoder,
 	struct psb_intel_mode_device *mode_dev = &dev_priv->mode_dev;
 	struct drm_mode_config *mode_config = &dev->mode_config;
 	struct drm_connector *connector = NULL;
+	struct drm_connector *tmp;
 	struct drm_crtc *crtc = encoder->crtc;
 	u32 lvds_port;
 	uint64_t v = DRM_MODE_SCALE_FULLSCREEN;
@@ -112,12 +113,14 @@ static void oaktrail_lvds_mode_set(struct drm_encoder *encoder,
 	REG_WRITE(LVDS, lvds_port);

 	/* Find the connector we're trying to set up */
-	list_for_each_entry(connector, &mode_config->connector_list, head) {
-		if (connector->encoder && connector->encoder->crtc == crtc)
+	list_for_each_entry(tmp, &mode_config->connector_list, head) {
+		if (tmp->encoder && tmp->encoder->crtc == crtc) {
+			connector = tmp;
 			break;
+		}
 	}

-	if (list_entry_is_head(connector, &mode_config->connector_list, head)) {
+	if (!connector) {
 		DRM_ERROR("Couldn't find connector when setting mode");
 		gma_power_end(dev);
 		return;
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, &ctx->handles_vma, &iter, 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(&obj->base.refcount))
 			continue;

 		spin_lock(&obj->lut_lock);
-		list_for_each_entry(lut, &obj->lut_list, obj_link) {
-			if (lut->ctx != ctx)
+		list_for_each_entry(tmp, &obj->lut_list, obj_link) {
+			if (tmp->ctx != ctx)
 				continue;

-			if (lut->handle != iter.index)
+			if (tmp->handle != iter.index)
 				continue;

-			list_del(&lut->obj_link);
+			list_del(&tmp->obj_link);
+			lut = tmp;
 			break;
 		}
 		spin_unlock(&obj->lut_lock);

-		if (&lut->obj_link != &obj->lut_list) {
+		if (lut) {
 			i915_lut_handle_free(lut);
 			radix_tree_iter_delete(&ctx->handles_vma, &iter, slot);
 			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, &tl->requests, link) {
-		if (rq->ring != ring)
+	list_for_each_entry(tmp, &tl->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 (&rq->link == &tl->requests)
+	if (!rq)
 		return NULL; /* weird, we will check again later for real */

 	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 i915_request *target = NULL;
+	struct i915_request *tmp;
 	long timeout;

 	if (intel_ring_update_space(ring) >= bytes)
 		return 0;

 	GEM_BUG_ON(list_empty(&tl->requests));
-	list_for_each_entry(target, &tl->requests, link) {
-		if (target->ring != ring)
+	list_for_each_entry(tmp, &tl->requests, link) {
+		if (tmp->ring != ring)
 			continue;

 		/* Would completion of this request free enough space? */
-		if (bytes <= __intel_ring_space(target->postfix,
-						ring->emit, ring->size))
+		if (bytes <= __intel_ring_space(tmp->postfix,
+						ring->emit, ring->size)) {
+			target = tmp;
 			break;
+		}
 	}

-	if (GEM_WARN_ON(&target->link == &tl->requests))
+	if (GEM_WARN_ON(!target))
 		return -ENOSPC;

 	timeout = i915_request_wait(target,
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/fb/ramgk104.c b/drivers/gpu/drm/nouveau/nvkm/subdev/fb/ramgk104.c
index 2b678b60b4d3..c1f99d34e334 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/fb/ramgk104.c
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/fb/ramgk104.c
@@ -1155,17 +1155,20 @@ static void
 gk104_ram_prog_0(struct gk104_ram *ram, u32 freq)
 {
 	struct nvkm_device *device = ram->base.fb->subdev.device;
-	struct nvkm_ram_data *cfg;
+	struct nvkm_ram_data *cfg = NULL;
+	struct nvkm_ram_data *tmp;
 	u32 mhz = freq / 1000;
 	u32 mask, data;

-	list_for_each_entry(cfg, &ram->cfg, head) {
-		if (mhz >= cfg->bios.rammap_min &&
-		    mhz <= cfg->bios.rammap_max)
+	list_for_each_entry(tmp, &ram->cfg, head) {
+		if (mhz >= tmp->bios.rammap_min &&
+		    mhz <= tmp->bios.rammap_max) {
+			cfg = tmp;
 			break;
+		}
 	}

-	if (&cfg->head == &ram->cfg)
+	if (!cfg)
 		return;

 	if (mask = 0, data = 0, ram->diff.rammap_11_0a_03fe) {
diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
index f91fb31ab7a7..2051abe5337a 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -1081,7 +1081,8 @@ void drm_sched_increase_karma_ext(struct drm_sched_job *bad, int type)
 {
 	int i;
 	struct drm_sched_entity *tmp;
-	struct drm_sched_entity *entity;
+	struct drm_sched_entity *entity = NULL;
+	struct drm_sched_entity *iter;
 	struct drm_gpu_scheduler *sched = bad->sched;

 	/* don't change @bad's karma if it's from KERNEL RQ,
@@ -1099,16 +1100,17 @@ void drm_sched_increase_karma_ext(struct drm_sched_job *bad, int type)
 			struct drm_sched_rq *rq = &sched->sched_rq[i];

 			spin_lock(&rq->lock);
-			list_for_each_entry_safe(entity, tmp, &rq->entities, list) {
+			list_for_each_entry_safe(iter, tmp, &rq->entities, list) {
 				if (bad->s_fence->scheduled.context ==
-				    entity->fence_context) {
-					if (entity->guilty)
-						atomic_set(entity->guilty, type);
+				    iter->fence_context) {
+					if (iter->guilty)
+						atomic_set(iter->guilty, type);
+					entity = iter;
 					break;
 				}
 			}
 			spin_unlock(&rq->lock);
-			if (&entity->list != &rq->entities)
+			if (entity)
 				break;
 		}
 	}
diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index db3dc7ef5382..d4e0899f87d3 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -672,37 +672,36 @@ int ttm_mem_evict_first(struct ttm_device *bdev,
 			struct ttm_operation_ctx *ctx,
 			struct ww_acquire_ctx *ticket)
 {
-	struct ttm_buffer_object *bo = NULL, *busy_bo = NULL;
+	struct ttm_buffer_object *bo = NULL, *tmp, *busy_bo = NULL;
 	bool locked = false;
 	unsigned i;
 	int ret;

 	spin_lock(&bdev->lru_lock);
 	for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
-		list_for_each_entry(bo, &man->lru[i], lru) {
+		list_for_each_entry(tmp, &man->lru[i], lru) {
 			bool busy;

-			if (!ttm_bo_evict_swapout_allowable(bo, ctx, place,
+			if (!ttm_bo_evict_swapout_allowable(tmp, ctx, place,
 							    &locked, &busy)) {
 				if (busy && !busy_bo && ticket !=
-				    dma_resv_locking_ctx(bo->base.resv))
-					busy_bo = bo;
+				    dma_resv_locking_ctx(tmp->base.resv))
+					busy_bo = tmp;
 				continue;
 			}

-			if (!ttm_bo_get_unless_zero(bo)) {
+			if (!ttm_bo_get_unless_zero(tmp)) {
 				if (locked)
-					dma_resv_unlock(bo->base.resv);
+					dma_resv_unlock(tmp->base.resv);
 				continue;
 			}
+			bo = tmp;
 			break;
 		}

 		/* If the inner loop terminated early, we have our candidate */
-		if (&bo->lru != &man->lru[i])
+		if (bo)
 			break;
-
-		bo = NULL;
 	}

 	if (!bo) {
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
index bbd2f4ec08ec..8f1890cf438e 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
@@ -2582,22 +2582,26 @@ int vmw_kms_fbdev_init_data(struct vmw_private *dev_priv,
 			    struct drm_crtc **p_crtc,
 			    struct drm_display_mode **p_mode)
 {
-	struct drm_connector *con;
+	struct drm_connector *con = NULL;
+	struct drm_connector *tmp1;
 	struct vmw_display_unit *du;
-	struct drm_display_mode *mode;
+	struct drm_display_mode *mode = NULL;
+	struct drm_display_mode *tmp2;
 	int i = 0;
 	int ret = 0;

 	mutex_lock(&dev_priv->drm.mode_config.mutex);
-	list_for_each_entry(con, &dev_priv->drm.mode_config.connector_list,
+	list_for_each_entry(tmp1, &dev_priv->drm.mode_config.connector_list,
 			    head) {
-		if (i == unit)
+		if (i == unit) {
+			con = tmp1;
 			break;
+		}

 		++i;
 	}

-	if (&con->head == &dev_priv->drm.mode_config.connector_list) {
+	if (!con) {
 		DRM_ERROR("Could not find initial display unit.\n");
 		ret = -EINVAL;
 		goto out_unlock;
@@ -2616,12 +2620,14 @@ int vmw_kms_fbdev_init_data(struct vmw_private *dev_priv,
 	*p_con = con;
 	*p_crtc = &du->crtc;

-	list_for_each_entry(mode, &con->modes, head) {
-		if (mode->type & DRM_MODE_TYPE_PREFERRED)
+	list_for_each_entry(tmp2, &con->modes, head) {
+		if (tmp2->type & DRM_MODE_TYPE_PREFERRED) {
+			mode = tmp2;
 			break;
+		}
 	}

-	if (&mode->head == &con->modes) {
+	if (!mode) {
 		WARN_ONCE(true, "Could not find initial preferred mode.\n");
 		*p_mode = list_first_entry(&con->modes,
 					   struct drm_display_mode,
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,  &rcd->tid_group_list.list, list) {
-		kern_add_tid_node(flow, rcd, "complete groups", group,
-				  group->size);
+	list_for_each_entry(tmp,  &rcd->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
 	 */
-	if (group && &group->list == &rcd->tid_group_list.list)
+	if (!group)
 		goto bail_eagain;
 	group = list_prepare_entry(group, &rcd->tid_group_list.list,
 				   list);
diff --git a/drivers/infiniband/hw/mlx4/main.c b/drivers/infiniband/hw/mlx4/main.c
index 93b1650eacfa..4659d879e97d 100644
--- a/drivers/infiniband/hw/mlx4/main.c
+++ b/drivers/infiniband/hw/mlx4/main.c
@@ -1920,17 +1920,19 @@ static int mlx4_ib_mcg_detach(struct ib_qp *ibqp, union ib_gid *gid, u16 lid)

 	if (mdev->dev->caps.steering_mode ==
 	    MLX4_STEERING_MODE_DEVICE_MANAGED) {
-		struct mlx4_ib_steering *ib_steering;
+		struct mlx4_ib_steering *ib_steering = NULL;
+		struct mlx4_ib_steering *tmp;

 		mutex_lock(&mqp->mutex);
-		list_for_each_entry(ib_steering, &mqp->steering_rules, list) {
-			if (!memcmp(ib_steering->gid.raw, gid->raw, 16)) {
-				list_del(&ib_steering->list);
+		list_for_each_entry(tmp, &mqp->steering_rules, list) {
+			if (!memcmp(tmp->gid.raw, gid->raw, 16)) {
+				list_del(&tmp->list);
+				ib_steering = tmp;
 				break;
 			}
 		}
 		mutex_unlock(&mqp->mutex);
-		if (&ib_steering->list == &mqp->steering_rules) {
+		if (!ib_steering) {
 			pr_err("Couldn't find reg_id for mgid. Steering rule is left attached\n");
 			return -EINVAL;
 		}
diff --git a/drivers/media/dvb-frontends/mxl5xx.c b/drivers/media/dvb-frontends/mxl5xx.c
index 934d1c0b214a..78c37ce56e32 100644
--- a/drivers/media/dvb-frontends/mxl5xx.c
+++ b/drivers/media/dvb-frontends/mxl5xx.c
@@ -492,17 +492,20 @@ static int enable_tuner(struct mxl *state, u32 tuner, u32 enable);
 static int sleep(struct dvb_frontend *fe)
 {
 	struct mxl *state = fe->demodulator_priv;
-	struct mxl *p;
+	struct mxl *p = NULL;
+	struct mxl *tmp;

 	cfg_demod_abort_tune(state);
 	if (state->tuner_in_use != 0xffffffff) {
 		mutex_lock(&state->base->tune_lock);
 		state->tuner_in_use = 0xffffffff;
-		list_for_each_entry(p, &state->base->mxls, mxl) {
-			if (p->tuner_in_use == state->tuner)
+		list_for_each_entry(tmp, &state->base->mxls, mxl) {
+			if (tmp->tuner_in_use == state->tuner) {
+				p = tmp;
 				break;
+			}
 		}
-		if (&p->mxl == &state->base->mxls)
+		if (!p)
 			enable_tuner(state, state->tuner, 0);
 		mutex_unlock(&state->base->tune_lock);
 	}
diff --git a/drivers/media/v4l2-core/v4l2-ctrls-api.c b/drivers/media/v4l2-core/v4l2-ctrls-api.c
index db9baa0bd05f..9245fd5e546c 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls-api.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls-api.c
@@ -942,6 +942,7 @@ int v4l2_query_ext_ctrl(struct v4l2_ctrl_handler *hdl, struct v4l2_query_ext_ctr
 	const unsigned int next_flags = V4L2_CTRL_FLAG_NEXT_CTRL | V4L2_CTRL_FLAG_NEXT_COMPOUND;
 	u32 id = qc->id & V4L2_CTRL_ID_MASK;
 	struct v4l2_ctrl_ref *ref;
+	struct v4l2_ctrl_ref *tmp;
 	struct v4l2_ctrl *ctrl;

 	if (!hdl)
@@ -976,15 +977,17 @@ int v4l2_query_ext_ctrl(struct v4l2_ctrl_handler *hdl, struct v4l2_query_ext_ctr
 			 * We found a control with the given ID, so just get
 			 * the next valid one in the list.
 			 */
-			list_for_each_entry_continue(ref, &hdl->ctrl_refs, node) {
-				is_compound = ref->ctrl->is_array ||
-					ref->ctrl->type >= V4L2_CTRL_COMPOUND_TYPES;
-				if (id < ref->ctrl->id &&
-				    (is_compound & mask) == match)
+			tmp = ref;
+			ref = NULL;
+			list_for_each_entry_continue(tmp, &hdl->ctrl_refs, node) {
+				is_compound = tmp->ctrl->is_array ||
+					tmp->ctrl->type >= V4L2_CTRL_COMPOUND_TYPES;
+				if (id < tmp->ctrl->id &&
+				    (is_compound & mask) == match) {
+					ref = tmp;
 					break;
+				}
 			}
-			if (&ref->node == &hdl->ctrl_refs)
-				ref = NULL;
 		} else {
 			/*
 			 * No control with the given ID exists, so start
@@ -992,15 +995,15 @@ int v4l2_query_ext_ctrl(struct v4l2_ctrl_handler *hdl, struct v4l2_query_ext_ctr
 			 * is one, otherwise the first 'if' above would have
 			 * been true.
 			 */
-			list_for_each_entry(ref, &hdl->ctrl_refs, node) {
-				is_compound = ref->ctrl->is_array ||
-					ref->ctrl->type >= V4L2_CTRL_COMPOUND_TYPES;
-				if (id < ref->ctrl->id &&
-				    (is_compound & mask) == match)
+			list_for_each_entry(tmp, &hdl->ctrl_refs, node) {
+				is_compound = tmp->ctrl->is_array ||
+					tmp->ctrl->type >= V4L2_CTRL_COMPOUND_TYPES;
+				if (id < tmp->ctrl->id &&
+				    (is_compound & mask) == match) {
+					ref = tmp;
 					break;
+				}
 			}
-			if (&ref->node == &hdl->ctrl_refs)
-				ref = NULL;
 		}
 	}
 	mutex_unlock(hdl->lock);
diff --git a/drivers/misc/mei/interrupt.c b/drivers/misc/mei/interrupt.c
index a67f4f2d33a9..f15b91e22b9d 100644
--- a/drivers/misc/mei/interrupt.c
+++ b/drivers/misc/mei/interrupt.c
@@ -329,7 +329,8 @@ int mei_irq_read_handler(struct mei_device *dev,
 {
 	struct mei_msg_hdr *mei_hdr;
 	struct mei_ext_meta_hdr *meta_hdr = NULL;
-	struct mei_cl *cl;
+	struct mei_cl *cl = NULL;
+	struct mei_cl *tmp;
 	int ret;
 	u32 hdr_size_left;
 	u32 hdr_size_ext;
@@ -421,15 +422,16 @@ int mei_irq_read_handler(struct mei_device *dev,
 	}

 	/* find recipient cl */
-	list_for_each_entry(cl, &dev->file_list, link) {
-		if (mei_cl_hbm_equal(cl, mei_hdr)) {
-			cl_dbg(dev, cl, "got a message\n");
+	list_for_each_entry(tmp, &dev->file_list, link) {
+		if (mei_cl_hbm_equal(tmp, mei_hdr)) {
+			cl_dbg(dev, tmp, "got a message\n");
+			cl = tmp;
 			break;
 		}
 	}

 	/* if no recipient cl was found we assume corrupted header */
-	if (&cl->link == &dev->file_list) {
+	if (!cl) {
 		/* A message for not connected fixed address clients
 		 * should be silently discarded
 		 * On power down client may be force cleaned,
diff --git a/drivers/net/ethernet/qlogic/qede/qede_filter.c b/drivers/net/ethernet/qlogic/qede/qede_filter.c
index 3010833ddde3..d3e59ee13705 100644
--- a/drivers/net/ethernet/qlogic/qede/qede_filter.c
+++ b/drivers/net/ethernet/qlogic/qede/qede_filter.c
@@ -829,18 +829,21 @@ int qede_configure_vlan_filters(struct qede_dev *edev)
 int qede_vlan_rx_kill_vid(struct net_device *dev, __be16 proto, u16 vid)
 {
 	struct qede_dev *edev = netdev_priv(dev);
-	struct qede_vlan *vlan;
+	struct qede_vlan *vlan = NULL;
+	struct qede_vlan *tmp;
 	int rc = 0;

 	DP_VERBOSE(edev, NETIF_MSG_IFDOWN, "Removing vlan 0x%04x\n", vid);

 	/* Find whether entry exists */
 	__qede_lock(edev);
-	list_for_each_entry(vlan, &edev->vlan_list, list)
-		if (vlan->vid == vid)
+	list_for_each_entry(tmp, &edev->vlan_list, list)
+		if (tmp->vid == vid) {
+			vlan = tmp;
 			break;
+		}

-	if (list_entry_is_head(vlan, &edev->vlan_list, list)) {
+	if (!vlan) {
 		DP_VERBOSE(edev, (NETIF_MSG_IFUP | NETIF_MSG_IFDOWN),
 			   "Vlan isn't configured\n");
 		goto out;
diff --git a/drivers/net/wireless/intel/ipw2x00/libipw_rx.c b/drivers/net/wireless/intel/ipw2x00/libipw_rx.c
index 7a684b76f39b..c78372e0dc15 100644
--- a/drivers/net/wireless/intel/ipw2x00/libipw_rx.c
+++ b/drivers/net/wireless/intel/ipw2x00/libipw_rx.c
@@ -1507,7 +1507,8 @@ static void libipw_process_probe_response(struct libipw_device
 {
 	struct net_device *dev = ieee->dev;
 	struct libipw_network network = { };
-	struct libipw_network *target;
+	struct libipw_network *target = NULL;
+	struct libipw_network *tmp;
 	struct libipw_network *oldest = NULL;
 #ifdef CONFIG_LIBIPW_DEBUG
 	struct libipw_info_element *info_element = beacon->info_element;
@@ -1555,18 +1556,20 @@ static void libipw_process_probe_response(struct libipw_device

 	spin_lock_irqsave(&ieee->lock, flags);

-	list_for_each_entry(target, &ieee->network_list, list) {
-		if (is_same_network(target, &network))
+	list_for_each_entry(tmp, &ieee->network_list, list) {
+		if (is_same_network(tmp, &network)) {
+			target = tmp;
 			break;
+		}

 		if ((oldest == NULL) ||
-		    time_before(target->last_scanned, oldest->last_scanned))
-			oldest = target;
+		    time_before(tmp->last_scanned, oldest->last_scanned))
+			oldest = tmp;
 	}

 	/* If we didn't find a match, then get a new network slot to initialize
 	 * with this beacon's information */
-	if (&target->list == &ieee->network_list) {
+	if (!target) {
 		if (list_empty(&ieee->network_free_list)) {
 			/* If there are no more slots, expire the oldest */
 			list_del(&oldest->list);
diff --git a/drivers/power/supply/cpcap-battery.c b/drivers/power/supply/cpcap-battery.c
index 18e3ff0e15d5..6542ff3eeccc 100644
--- a/drivers/power/supply/cpcap-battery.c
+++ b/drivers/power/supply/cpcap-battery.c
@@ -789,17 +789,20 @@ static irqreturn_t cpcap_battery_irq_thread(int irq, void *data)
 {
 	struct cpcap_battery_ddata *ddata = data;
 	struct cpcap_battery_state_data *latest;
-	struct cpcap_interrupt_desc *d;
+	struct cpcap_interrupt_desc *d = NULL;
+	struct cpcap_interrupt_desc *tmp;

 	if (!atomic_read(&ddata->active))
 		return IRQ_NONE;

-	list_for_each_entry(d, &ddata->irq_list, node) {
-		if (irq == d->irq)
+	list_for_each_entry(tmp, &ddata->irq_list, node) {
+		if (irq == tmp->irq) {
+			d = tmp;
 			break;
+		}
 	}

-	if (list_entry_is_head(d, &ddata->irq_list, node))
+	if (!d)
 		return IRQ_NONE;

 	latest = cpcap_battery_latest(ddata);
diff --git a/drivers/scsi/lpfc/lpfc_bsg.c b/drivers/scsi/lpfc/lpfc_bsg.c
index fdf08cb57207..30174bddf024 100644
--- a/drivers/scsi/lpfc/lpfc_bsg.c
+++ b/drivers/scsi/lpfc/lpfc_bsg.c
@@ -1185,7 +1185,8 @@ lpfc_bsg_hba_set_event(struct bsg_job *job)
 	struct lpfc_hba *phba = vport->phba;
 	struct fc_bsg_request *bsg_request = job->request;
 	struct set_ct_event *event_req;
-	struct lpfc_bsg_event *evt;
+	struct lpfc_bsg_event *evt = NULL;
+	struct lpfc_bsg_event *tmp;
 	int rc = 0;
 	struct bsg_job_data *dd_data = NULL;
 	uint32_t ev_mask;
@@ -1205,17 +1206,18 @@ lpfc_bsg_hba_set_event(struct bsg_job *job)
 	ev_mask = ((uint32_t)(unsigned long)event_req->type_mask &
 				FC_REG_EVENT_MASK);
 	spin_lock_irqsave(&phba->ct_ev_lock, flags);
-	list_for_each_entry(evt, &phba->ct_ev_waiters, node) {
-		if (evt->reg_id == event_req->ev_reg_id) {
-			lpfc_bsg_event_ref(evt);
-			evt->wait_time_stamp = jiffies;
-			dd_data = (struct bsg_job_data *)evt->dd_data;
+	list_for_each_entry(tmp, &phba->ct_ev_waiters, node) {
+		if (tmp->reg_id == event_req->ev_reg_id) {
+			lpfc_bsg_event_ref(tmp);
+			tmp->wait_time_stamp = jiffies;
+			dd_data = (struct bsg_job_data *)tmp->dd_data;
+			evt = tmp;
 			break;
 		}
 	}
 	spin_unlock_irqrestore(&phba->ct_ev_lock, flags);

-	if (&evt->node == &phba->ct_ev_waiters) {
+	if (!evt) {
 		/* no event waiting struct yet - first call */
 		dd_data = kmalloc(sizeof(struct bsg_job_data), GFP_KERNEL);
 		if (dd_data == NULL) {
diff --git a/drivers/staging/rtl8192e/rtl819x_TSProc.c b/drivers/staging/rtl8192e/rtl819x_TSProc.c
index 34b00a76b6bd..7ed60edc5aa8 100644
--- a/drivers/staging/rtl8192e/rtl819x_TSProc.c
+++ b/drivers/staging/rtl8192e/rtl819x_TSProc.c
@@ -213,6 +213,7 @@ static struct ts_common_info *SearchAdmitTRStream(struct rtllib_device *ieee,
 	bool	search_dir[4] = {0};
 	struct list_head *psearch_list;
 	struct ts_common_info *pRet = NULL;
+	struct ts_common_info *tmp;

 	if (ieee->iw_mode == IW_MODE_MASTER) {
 		if (TxRxSelect == TX_DIR) {
@@ -247,19 +248,19 @@ static struct ts_common_info *SearchAdmitTRStream(struct rtllib_device *ieee,
 	for (dir = 0; dir <= DIR_BI_DIR; dir++) {
 		if (!search_dir[dir])
 			continue;
-		list_for_each_entry(pRet, psearch_list, List) {
-			if (memcmp(pRet->Addr, Addr, 6) == 0 &&
-			    pRet->TSpec.f.TSInfo.field.ucTSID == TID &&
-			    pRet->TSpec.f.TSInfo.field.ucDirection == dir)
+		list_for_each_entry(tmp, psearch_list, List) {
+			if (memcmp(tmp->Addr, Addr, 6) == 0 &&
+			    tmp->TSpec.f.TSInfo.field.ucTSID == TID &&
+			    tmp->TSpec.f.TSInfo.field.ucDirection == dir) {
+				pRet = tmp;
 				break;
+			}
 		}
-		if (&pRet->List  != psearch_list)
+		if (pRet)
 			break;
 	}

-	if (pRet && &pRet->List  != psearch_list)
-		return pRet;
-	return NULL;
+	return pRet;
 }

 static void MakeTSEntry(struct ts_common_info *pTsCommonInfo, u8 *Addr,
diff --git a/drivers/staging/rtl8192e/rtllib_rx.c b/drivers/staging/rtl8192e/rtllib_rx.c
index e3d0a361d370..5f44bc5dcd76 100644
--- a/drivers/staging/rtl8192e/rtllib_rx.c
+++ b/drivers/staging/rtl8192e/rtllib_rx.c
@@ -2540,7 +2540,8 @@ static inline void rtllib_process_probe_response(
 	struct rtllib_probe_response *beacon,
 	struct rtllib_rx_stats *stats)
 {
-	struct rtllib_network *target;
+	struct rtllib_network *target = NULL;
+	struct rtllib_network *tmp;
 	struct rtllib_network *oldest = NULL;
 	struct rtllib_info_element *info_element = &beacon->info_element[0];
 	unsigned long flags;
@@ -2623,19 +2624,21 @@ static inline void rtllib_process_probe_response(
 				ieee->LinkDetectInfo.NumRecvBcnInPeriod++;
 		}
 	}
-	list_for_each_entry(target, &ieee->network_list, list) {
-		if (is_same_network(target, network,
-		   (target->ssid_len ? 1 : 0)))
+	list_for_each_entry(tmp, &ieee->network_list, list) {
+		if (is_same_network(tmp, network,
+		   (tmp->ssid_len ? 1 : 0))) {
+			target = tmp;
 			break;
+		}
 		if ((oldest == NULL) ||
-		    (target->last_scanned < oldest->last_scanned))
-			oldest = target;
+		    (tmp->last_scanned < oldest->last_scanned))
+			oldest = tmp;
 	}

 	/* If we didn't find a match, then get a new network slot to initialize
 	 * with this beacon's information
 	 */
-	if (&target->list == &ieee->network_list) {
+	if (!target) {
 		if (list_empty(&ieee->network_free_list)) {
 			/* If there are no more slots, expire the oldest */
 			list_del(&oldest->list);
diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211_rx.c b/drivers/staging/rtl8192u/ieee80211/ieee80211_rx.c
index b58e75932ecd..2843c1c1c2f2 100644
--- a/drivers/staging/rtl8192u/ieee80211/ieee80211_rx.c
+++ b/drivers/staging/rtl8192u/ieee80211/ieee80211_rx.c
@@ -2239,7 +2239,8 @@ static inline void ieee80211_process_probe_response(
 	struct ieee80211_rx_stats *stats)
 {
 	struct ieee80211_network *network;
-	struct ieee80211_network *target;
+	struct ieee80211_network *target = NULL;
+	struct ieee80211_network *tmp;
 	struct ieee80211_network *oldest = NULL;
 #ifdef CONFIG_IEEE80211_DEBUG
 	struct ieee80211_info_element *info_element = &beacon->info_element[0];
@@ -2357,17 +2358,19 @@ static inline void ieee80211_process_probe_response(
 			network->flags = (~NETWORK_EMPTY_ESSID & network->flags) | (NETWORK_EMPTY_ESSID & ieee->current_network.flags);
 	}

-	list_for_each_entry(target, &ieee->network_list, list) {
-		if (is_same_network(target, network, ieee))
+	list_for_each_entry(tmp, &ieee->network_list, list) {
+		if (is_same_network(tmp, network, ieee)) {
+			target = tmp;
 			break;
+		}
 		if (!oldest ||
-		    (target->last_scanned < oldest->last_scanned))
-			oldest = target;
+		    (tmp->last_scanned < oldest->last_scanned))
+			oldest = tmp;
 	}

 	/* If we didn't find a match, then get a new network slot to initialize
 	 * with this beacon's information */
-	if (&target->list == &ieee->network_list) {
+	if (!target) {
 		if (list_empty(&ieee->network_free_list)) {
 			/* If there are no more slots, expire the oldest */
 			list_del(&oldest->list);
diff --git a/drivers/staging/rtl8192u/ieee80211/rtl819x_TSProc.c b/drivers/staging/rtl8192u/ieee80211/rtl819x_TSProc.c
index 3aabb401b15a..1b8f3fd8e51d 100644
--- a/drivers/staging/rtl8192u/ieee80211/rtl819x_TSProc.c
+++ b/drivers/staging/rtl8192u/ieee80211/rtl819x_TSProc.c
@@ -209,6 +209,7 @@ static struct ts_common_info *SearchAdmitTRStream(struct ieee80211_device *ieee,
 	bool				search_dir[4] = {0};
 	struct list_head		*psearch_list; //FIXME
 	struct ts_common_info	*pRet = NULL;
+	struct ts_common_info	*tmp;
 	if (ieee->iw_mode == IW_MODE_MASTER) { //ap mode
 		if (TxRxSelect == TX_DIR) {
 			search_dir[DIR_DOWN] = true;
@@ -243,23 +244,21 @@ static struct ts_common_info *SearchAdmitTRStream(struct ieee80211_device *ieee,
 	for (dir = 0; dir <= DIR_BI_DIR; dir++) {
 		if (!search_dir[dir])
 			continue;
-		list_for_each_entry(pRet, psearch_list, list) {
-	//		IEEE80211_DEBUG(IEEE80211_DL_TS, "ADD:%pM, TID:%d, dir:%d\n", pRet->Addr, pRet->TSpec.ts_info.ucTSID, pRet->TSpec.ts_info.ucDirection);
-			if (memcmp(pRet->addr, Addr, 6) == 0)
-				if (pRet->t_spec.ts_info.uc_tsid == TID)
-					if (pRet->t_spec.ts_info.uc_direction == dir) {
+		list_for_each_entry(tmp, psearch_list, list) {
+	//		IEEE80211_DEBUG(IEEE80211_DL_TS, "ADD:%pM, TID:%d, dir:%d\n", tmp->Addr, tmp->TSpec.ts_info.ucTSID, tmp->TSpec.ts_info.ucDirection);
+			if (memcmp(tmp->addr, Addr, 6) == 0)
+				if (tmp->t_spec.ts_info.uc_tsid == TID)
+					if (tmp->t_spec.ts_info.uc_direction == dir) {
 	//					printk("Bingo! got it\n");
+	//					pRet = tmp;
 						break;
 					}
 		}
-		if (&pRet->list  != psearch_list)
+		if (pRet)
 			break;
 	}

-	if (&pRet->list  != psearch_list)
-		return pRet;
-	else
-		return NULL;
+	return pRet;
 }

 static void MakeTSEntry(struct ts_common_info *pTsCommonInfo, u8 *Addr,
diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index 9315313108c9..26908d012ac8 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -1690,6 +1690,7 @@ composite_setup(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
 	u16				w_value = le16_to_cpu(ctrl->wValue);
 	u16				w_length = le16_to_cpu(ctrl->wLength);
 	struct usb_function		*f = NULL;
+	struct usb_function		*tmp;
 	u8				endp;

 	if (w_length > USB_COMP_EP0_BUFSIZ) {
@@ -2046,12 +2047,12 @@ composite_setup(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
 			if (!cdev->config)
 				break;
 			endp = ((w_index & 0x80) >> 3) | (w_index & 0x0f);
-			list_for_each_entry(f, &cdev->config->functions, list) {
-				if (test_bit(endp, f->endpoints))
+			list_for_each_entry(tmp, &cdev->config->functions, list) {
+				if (test_bit(endp, tmp->endpoints)) {
+					f = tmp;
 					break;
+				}
 			}
-			if (&f->list == &cdev->config->functions)
-				f = NULL;
 			break;
 		}
 try_fun_setup:
diff --git a/fs/cifs/smb2misc.c b/fs/cifs/smb2misc.c
index b25623e3fe3d..e012a2bdab82 100644
--- a/fs/cifs/smb2misc.c
+++ b/fs/cifs/smb2misc.c
@@ -150,16 +150,18 @@ smb2_check_message(char *buf, unsigned int len, struct TCP_Server_Info *srvr)
 		struct smb2_transform_hdr *thdr =
 			(struct smb2_transform_hdr *)buf;
 		struct cifs_ses *ses = NULL;
+		struct cifs_ses *tmp;

 		/* decrypt frame now that it is completely read in */
 		spin_lock(&cifs_tcp_ses_lock);
-		list_for_each_entry(ses, &srvr->smb_ses_list, smb_ses_list) {
-			if (ses->Suid == le64_to_cpu(thdr->SessionId))
+		list_for_each_entry(tmp, &srvr->smb_ses_list, smb_ses_list) {
+			if (tmp->Suid == le64_to_cpu(thdr->SessionId)) {
+				ses = tmp;
 				break;
+			}
 		}
 		spin_unlock(&cifs_tcp_ses_lock);
-		if (list_entry_is_head(ses, &srvr->smb_ses_list,
-				       smb_ses_list)) {
+		if (!ses) {
 			cifs_dbg(VFS, "no decryption - session id not found\n");
 			return 1;
 		}
diff --git a/fs/proc/kcore.c b/fs/proc/kcore.c
index 982e694aae77..f1ac6d765367 100644
--- a/fs/proc/kcore.c
+++ b/fs/proc/kcore.c
@@ -316,6 +316,7 @@ read_kcore(struct file *file, char __user *buffer, size_t buflen, loff_t *fpos)
 	size_t page_offline_frozen = 1;
 	size_t phdrs_len, notes_len;
 	struct kcore_list *m;
+	struct kcore_list *tmp;
 	size_t tsz;
 	int nphdr;
 	unsigned long start;
@@ -479,10 +480,13 @@ read_kcore(struct file *file, char __user *buffer, size_t buflen, loff_t *fpos)
 		 * the previous entry, search for a matching entry.
 		 */
 		if (!m || start < m->addr || start >= m->addr + m->size) {
-			list_for_each_entry(m, &kclist_head, list) {
-				if (start >= m->addr &&
-				    start < m->addr + m->size)
+			m = NULL;
+			list_for_each_entry(tmp, &kclist_head, list) {
+				if (start >= tmp->addr &&
+				    start < tmp->addr + tmp->size) {
+					m = tmp;
 					break;
+				}
 			}
 		}
@@ -492,12 +496,11 @@ read_kcore(struct file *file, char __user *buffer, size_t buflen, loff_t *fpos)
 			page_offline_freeze();
 		}

-		if (&m->list == &kclist_head) {
+		if (!m) {
 			if (clear_user(buffer, tsz)) {
 				ret = -EFAULT;
 				goto out;
 			}
-			m = NULL;	/* skip the list anchor */
 			goto skip;
 		}
diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c
index 0852a537dad4..2d3d6558cef0 100644
--- a/kernel/debug/kdb/kdb_main.c
+++ b/kernel/debug/kdb/kdb_main.c
@@ -781,18 +781,21 @@ static int kdb_defcmd(int argc, const char **argv)
 static int kdb_exec_defcmd(int argc, const char **argv)
 {
 	int ret;
-	kdbtab_t *kp;
+	kdbtab_t *kp = NULL;
+	kdbtab_t *tmp;
 	struct kdb_macro *kmp;
 	struct kdb_macro_statement *kms;

 	if (argc != 0)
 		return KDB_ARGCOUNT;

-	list_for_each_entry(kp, &kdb_cmds_head, list_node) {
-		if (strcmp(kp->name, argv[0]) == 0)
+	list_for_each_entry(tmp, &kdb_cmds_head, list_node) {
+		if (strcmp(tmp->name, argv[0]) == 0) {
+			kp = tmp;
 			break;
+		}
 	}
-	if (list_entry_is_head(kp, &kdb_cmds_head, list_node)) {
+	if (!kp) {
 		kdb_printf("kdb_exec_defcmd: could not find commands for %s\n",
 			   argv[0]);
 		return KDB_NOTIMP;
@@ -919,7 +922,8 @@ int kdb_parse(const char *cmdstr)
 	static char cbuf[CMD_BUFLEN+2];
 	char *cp;
 	char *cpp, quoted;
-	kdbtab_t *tp;
+	kdbtab_t *tp = NULL;
+	kdbtab_t *tmp;
 	int escaped, ignore_errors = 0, check_grep = 0;

 	/*
@@ -1010,17 +1014,21 @@ int kdb_parse(const char *cmdstr)
 		++argv[0];
 	}

-	list_for_each_entry(tp, &kdb_cmds_head, list_node) {
+	list_for_each_entry(tmp, &kdb_cmds_head, list_node) {
 		/*
 		 * If this command is allowed to be abbreviated,
 		 * check to see if this is it.
 		 */
-		if (tp->minlen && (strlen(argv[0]) <= tp->minlen) &&
-		    (strncmp(argv[0], tp->name, tp->minlen) == 0))
+		if (tmp->minlen && (strlen(argv[0]) <= tmp->minlen) &&
+		    (strncmp(argv[0], tmp->name, tmp->minlen) == 0)) {
+			tp = tmp;
 			break;
+		}

-		if (strcmp(argv[0], tp->name) == 0)
+		if (strcmp(argv[0], tmp->name) == 0) {
+			tp = tmp;
 			break;
+		}
 	}

 	/*
@@ -1028,14 +1036,16 @@ int kdb_parse(const char *cmdstr)
 	 * few characters of this match any of the known commands.
 	 * e.g., md1c20 should match md.
 	 */
-	if (list_entry_is_head(tp, &kdb_cmds_head, list_node)) {
-		list_for_each_entry(tp, &kdb_cmds_head, list_node) {
-			if (strncmp(argv[0], tp->name, strlen(tp->name)) == 0)
+	if (!tp) {
+		list_for_each_entry(tmp, &kdb_cmds_head, list_node) {
+			if (strncmp(argv[0], tmp->name, strlen(tmp->name)) == 0) {
+				tp = tmp;
 				break;
+			}
 		}
 	}

-	if (!list_entry_is_head(tp, &kdb_cmds_head, list_node)) {
+	if (tp) {
 		int result;

 		if (!kdb_check_flags(tp->flags, kdb_cmd_enabled, argc <= 1))
diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c
index 330d49937692..6ded22451c73 100644
--- a/kernel/power/snapshot.c
+++ b/kernel/power/snapshot.c
@@ -625,16 +625,18 @@ static int create_mem_extents(struct list_head *list, gfp_t gfp_mask)

 	for_each_populated_zone(zone) {
 		unsigned long zone_start, zone_end;
-		struct mem_extent *ext, *cur, *aux;
+		struct mem_extent *ext = NULL, *tmp, *cur, *aux;

 		zone_start = zone->zone_start_pfn;
 		zone_end = zone_end_pfn(zone);

-		list_for_each_entry(ext, list, hook)
-			if (zone_start <= ext->end)
+		list_for_each_entry(tmp, list, hook)
+			if (zone_start <= tmp->end) {
+				ext = tmp;
 				break;
+			}

-		if (&ext->hook == list || zone_end < ext->start) {
+		if (!ext || zone_end < ext->start) {
 			/* New extent is necessary */
 			struct mem_extent *new_ext;
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index f9feb197b2da..d1cc4dcf1b1e 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -4544,7 +4544,8 @@ register_ftrace_function_probe(char *glob, struct trace_array *tr,
 			       void *data)
 {
 	struct ftrace_func_entry *entry;
-	struct ftrace_func_probe *probe;
+	struct ftrace_func_probe *probe = NULL;
+	struct ftrace_func_probe *tmp;
 	struct ftrace_hash **orig_hash;
 	struct ftrace_hash *old_hash;
 	struct ftrace_hash *hash;
@@ -4563,11 +4564,13 @@ register_ftrace_function_probe(char *glob, struct trace_array *tr,

 	mutex_lock(&ftrace_lock);
 	/* Check if the probe_ops is already registered */
-	list_for_each_entry(probe, &tr->func_probes, list) {
-		if (probe->probe_ops == probe_ops)
+	list_for_each_entry(tmp, &tr->func_probes, list) {
+		if (tmp->probe_ops == probe_ops) {
+			probe = tmp;
 			break;
+		}
 	}
-	if (&probe->list == &tr->func_probes) {
+	if (!probe) {
 		probe = kzalloc(sizeof(*probe), GFP_KERNEL);
 		if (!probe) {
 			mutex_unlock(&ftrace_lock);
@@ -4687,7 +4690,8 @@ unregister_ftrace_function_probe_func(char *glob, struct trace_array *tr,
 {
 	struct ftrace_ops_hash old_hash_ops;
 	struct ftrace_func_entry *entry;
-	struct ftrace_func_probe *probe;
+	struct ftrace_func_probe *probe = NULL;
+	struct ftrace_func_probe *iter;
 	struct ftrace_glob func_g;
 	struct ftrace_hash **orig_hash;
 	struct ftrace_hash *old_hash;
@@ -4715,11 +4719,13 @@ unregister_ftrace_function_probe_func(char *glob, struct trace_array *tr,

 	mutex_lock(&ftrace_lock);
 	/* Check if the probe_ops is already registered */
-	list_for_each_entry(probe, &tr->func_probes, list) {
-		if (probe->probe_ops == probe_ops)
+	list_for_each_entry(iter, &tr->func_probes, list) {
+		if (iter->probe_ops == probe_ops) {
+			probe = iter;
 			break;
+		}
 	}
-	if (&probe->list == &tr->func_probes)
+	if (!probe)
 		goto err_unlock_ftrace;

 	ret = -EINVAL;
diff --git a/kernel/trace/trace_eprobe.c b/kernel/trace/trace_eprobe.c
index 191db32dec46..4d9143bc38c8 100644
--- a/kernel/trace/trace_eprobe.c
+++ b/kernel/trace/trace_eprobe.c
@@ -652,7 +652,8 @@ static struct trace_event_functions eprobe_funcs = {
 static int disable_eprobe(struct trace_eprobe *ep,
 			  struct trace_array *tr)
 {
-	struct event_trigger_data *trigger;
+	struct event_trigger_data *trigger = NULL;
+	struct event_trigger_data *tmp;
 	struct trace_event_file *file;
 	struct eprobe_data *edata;
@@ -660,14 +661,16 @@ static int disable_eprobe(struct trace_eprobe *ep,
 	if (!file)
 		return -ENOENT;

-	list_for_each_entry(trigger, &file->triggers, list) {
-		if (!(trigger->flags & EVENT_TRIGGER_FL_PROBE))
+	list_for_each_entry(tmp, &file->triggers, list) {
+		if (!(tmp->flags & EVENT_TRIGGER_FL_PROBE))
 			continue;
-		edata = trigger->private_data;
-		if (edata->ep == ep)
+		edata = tmp->private_data;
+		if (edata->ep == ep) {
+			trigger = tmp;
 			break;
+		}
 	}
-	if (list_entry_is_head(trigger, &file->triggers, list))
+	if (!trigger)
 		return -ENODEV;

 	list_del_rcu(&trigger->list);
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 3147614c1812..6c0642ea42da 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -2264,6 +2264,7 @@ event_subsystem_dir(struct trace_array *tr, const char *name,
 {
 	struct trace_subsystem_dir *dir;
 	struct event_subsystem *system;
+	struct event_subsystem *tmp;
 	struct dentry *entry;

 	/* First see if we did not already create this dir */
@@ -2277,13 +2278,13 @@ event_subsystem_dir(struct trace_array *tr, const char *name,
 	}

 	/* Now see if the system itself exists. */
-	list_for_each_entry(system, &event_subsystems, list) {
-		if (strcmp(system->name, name) == 0)
+	system = NULL;
+	list_for_each_entry(tmp, &event_subsystems, list) {
+		if (strcmp(tmp->name, name) == 0) {
+			system = tmp;
 			break;
+		}
 	}
-	/* Reset system variable when not found */
-	if (&system->list == &event_subsystems)
-		system = NULL;

 	dir = kmalloc(sizeof(*dir), GFP_KERNEL);
 	if (!dir)
diff --git a/net/9p/trans_xen.c b/net/9p/trans_xen.c
index eb9fb55280ef..a1222f9bdda3 100644
--- a/net/9p/trans_xen.c
+++ b/net/9p/trans_xen.c
@@ -115,7 +115,8 @@ static bool p9_xen_write_todo(struct xen_9pfs_dataring *ring, RING_IDX size)

 static int p9_xen_request(struct p9_client *client, struct p9_req_t *p9_req)
 {
-	struct xen_9pfs_front_priv *priv;
+	struct xen_9pfs_front_priv *priv = NULL;
+	struct xen_9pfs_front_priv *tmp;
 	RING_IDX cons, prod, masked_cons, masked_prod;
 	unsigned long flags;
 	u32 size = p9_req->tc.size;
@@ -123,12 +124,14 @@ static int p9_xen_request(struct p9_client *client, struct p9_req_t *p9_req)
 	int num;

 	read_lock(&xen_9pfs_lock);
-	list_for_each_entry(priv, &xen_9pfs_devs, list) {
-		if (priv->client == client)
+	list_for_each_entry(tmp, &xen_9pfs_devs, list) {
+		if (tmp->client == client) {
+			priv = tmp;
 			break;
+		}
 	}
 	read_unlock(&xen_9pfs_lock);
-	if (list_entry_is_head(priv, &xen_9pfs_devs, list))
+	if (!priv)
 		return -EINVAL;

 	num = p9_req->tc.tag % priv->num_rings;
diff --git a/net/ipv4/udp_tunnel_nic.c b/net/ipv4/udp_tunnel_nic.c
index bc3a043a5d5c..981d1c5970c5 100644
--- a/net/ipv4/udp_tunnel_nic.c
+++ b/net/ipv4/udp_tunnel_nic.c
@@ -841,12 +841,14 @@ udp_tunnel_nic_unregister(struct net_device *dev, struct udp_tunnel_nic *utn)
 	 * and if there are other devices just detach.
 	 */
 	if (info->shared) {
-		struct udp_tunnel_nic_shared_node *node, *first;
+		struct udp_tunnel_nic_shared_node *node = NULL, *first, *tmp;

-		list_for_each_entry(node, &info->shared->devices, list)
-			if (node->dev == dev)
+		list_for_each_entry(tmp, &info->shared->devices, list)
+			if (tmp->dev == dev) {
+				node = tmp;
 				break;
-		if (list_entry_is_head(node, &info->shared->devices, list))
+			}
+		if (!node)
 			return;

 		list_del(&node->list);
diff --git a/net/tipc/name_table.c b/net/tipc/name_table.c
index 1d8ba233d047..760a55f572b5 100644
--- a/net/tipc/name_table.c
+++ b/net/tipc/name_table.c
@@ -958,16 +958,19 @@ static int __tipc_nl_add_nametable_publ(struct tipc_nl_msg *msg,
 					struct service_range *sr,
 					u32 *last_key)
 {
-	struct publication *p;
+	struct publication *p = NULL;
+	struct publication *tmp;
 	struct nlattr *attrs;
 	struct nlattr *b;
 	void *hdr;

 	if (*last_key) {
-		list_for_each_entry(p, &sr->all_publ, all_publ)
-			if (p->key == *last_key)
+		list_for_each_entry(tmp, &sr->all_publ, all_publ)
+			if (tmp->key == *last_key) {
+				p = tmp;
 				break;
-		if (list_entry_is_head(p, &sr->all_publ, all_publ))
+			}
+		if (!p)
 			return -EPIPE;
 	} else {
 		p = list_first_entry(&sr->all_publ,
diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index 7545321c3440..60f12a8ed4d4 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -3742,14 +3742,17 @@ static int __tipc_nl_list_sk_publ(struct sk_buff *skb,
 				  struct tipc_sock *tsk, u32 *last_publ)
 {
 	int err;
-	struct publication *p;
+	struct publication *p = NULL;
+	struct publication *tmp;

 	if (*last_publ) {
-		list_for_each_entry(p, &tsk->publications, binding_sock) {
-			if (p->key == *last_publ)
+		list_for_each_entry(tmp, &tsk->publications, binding_sock) {
+			if (tmp->key == *last_publ) {
+				p = tmp;
 				break;
+			}
 		}
-		if (list_entry_is_head(p, &tsk->publications, binding_sock)) {
+		if (!p) {
 			/* We never set seq or call nl_dump_check_consistent()
 			 * this means that setting prev_seq here will cause the
 			 * consistence check to fail in the netlink callback
diff --git a/net/xfrm/xfrm_ipcomp.c b/net/xfrm/xfrm_ipcomp.c
index cb40ff0ff28d..03a10bff89c5 100644
--- a/net/xfrm/xfrm_ipcomp.c
+++ b/net/xfrm/xfrm_ipcomp.c
@@ -233,15 +233,18 @@ static void * __percpu *ipcomp_alloc_scratches(void)

 static void ipcomp_free_tfms(struct crypto_comp * __percpu *tfms)
 {
-	struct ipcomp_tfms *pos;
+	struct ipcomp_tfms *pos = NULL;
+	struct ipcomp_tfms *tmp;
 	int cpu;

-	list_for_each_entry(pos, &ipcomp_tfms_list, list) {
-		if (pos->tfms == tfms)
+	list_for_each_entry(tmp, &ipcomp_tfms_list, list) {
+		if (tmp->tfms == tfms) {
+			pos = tmp;
 			break;
+		}
 	}

-	WARN_ON(list_entry_is_head(pos, &ipcomp_tfms_list, list));
+	WARN_ON(!pos);

 	if (--pos->users)
 		return;
diff --git a/sound/soc/intel/catpt/pcm.c b/sound/soc/intel/catpt/pcm.c
index 939a9b801dec..e030c59468bb 100644
--- a/sound/soc/intel/catpt/pcm.c
+++ b/sound/soc/intel/catpt/pcm.c
@@ -330,7 +330,8 @@ static int catpt_dai_apply_usettings(struct snd_soc_dai *dai,
 				     struct catpt_stream_runtime *stream)
 {
 	struct snd_soc_component *component = dai->component;
-	struct snd_kcontrol *pos;
+	struct snd_kcontrol *pos = NULL;
+	struct snd_kcontrol *tmp;
 	struct catpt_dev *cdev = dev_get_drvdata(dai->dev);
 	const char *name;
 	int ret;
@@ -354,12 +355,14 @@ static int catpt_dai_apply_usettings(struct snd_soc_dai *dai,
 		return 0;
 	}

-	list_for_each_entry(pos, &component->card->snd_card->controls, list) {
-		if (pos->private_data == component &&
-		    !strncmp(name, pos->id.name, sizeof(pos->id.name)))
+	list_for_each_entry(tmp, &component->card->snd_card->controls, list) {
+		if (tmp->private_data == component &&
+		    !strncmp(name, tmp->id.name, sizeof(tmp->id.name))) {
+			pos = tmp;
 			break;
+		}
 	}
-	if (list_entry_is_head(pos, &component->card->snd_card->controls, list))
+	if (!pos)
 		return -ENOENT;

 	if (stream->template->type != CATPT_STRM_TYPE_LOOPBACK)
diff --git a/sound/soc/sprd/sprd-mcdt.c b/sound/soc/sprd/sprd-mcdt.c
index f6a55fa60c1b..f37d503e4950 100644
--- a/sound/soc/sprd/sprd-mcdt.c
+++ b/sound/soc/sprd/sprd-mcdt.c
@@ -866,20 +866,19 @@ EXPORT_SYMBOL_GPL(sprd_mcdt_chan_dma_disable);
 struct sprd_mcdt_chan *sprd_mcdt_request_chan(u8 channel,
 					      enum sprd_mcdt_channel_type type)
 {
-	struct sprd_mcdt_chan *temp;
+	struct sprd_mcdt_chan *temp = NULL;
+	struct sprd_mcdt_chan *tmp;

 	mutex_lock(&sprd_mcdt_list_mutex);

-	list_for_each_entry(temp, &sprd_mcdt_chan_list, list) {
-		if (temp->type == type && temp->id == channel) {
-			list_del_init(&temp->list);
+	list_for_each_entry(tmp, &sprd_mcdt_chan_list, list) {
+		if (tmp->type == type && tmp->id == channel) {
+			list_del_init(&tmp->list);
+			temp = tmp;
 			break;
 		}
 	}

-	if (list_entry_is_head(temp, &sprd_mcdt_chan_list, list))
-		temp = NULL;
-
 	mutex_unlock(&sprd_mcdt_list_mutex);

 	return temp;
--
2.25.1

Re: [PATCH 6/6] treewide: remove check of list iterator against head past the loop body

From: Dominique Martinet <asmadeus@codewreck.org>
Date: 2022-02-28 11:46:19

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 <asmadeus@codewreck.org> # 9p

-- 
Dominique

Re: [PATCH 6/6] treewide: remove check of list iterator against head past the loop body

From: Dan Carpenter <hidden>
Date: 2022-02-28 13:15:52

On Mon, Feb 28, 2022 at 12:08:22PM +0100, Jakob Koschel wrote:
quoted hunk
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,  &rcd->tid_group_list.list, list) {
-		kern_add_tid_node(flow, rcd, "complete groups", group,
-				  group->size);
+	list_for_each_entry(tmp,  &rcd->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 && &group->list == &rcd->tid_group_list.list)
+	if (!group)
 		goto bail_eagain;
 	group = list_prepare_entry(group, &rcd->tid_group_list.list,
 				   list);
regards,
dan carpenter

Re: [PATCH 6/6] treewide: remove check of list iterator against head past the loop body

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: 2022-03-01 20:44:53

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 [off-list ref] wrote:
quoted hunk
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(&sram_lock);

-       list_for_each_entry(info, &sram_bank_list, node)
-               if (!strcmp(pool_name, info->pool_name))
+       list_for_each_entry(tmp, &sram_bank_list, node)
+               if (!strcmp(pool_name, tmp->pool_name)) {
+                       info = tmp;
                        break;
+               }

        mutex_unlock(&sram_lock);

-       if (&info->node == &sram_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(&sram_lock);
        list_for_each_entry(info, &sram_bank_list, node) {
                if (!strcmp(pool_name, info->pool_name)) {
                        mutex_unlock(&sram_lock);
                        return info;
                }
        }
        mutex_unlock(&sram_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: [Intel-gfx] [PATCH 6/6] treewide: remove check of list iterator against head past the loop body

From: Tvrtko Ursulin <hidden>
Date: 2022-03-02 17:15:11

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
&pos->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 <redacted>
[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]
quoted hunk
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, &ctx->handles_vma, &iter, 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(&obj->base.refcount))
  			continue;

  		spin_lock(&obj->lut_lock);
-		list_for_each_entry(lut, &obj->lut_list, obj_link) {
-			if (lut->ctx != ctx)
+		list_for_each_entry(tmp, &obj->lut_list, obj_link) {
+			if (tmp->ctx != ctx)
  				continue;

-			if (lut->handle != iter.index)
+			if (tmp->handle != iter.index)
  				continue;

-			list_del(&lut->obj_link);
+			list_del(&tmp->obj_link);
+			lut = tmp;
  			break;
  		}
  		spin_unlock(&obj->lut_lock);

-		if (&lut->obj_link != &obj->lut_list) {
+		if (lut) {
  			i915_lut_handle_free(lut);
  			radix_tree_iter_delete(&ctx->handles_vma, &iter, 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'.
quoted hunk
  			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, &tl->requests, link) {
-		if (rq->ring != ring)
+	list_for_each_entry(tmp, &tl->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 (&rq->link == &tl->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.
quoted hunk
  	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 i915_request *target = NULL;
+	struct i915_request *tmp;
  	long timeout;

  	if (intel_ring_update_space(ring) >= bytes)
  		return 0;

  	GEM_BUG_ON(list_empty(&tl->requests));
-	list_for_each_entry(target, &tl->requests, link) {
-		if (target->ring != ring)
+	list_for_each_entry(tmp, &tl->requests, link) {
+		if (tmp->ring != ring)
  			continue;

  		/* Would completion of this request free enough space? */
-		if (bytes <= __intel_ring_space(target->postfix,
-						ring->emit, ring->size))
+		if (bytes <= __intel_ring_space(tmp->postfix,
+						ring->emit, ring->size)) {
+			target = tmp;
  			break;
+		}
  	}

-	if (GEM_WARN_ON(&target->link == &tl->requests))
+	if (GEM_WARN_ON(!target))
  		return -ENOSPC;

  	timeout = i915_request_wait(target,
Looks okay as well. Less clear here if there is a clean solution to make 
the diff smaller so no suggestions. I mean do I dare mention "goto 
found;" from inside the loop, where the break is, instead of the 
variable renames.. risky.. :) (And ofc "return -ENOSPC" immediately 
after the loop.)

As a summary changes looks okay, up to you if you want to try to make 
the diffs smaller or not. It doesn't matter hugely really, all I have is 
a vague and uncertain "maybe it makes backporting of something, someday 
easier". So for i915 it is good either way.

Reviewed-by: Tvrtko Ursulin <redacted> # i915 bits only

Regards,

Tvrtko

[PATCH 5/6] treewide: remove dereference of list iterator after loop body

From: Jakob Koschel <hidden>
Date: 2022-02-28 11:09:53

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 <redacted>
---
 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

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

From: Jakob Koschel <hidden>
Date: 2022-02-28 11:09:54

If the list does not contain the expected element, the value of
list_for_each_entry() iterator will not point to a valid structure.
To avoid type confusion in such case, the list iterator
scope will be limited to list_for_each_entry() loop.

In preparation to limiting scope of a list iterator to the list traversal
loop, use a dedicated pointer to point to the found element.
Determining if an element was found is then simply checking if
the pointer is != NULL.

Signed-off-by: Jakob Koschel <redacted>
---
 arch/x86/kernel/cpu/sgx/encl.c       |  6 +++--
 drivers/scsi/scsi_transport_sas.c    | 17 ++++++++-----
 drivers/thermal/thermal_core.c       | 38 ++++++++++++++++++----------
 drivers/usb/gadget/configfs.c        | 22 ++++++++++------
 drivers/usb/gadget/udc/max3420_udc.c | 11 +++++---
 drivers/usb/gadget/udc/tegra-xudc.c  | 11 +++++---
 drivers/usb/mtu3/mtu3_gadget.c       | 11 +++++---
 drivers/usb/musb/musb_gadget.c       | 11 +++++---
 drivers/vfio/mdev/mdev_core.c        | 11 +++++---
 9 files changed, 88 insertions(+), 50 deletions(-)
diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
index 48afe96ae0f0..6c916416decc 100644
--- a/arch/x86/kernel/cpu/sgx/encl.c
+++ b/arch/x86/kernel/cpu/sgx/encl.c
@@ -450,7 +450,8 @@ static void sgx_mmu_notifier_release(struct mmu_notifier *mn,
 				     struct mm_struct *mm)
 {
 	struct sgx_encl_mm *encl_mm = container_of(mn, struct sgx_encl_mm, mmu_notifier);
-	struct sgx_encl_mm *tmp = NULL;
+	struct sgx_encl_mm *found_encl_mm = NULL;
+	struct sgx_encl_mm *tmp;

 	/*
 	 * The enclave itself can remove encl_mm.  Note, objects can't be moved
@@ -460,12 +461,13 @@ static void sgx_mmu_notifier_release(struct mmu_notifier *mn,
 	list_for_each_entry(tmp, &encl_mm->encl->mm_list, list) {
 		if (tmp == encl_mm) {
 			list_del_rcu(&encl_mm->list);
+			found_encl_mm = tmp;
 			break;
 		}
 	}
 	spin_unlock(&encl_mm->encl->mm_lock);

-	if (tmp == encl_mm) {
+	if (found_encl_mm) {
 		synchronize_srcu(&encl_mm->encl->srcu);
 		mmu_notifier_put(mn);
 	}
diff --git a/drivers/scsi/scsi_transport_sas.c b/drivers/scsi/scsi_transport_sas.c
index 4ee578b181da..a8cbd90db9d2 100644
--- a/drivers/scsi/scsi_transport_sas.c
+++ b/drivers/scsi/scsi_transport_sas.c
@@ -1060,26 +1060,29 @@ EXPORT_SYMBOL(sas_port_get_phy);
  * connected to a remote device is a port, so ports must be formed on
  * all devices with phys if they're connected to anything.
  */
-void sas_port_add_phy(struct sas_port *port, struct sas_phy *phy)
+void sas_port_add_phy(struct sas_port *port, struct sas_phy *_phy)
 {
 	mutex_lock(&port->phy_list_mutex);
-	if (unlikely(!list_empty(&phy->port_siblings))) {
+	if (unlikely(!list_empty(&_phy->port_siblings))) {
 		/* make sure we're already on this port */
+		struct sas_phy *phy = NULL;
 		struct sas_phy *tmp;

 		list_for_each_entry(tmp, &port->phy_list, port_siblings)
-			if (tmp == phy)
+			if (tmp == _phy) {
+				phy = tmp;
 				break;
+			}
 		/* If this trips, you added a phy that was already
 		 * part of a different port */
-		if (unlikely(tmp != phy)) {
+		if (unlikely(!phy)) {
 			dev_printk(KERN_ERR, &port->dev, "trying to add phy %s fails: it's already part of another port\n",
-				   dev_name(&phy->dev));
+				   dev_name(&_phy->dev));
 			BUG();
 		}
 	} else {
-		sas_port_create_link(port, phy);
-		list_add_tail(&phy->port_siblings, &port->phy_list);
+		sas_port_create_link(port, _phy);
+		list_add_tail(&_phy->port_siblings, &port->phy_list);
 		port->num_phys++;
 	}
 	mutex_unlock(&port->phy_list_mutex);
diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index 82654dc8382b..97198543448b 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -625,24 +625,30 @@ int thermal_zone_bind_cooling_device(struct thermal_zone_device *tz,
 {
 	struct thermal_instance *dev;
 	struct thermal_instance *pos;
-	struct thermal_zone_device *pos1;
-	struct thermal_cooling_device *pos2;
+	struct thermal_zone_device *pos1 = NULL;
+	struct thermal_zone_device *tmp1;
+	struct thermal_cooling_device *pos2 = NULL;
+	struct thermal_cooling_device *tmp2;
 	unsigned long max_state;
 	int result, ret;

 	if (trip >= tz->trips || trip < 0)
 		return -EINVAL;

-	list_for_each_entry(pos1, &thermal_tz_list, node) {
-		if (pos1 == tz)
+	list_for_each_entry(tmp1, &thermal_tz_list, node) {
+		if (tmp1 == tz) {
+			pos1 = tmp1;
 			break;
+		}
 	}
-	list_for_each_entry(pos2, &thermal_cdev_list, node) {
-		if (pos2 == cdev)
+	list_for_each_entry(tmp2, &thermal_cdev_list, node) {
+		if (tmp2 == cdev) {
+			pos2 = tmp2;
 			break;
+		}
 	}

-	if (tz != pos1 || cdev != pos2)
+	if (!pos1 || !pos2)
 		return -EINVAL;

 	ret = cdev->ops->get_max_state(cdev, &max_state);
@@ -1074,15 +1080,18 @@ void thermal_cooling_device_unregister(struct thermal_cooling_device *cdev)
 	const struct thermal_zone_params *tzp;
 	struct thermal_zone_device *tz;
 	struct thermal_cooling_device *pos = NULL;
+	struct thermal_cooling_device *tmp;

 	if (!cdev)
 		return;

 	mutex_lock(&thermal_list_lock);
-	list_for_each_entry(pos, &thermal_cdev_list, node)
-		if (pos == cdev)
+	list_for_each_entry(tmp, &thermal_cdev_list, node)
+		if (tmp == cdev) {
+			pos = tmp;
 			break;
-	if (pos != cdev) {
+		}
+	if (!pos) {
 		/* thermal cooling device not found */
 		mutex_unlock(&thermal_list_lock);
 		return;
@@ -1335,6 +1344,7 @@ void thermal_zone_device_unregister(struct thermal_zone_device *tz)
 	const struct thermal_zone_params *tzp;
 	struct thermal_cooling_device *cdev;
 	struct thermal_zone_device *pos = NULL;
+	struct thermal_zone_device *tmp;

 	if (!tz)
 		return;
@@ -1343,10 +1353,12 @@ void thermal_zone_device_unregister(struct thermal_zone_device *tz)
 	tz_id = tz->id;

 	mutex_lock(&thermal_list_lock);
-	list_for_each_entry(pos, &thermal_tz_list, node)
-		if (pos == tz)
+	list_for_each_entry(tmp, &thermal_tz_list, node)
+		if (tmp == tz) {
+			pos = tmp;
 			break;
-	if (pos != tz) {
+		}
+	if (!pos) {
 		/* thermal zone device not found */
 		mutex_unlock(&thermal_list_lock);
 		return;
diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
index d4a678c0806e..99f10cbd8878 100644
--- a/drivers/usb/gadget/configfs.c
+++ b/drivers/usb/gadget/configfs.c
@@ -418,7 +418,8 @@ static int config_usb_cfg_link(

 	struct usb_function_instance *fi =
 			to_usb_function_instance(usb_func_ci);
-	struct usb_function_instance *a_fi;
+	struct usb_function_instance *a_fi = NULL;
+	struct usb_function_instance *tmp;
 	struct usb_function *f;
 	int ret;
@@ -428,11 +429,13 @@ static int config_usb_cfg_link(
 	 * from another gadget or a random directory.
 	 * Also a function instance can only be linked once.
 	 */
-	list_for_each_entry(a_fi, &gi->available_func, cfs_list) {
-		if (a_fi == fi)
+	list_for_each_entry(tmp, &gi->available_func, cfs_list) {
+		if (tmp == fi) {
+			a_fi = tmp;
 			break;
+		}
 	}
-	if (a_fi != fi) {
+	if (!a_fi) {
 		ret = -EINVAL;
 		goto out;
 	}
@@ -882,15 +885,18 @@ static int os_desc_link(struct config_item *os_desc_ci,
 	struct gadget_info *gi = os_desc_item_to_gadget_info(os_desc_ci);
 	struct usb_composite_dev *cdev = &gi->cdev;
 	struct config_usb_cfg *c_target = to_config_usb_cfg(usb_cfg_ci);
-	struct usb_configuration *c;
+	struct usb_configuration *c = NULL;
+	struct usb_configuration *tmp;
 	int ret;

 	mutex_lock(&gi->lock);
-	list_for_each_entry(c, &cdev->configs, list) {
-		if (c == &c_target->c)
+	list_for_each_entry(tmp, &cdev->configs, list) {
+		if (tmp == &c_target->c) {
+			c = tmp;
 			break;
+		}
 	}
-	if (c != &c_target->c) {
+	if (!c) {
 		ret = -EINVAL;
 		goto out;
 	}
diff --git a/drivers/usb/gadget/udc/max3420_udc.c b/drivers/usb/gadget/udc/max3420_udc.c
index d2a2b20cc1ad..d1b010b5f4a0 100644
--- a/drivers/usb/gadget/udc/max3420_udc.c
+++ b/drivers/usb/gadget/udc/max3420_udc.c
@@ -1044,22 +1044,25 @@ static int max3420_ep_queue(struct usb_ep *_ep, struct usb_request *_req,

 static int max3420_ep_dequeue(struct usb_ep *_ep, struct usb_request *_req)
 {
-	struct max3420_req *t, *req = to_max3420_req(_req);
+	struct max3420_req *t = NULL;
+	struct max3420_req *req = to_max3420_req(_req);
+	struct max3420_req *tmp;
 	struct max3420_ep *ep = to_max3420_ep(_ep);
 	unsigned long flags;

 	spin_lock_irqsave(&ep->lock, flags);

 	/* Pluck the descriptor from queue */
-	list_for_each_entry(t, &ep->queue, queue)
-		if (t == req) {
+	list_for_each_entry(tmp, &ep->queue, queue)
+		if (tmp == req) {
 			list_del_init(&req->queue);
+			t = tmp;
 			break;
 		}

 	spin_unlock_irqrestore(&ep->lock, flags);

-	if (t == req)
+	if (t)
 		max3420_req_done(req, -ECONNRESET);

 	return 0;
diff --git a/drivers/usb/gadget/udc/tegra-xudc.c b/drivers/usb/gadget/udc/tegra-xudc.c
index 43f1b0d461c1..c37e3148a208 100644
--- a/drivers/usb/gadget/udc/tegra-xudc.c
+++ b/drivers/usb/gadget/udc/tegra-xudc.c
@@ -1413,18 +1413,21 @@ __tegra_xudc_ep_dequeue(struct tegra_xudc_ep *ep,
 			struct tegra_xudc_request *req)
 {
 	struct tegra_xudc *xudc = ep->xudc;
-	struct tegra_xudc_request *r;
+	struct tegra_xudc_request *r = NULL;
+	struct tegra_xudc_request *tmp;
 	struct tegra_xudc_trb *deq_trb;
 	bool busy, kick_queue = false;
 	int ret = 0;

 	/* Make sure the request is actually queued to this endpoint. */
-	list_for_each_entry(r, &ep->queue, list) {
-		if (r == req)
+	list_for_each_entry(tmp, &ep->queue, list) {
+		if (tmp == req) {
+			r = tmp;
 			break;
+		}
 	}

-	if (r != req)
+	if (!r)
 		return -EINVAL;

 	/* Request hasn't been queued in the transfer ring yet. */
diff --git a/drivers/usb/mtu3/mtu3_gadget.c b/drivers/usb/mtu3/mtu3_gadget.c
index 9977600616d7..2e4daaa081a0 100644
--- a/drivers/usb/mtu3/mtu3_gadget.c
+++ b/drivers/usb/mtu3/mtu3_gadget.c
@@ -323,7 +323,8 @@ static int mtu3_gadget_dequeue(struct usb_ep *ep, struct usb_request *req)
 {
 	struct mtu3_ep *mep = to_mtu3_ep(ep);
 	struct mtu3_request *mreq = to_mtu3_request(req);
-	struct mtu3_request *r;
+	struct mtu3_request *r = NULL;
+	struct mtu3_request *tmp;
 	struct mtu3 *mtu = mep->mtu;
 	unsigned long flags;
 	int ret = 0;
@@ -336,11 +337,13 @@ static int mtu3_gadget_dequeue(struct usb_ep *ep, struct usb_request *req)

 	spin_lock_irqsave(&mtu->lock, flags);

-	list_for_each_entry(r, &mep->req_list, list) {
-		if (r == mreq)
+	list_for_each_entry(tmp, &mep->req_list, list) {
+		if (tmp == mreq) {
+			r = tmp;
 			break;
+		}
 	}
-	if (r != mreq) {
+	if (!r) {
 		dev_dbg(mtu->dev, "req=%p not queued to %s\n", req, ep->name);
 		ret = -EINVAL;
 		goto done;
diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
index 51274b87f46c..26b61ad7ab1b 100644
--- a/drivers/usb/musb/musb_gadget.c
+++ b/drivers/usb/musb/musb_gadget.c
@@ -1266,7 +1266,8 @@ static int musb_gadget_dequeue(struct usb_ep *ep, struct usb_request *request)
 {
 	struct musb_ep		*musb_ep = to_musb_ep(ep);
 	struct musb_request	*req = to_musb_request(request);
-	struct musb_request	*r;
+	struct musb_request	*r = NULL;
+	struct musb_request	*tmp;
 	unsigned long		flags;
 	int			status = 0;
 	struct musb		*musb = musb_ep->musb;
@@ -1278,11 +1279,13 @@ static int musb_gadget_dequeue(struct usb_ep *ep, struct usb_request *request)

 	spin_lock_irqsave(&musb->lock, flags);

-	list_for_each_entry(r, &musb_ep->req_list, list) {
-		if (r == req)
+	list_for_each_entry(tmp, &musb_ep->req_list, list) {
+		if (tmp == req) {
+			r = tmp;
 			break;
+		}
 	}
-	if (r != req) {
+	if (!r) {
 		dev_err(musb->controller, "request %p not queued to %s\n",
 				request, ep->name);
 		status = -EINVAL;
diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
index b314101237fe..52cfa44c24a7 100644
--- a/drivers/vfio/mdev/mdev_core.c
+++ b/drivers/vfio/mdev/mdev_core.c
@@ -337,16 +337,19 @@ int mdev_device_create(struct mdev_type *type, const guid_t *uuid)

 int mdev_device_remove(struct mdev_device *mdev)
 {
-	struct mdev_device *tmp;
+	struct mdev_device *tmp = NULL;
+	struct mdev_device *iter;
 	struct mdev_parent *parent = mdev->type->parent;

 	mutex_lock(&mdev_list_lock);
-	list_for_each_entry(tmp, &mdev_list, next) {
-		if (tmp == mdev)
+	list_for_each_entry(iter, &mdev_list, next) {
+		if (iter == mdev) {
+			tmp = iter;
 			break;
+		}
 	}

-	if (tmp != mdev) {
+	if (!tmp) {
 		mutex_unlock(&mdev_list_lock);
 		return -ENODEV;
 	}
--
2.25.1

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

From: Greg KH <gregkh@linuxfoundation.org>
Date: 2022-02-28 11:20:15

On Mon, Feb 28, 2022 at 12:08:18PM +0100, Jakob Koschel wrote:
If the list does not contain the expected element, the value of
list_for_each_entry() iterator will not point to a valid structure.
To avoid type confusion in such case, the list iterator
scope will be limited to list_for_each_entry() loop.

In preparation to limiting scope of a list iterator to the list traversal
loop, use a dedicated pointer to point to the found element.
Determining if an element was found is then simply checking if
the pointer is != NULL.

Signed-off-by: Jakob Koschel <redacted>
---
 arch/x86/kernel/cpu/sgx/encl.c       |  6 +++--
 drivers/scsi/scsi_transport_sas.c    | 17 ++++++++-----
 drivers/thermal/thermal_core.c       | 38 ++++++++++++++++++----------
 drivers/usb/gadget/configfs.c        | 22 ++++++++++------
 drivers/usb/gadget/udc/max3420_udc.c | 11 +++++---
 drivers/usb/gadget/udc/tegra-xudc.c  | 11 +++++---
 drivers/usb/mtu3/mtu3_gadget.c       | 11 +++++---
 drivers/usb/musb/musb_gadget.c       | 11 +++++---
 drivers/vfio/mdev/mdev_core.c        | 11 +++++---
 9 files changed, 88 insertions(+), 50 deletions(-)
The drivers/usb/ portion of this patch should be in patch 1/X, right?

Also, you will have to split these up per-subsystem so that the
different subsystem maintainers can take these in their trees.

thanks,

greg k-h

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

From: Jakob Koschel <hidden>
Date: 2022-02-28 12:07:06

On 28. Feb 2022, at 12:20, Greg KH [off-list ref] wrote:

On Mon, Feb 28, 2022 at 12:08:18PM +0100, Jakob Koschel wrote:
quoted
If the list does not contain the expected element, the value of
list_for_each_entry() iterator will not point to a valid structure.
To avoid type confusion in such case, the list iterator
scope will be limited to list_for_each_entry() loop.

In preparation to limiting scope of a list iterator to the list traversal
loop, use a dedicated pointer to point to the found element.
Determining if an element was found is then simply checking if
the pointer is != NULL.

Signed-off-by: Jakob Koschel <redacted>
---
arch/x86/kernel/cpu/sgx/encl.c       |  6 +++--
drivers/scsi/scsi_transport_sas.c    | 17 ++++++++-----
drivers/thermal/thermal_core.c       | 38 ++++++++++++++++++----------
drivers/usb/gadget/configfs.c        | 22 ++++++++++------
drivers/usb/gadget/udc/max3420_udc.c | 11 +++++---
drivers/usb/gadget/udc/tegra-xudc.c  | 11 +++++---
drivers/usb/mtu3/mtu3_gadget.c       | 11 +++++---
drivers/usb/musb/musb_gadget.c       | 11 +++++---
drivers/vfio/mdev/mdev_core.c        | 11 +++++---
9 files changed, 88 insertions(+), 50 deletions(-)
The drivers/usb/ portion of this patch should be in patch 1/X, right?
I kept them separate since it's a slightly different case.
Using 'ptr' instead of '&ptr->other_member'. Regardless, I'll split
this commit up as you mentioned.
Also, you will have to split these up per-subsystem so that the
different subsystem maintainers can take these in their trees.
Thanks for the feedback.
To clarify I understand you correctly:
I should do one patch set per-subsystem with every instance/(file?)
fixed as a separate commit?

If I understand correctly, I'll repost accordingly.
thanks,

greg k-h
thanks,
Jakob Koschel

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

From: Greg KH <gregkh@linuxfoundation.org>
Date: 2022-03-01 17:37:28

On Mon, Feb 28, 2022 at 01:06:57PM +0100, Jakob Koschel wrote:
quoted
On 28. Feb 2022, at 12:20, Greg KH [off-list ref] wrote:

On Mon, Feb 28, 2022 at 12:08:18PM +0100, Jakob Koschel wrote:
quoted
If the list does not contain the expected element, the value of
list_for_each_entry() iterator will not point to a valid structure.
To avoid type confusion in such case, the list iterator
scope will be limited to list_for_each_entry() loop.

In preparation to limiting scope of a list iterator to the list traversal
loop, use a dedicated pointer to point to the found element.
Determining if an element was found is then simply checking if
the pointer is != NULL.

Signed-off-by: Jakob Koschel <redacted>
---
arch/x86/kernel/cpu/sgx/encl.c       |  6 +++--
drivers/scsi/scsi_transport_sas.c    | 17 ++++++++-----
drivers/thermal/thermal_core.c       | 38 ++++++++++++++++++----------
drivers/usb/gadget/configfs.c        | 22 ++++++++++------
drivers/usb/gadget/udc/max3420_udc.c | 11 +++++---
drivers/usb/gadget/udc/tegra-xudc.c  | 11 +++++---
drivers/usb/mtu3/mtu3_gadget.c       | 11 +++++---
drivers/usb/musb/musb_gadget.c       | 11 +++++---
drivers/vfio/mdev/mdev_core.c        | 11 +++++---
9 files changed, 88 insertions(+), 50 deletions(-)
The drivers/usb/ portion of this patch should be in patch 1/X, right?
I kept them separate since it's a slightly different case.
Using 'ptr' instead of '&ptr->other_member'. Regardless, I'll split
this commit up as you mentioned.
quoted
Also, you will have to split these up per-subsystem so that the
different subsystem maintainers can take these in their trees.
Thanks for the feedback.
To clarify I understand you correctly:
I should do one patch set per-subsystem with every instance/(file?)
fixed as a separate commit?
Yes, each file needs a different commit as they are usually all written
or maintained by different people.

As I said in my other response, if you need any help with this, just let
me know, I'm glad to help.

thanks,

greg k-h

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

From: Christian König <christian.koenig@amd.com>
Date: 2022-02-28 12:19:37

Am 28.02.22 um 12:08 schrieb Jakob Koschel:
If the list does not contain the expected element, the value of
list_for_each_entry() iterator will not point to a valid structure.
To avoid type confusion in such case, the list iterator
scope will be limited to list_for_each_entry() loop.
We explicitly have the list_entry_is_head() macro to test after a loop 
if the element pointer points to the head of the list instead of a valid 
list entry.

So at least from my side I absolutely don't think that this is a good idea.
In preparation to limiting scope of a list iterator to the list traversal
loop, use a dedicated pointer to point to the found element.
Determining if an element was found is then simply checking if
the pointer is != NULL.
Since when do we actually want to do this?

Take this code here as an example:
quoted hunk
diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
index 48afe96ae0f0..6c916416decc 100644
--- a/arch/x86/kernel/cpu/sgx/encl.c
+++ b/arch/x86/kernel/cpu/sgx/encl.c
@@ -450,7 +450,8 @@ static void sgx_mmu_notifier_release(struct mmu_notifier *mn,
  				     struct mm_struct *mm)
  {
  	struct sgx_encl_mm *encl_mm = container_of(mn, struct sgx_encl_mm, mmu_notifier);
-	struct sgx_encl_mm *tmp = NULL;
+	struct sgx_encl_mm *found_encl_mm = NULL;
+	struct sgx_encl_mm *tmp;

  	/*
  	 * The enclave itself can remove encl_mm.  Note, objects can't be moved
@@ -460,12 +461,13 @@ static void sgx_mmu_notifier_release(struct mmu_notifier *mn,
  	list_for_each_entry(tmp, &encl_mm->encl->mm_list, list) {
  		if (tmp == encl_mm) {
  			list_del_rcu(&encl_mm->list);
+			found_encl_mm = tmp;
  			break;
  		}
  	}
  	spin_unlock(&encl_mm->encl->mm_lock);

-	if (tmp == encl_mm) {
+	if (found_encl_mm) {
  		synchronize_srcu(&encl_mm->encl->srcu);
  		mmu_notifier_put(mn);
  	}
I don't think that using the extra variable makes the code in any way 
more reliable or easier to read.

Regards,
Christian.

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

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: 2022-02-28 19:56:44

On Mon, Feb 28, 2022 at 4:19 AM Christian König
[off-list ref] wrote:
I don't think that using the extra variable makes the code in any way
more reliable or easier to read.
So I think the next step is to do the attached patch (which requires
that "-std=gnu11" that was discussed in the original thread).

That will guarantee that the 'pos' parameter of list_for_each_entry()
is only updated INSIDE the for_each_list_entry() loop, and can never
point to the (wrongly typed) head entry.

And I would actually hope that it should actually cause compiler
warnings about possibly uninitialized variables if people then use the
'pos' pointer outside the loop. Except

 (a) that code in sgx/encl.c currently initializes 'tmp' to NULL for
inexplicable reasons - possibly because it already expected this
behavior

 (b) when I remove that NULL initializer, I still don't get a warning,
because we've disabled -Wno-maybe-uninitialized since it results in so
many false positives.

Oh well.

Anyway, give this patch a look, and at least if it's expanded to do
"(pos) = NULL" in the entry statement for the for-loop, it will avoid
the HEAD type confusion that Jakob is working on. And I think in a
cleaner way than the horrid games he plays.

(But it won't avoid possible CPU speculation of such type confusion.
That, in my opinion, is a completely different issue)

I do wish we could actually poison the 'pos' value after the loop
somehow - but clearly the "might be uninitialized" I was hoping for
isn't the way to do it.

Anybody have any ideas?

                Linus

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

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: 2022-02-28 20:04:14

On Mon, Feb 28, 2022 at 11:56 AM Linus Torvalds
[off-list ref] wrote:
I do wish we could actually poison the 'pos' value after the loop
somehow - but clearly the "might be uninitialized" I was hoping for
isn't the way to do it.
Side note: we do need *some* way to do it.

Because if we make that change, and only set it to another pointer
when not the head, then we very much change the semantics of
"list_for_each_head()". The "list was empty" case would now exit with
'pos' not set at all (or set to NULL if we add that). Or it would be
set to the last entry.

And regardless, that means that all the

        if (pos == head)

kinds of checks after the loop would be fundamentally broken.

Darn. I really hoped for (and naively expected) that we could actually
have the compiler warn about the use-after-loop case. That whole
"compiler will now complain about bad use" was integral to my clever
plan to use the C99 feature of declaring the iterator inside the loop.

But my "clever plan" was apparently some ACME-level Wile E. Coyote sh*t.

Darn.

                   Linus

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

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: 2022-02-28 20:10:59

On Mon, Feb 28, 2022 at 12:03 PM Linus Torvalds
[off-list ref] wrote:
Side note: we do need *some* way to do it.
Ooh.

This patch is a work of art.

And I mean that in the worst possible way.

We can do

        typeof(pos) pos

in the 'for ()' loop, and never use __iter at all.

That means that inside the for-loop, we use a _different_ 'pos' than outside.

And then the compiler will not see some "might be uninitialized", but
the outer 'pos' *will* be uninitialized.

Unless, of course, the outer 'pos' had that pointless explicit initializer.

Here - can somebody poke holes in this "work of art" patch?

                     Linus

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

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: 2022-02-28 20:15:20

On Mon, Feb 28, 2022 at 12:10 PM Linus Torvalds
[off-list ref] wrote:
We can do

        typeof(pos) pos

in the 'for ()' loop, and never use __iter at all.

That means that inside the for-loop, we use a _different_ 'pos' than outside.
The thing that makes me throw up in my mouth a bit is that in that

        typeof(pos) pos

the first 'pos' (that we use for just the typeof) is that outer-level
'pos', IOW it's a *different* 'pos' than the second 'pos' in that same
declaration that declares the inner level shadowing new 'pos'
variable.

If I was a compiler person, I would say "Linus, that thing is too ugly
to live", and I would hate it. I'm just hoping that even compiler
people say "that's *so* ugly it's almost beautiful".

Because it does seem to work. It's not pretty, but hey, it's not like
our headers are really ever be winning any beauty contests...

                Linus

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

From: Segher Boessenkool <hidden>
Date: 2022-02-28 22:07:10

On Mon, Feb 28, 2022 at 12:14:44PM -0800, Linus Torvalds wrote:
On Mon, Feb 28, 2022 at 12:10 PM Linus Torvalds
[off-list ref] wrote:
quoted
We can do

        typeof(pos) pos

in the 'for ()' loop, and never use __iter at all.

That means that inside the for-loop, we use a _different_ 'pos' than outside.
The thing that makes me throw up in my mouth a bit is that in that

        typeof(pos) pos

the first 'pos' (that we use for just the typeof) is that outer-level
'pos', IOW it's a *different* 'pos' than the second 'pos' in that same
declaration that declares the inner level shadowing new 'pos'
variable.
The new "pos" has not yet been declared, so this has to refer to the
outer "pos", it cannot be the inner one.  Because it hasn't been
declared yet :-)

Compare this to
  typeof (pos) pos = pos;
where that last "pos" *does* refer to the newly declared one: that
declaration has already been done!  (So this code is UB btw, 6.3.2.1/2).
If I was a compiler person, I would say "Linus, that thing is too ugly
to live", and I would hate it. I'm just hoping that even compiler
people say "that's *so* ugly it's almost beautiful".
It is perfectly well-defined.  Well, it would be good if we (GCC) would
document it does work, and if someone tested it on LLVM as well.  But it
is really hard to implement it to *not* work :-)
Because it does seem to work. It's not pretty, but hey, it's not like
our headers are really ever be winning any beauty contests...
It is very pretty!  Needs a comment though :-)


Segher

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

From: Matthew Wilcox <willy@infradead.org>
Date: 2022-02-28 20:17:05

On Mon, Feb 28, 2022 at 12:10:24PM -0800, Linus Torvalds wrote:
We can do

        typeof(pos) pos

in the 'for ()' loop, and never use __iter at all.

That means that inside the for-loop, we use a _different_ 'pos' than outside.
Then we can never use -Wshadow ;-(  I'd love to be able to turn it on;
it catches real bugs.
+#define list_for_each_entry(pos, head, member)					\
+	for (typeof(pos) pos = list_first_entry(head, typeof(*pos), member);	\
+	     !list_entry_is_head(pos, head, member);	\
 	     pos = list_next_entry(pos, member))

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

From: Johannes Berg <johannes@sipsolutions.net>
Date: 2022-02-28 20:29:10

On Mon, 2022-02-28 at 20:16 +0000, Matthew Wilcox wrote:
On Mon, Feb 28, 2022 at 12:10:24PM -0800, Linus Torvalds wrote:
quoted
We can do

        typeof(pos) pos

in the 'for ()' loop, and never use __iter at all.

That means that inside the for-loop, we use a _different_ 'pos' than outside.
Then we can never use -Wshadow ;-(  I'd love to be able to turn it on;
it catches real bugs.
I was just going to say the same thing...

If we're willing to change the API for the macro, we could do

  list_for_each_entry(type, pos, head, member)

and then actually take advantage of -Wshadow?

johannes

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

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: 2022-02-28 20:41:32

On Mon, Feb 28, 2022 at 12:29 PM Johannes Berg
[off-list ref] wrote:
If we're willing to change the API for the macro, we could do

  list_for_each_entry(type, pos, head, member)

and then actually take advantage of -Wshadow?
See my reply to Willy. There is no way -Wshadow will ever happen.

I considered that (type, pos, head, member) kind of thing, to the
point of trying it for one file, but it ends up as horrendous syntax.
It turns out that declaring the type separately really helps, and
avoids crazy long lines among other things.

It would be unacceptable for another reason too - the amount of churn
would just be immense. Every single use of that macro (and related
macros) would change, even the ones that really don't need it or want
it (ie the good kinds that already only use the variable inside the
loop).

So "typeof(pos) pos" may be ugly - but it's a very localized ugly.

                    Linus

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

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: 2022-02-28 20:37:50

On Mon, Feb 28, 2022 at 12:16 PM Matthew Wilcox [off-list ref] wrote:
Then we can never use -Wshadow ;-(  I'd love to be able to turn it on;
it catches real bugs.
Oh, we already can never use -Wshadow regardless of things like this.
That bridge hasn't just been burned, it never existed in the first
place.

The whole '-Wshadow' thing simply cannot work with local variables in
macros - something that we've used since day 1.

Try this (as a "p.c" file):

        #define min(a,b) ({                     \
                typeof(a) __a = (a);            \
                typeof(b) __b = (b);            \
                __a < __b ? __a : __b; })

        int min3(int a, int b, int c)
        {
                return min(a,min(b,c));
        }

and now do "gcc -O2 -S t.c".

Then try it with -Wshadow.

In other words, -Wshadow is simply not acceptable. Never has been,
never will be, and that has nothing to do with the

        typeof(pos) pos

kind of thing.

Your argument just isn't an argument.

              Linus

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

From: Matthew Wilcox <willy@infradead.org>
Date: 2022-02-28 23:27:09

On Mon, Feb 28, 2022 at 12:37:15PM -0800, Linus Torvalds wrote:
On Mon, Feb 28, 2022 at 12:16 PM Matthew Wilcox [off-list ref] wrote:
quoted
Then we can never use -Wshadow ;-(  I'd love to be able to turn it on;
it catches real bugs.
Oh, we already can never use -Wshadow regardless of things like this.
That bridge hasn't just been burned, it never existed in the first
place.

The whole '-Wshadow' thing simply cannot work with local variables in
macros - something that we've used since day 1.

Try this (as a "p.c" file):

        #define min(a,b) ({                     \
                typeof(a) __a = (a);            \
                typeof(b) __b = (b);            \
                __a < __b ? __a : __b; })

        int min3(int a, int b, int c)
        {
                return min(a,min(b,c));
        }

and now do "gcc -O2 -S t.c".

Then try it with -Wshadow.
#define ___PASTE(a, b)	a##b
#define __PASTE(a, b) ___PASTE(a, b)
#define _min(a, b, u) ({         \
        typeof(a) __PASTE(__a,u) = (a);            \
        typeof(b) __PASTE(__b,u) = (b);            \
        __PASTE(__a,u) < __PASTE(__b,u) ? __PASTE(__a,u) : __PASTE(__b,u); })

#define min(a, b) _min(a, b, __COUNTER__)

int min3(int a, int b, int c)
{
        return min(a,min(b,c));
}

(probably there's a more elegant way to do this)

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

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: 2022-03-01 00:52:28

On Mon, Feb 28, 2022 at 3:26 PM Matthew Wilcox [off-list ref] wrote:
#define ___PASTE(a, b)  a##b
#define __PASTE(a, b) ___PASTE(a, b)
#define _min(a, b, u) ({         \
Yeah, except that's ugly beyond belief, plus it's literally not what
we do in the kernel.

Really. The "-Wshadow doesn't work on the kernel" is not some new
issue, because you have to do completely insane things to the source
code to enable it.

Just compare your uglier-than-sin version to my straightforward one.
One does the usual and obvious "use a private variable to avoid the
classic multi-use of a macro argument". And the other one is an
abomination.

              Linus

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

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: 2022-03-01 01:04:16

On Mon, Feb 28, 2022 at 4:45 PM Linus Torvalds
[off-list ref] wrote:
Yeah, except that's ugly beyond belief, plus it's literally not what
we do in the kernel.
(Of course, I probably shouldn't have used 'min()' as an example,
because that is actually one of the few places where we do exactly
that, using our __UNIQUE_ID() macros. Exactly because people _have_
tried to do -Wshadow when doing W=2).

                 Linus

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

From: Kees Cook <hidden>
Date: 2022-03-01 18:14:19

On Mon, Feb 28, 2022 at 04:45:11PM -0800, Linus Torvalds wrote:
Really. The "-Wshadow doesn't work on the kernel" is not some new
issue, because you have to do completely insane things to the source
code to enable it.
The first big glitch with -Wshadow was with shadowed global variables.
GCC 4.8 fixed that, but it still yells about shadowed functions. What
_almost_ works is -Wshadow=local. At first glace, all the warnings
look solvable, but then one will eventually discover __wait_event()
and associated macros that mix when and how deeply it intentionally
shadows variables. :)

Another way to try to catch misused shadow variables is
-Wunused-but-set-varible, but it, too, has tons of false positives.

I tried to capture some of the rationale and research here:
https://github.com/KSPP/linux/issues/152

-- 
Kees Cook

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

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: 2022-03-01 18:54:10

On Tue, Mar 1, 2022 at 10:14 AM Kees Cook [off-list ref] wrote:
The first big glitch with -Wshadow was with shadowed global variables.
GCC 4.8 fixed that, but it still yells about shadowed functions. What
_almost_ works is -Wshadow=local.
Heh. Yeah, I just have long memories of "-Wshadow was a disaster". You
looked into the details.
Another way to try to catch misused shadow variables is
-Wunused-but-set-varible, but it, too, has tons of false positives.
That on the face of it should be an easy warning to get technically
right for a compiler.

So I assume the "false positives" are simply because we end up having
various variables that really don't end up being used - and
"intentionally" so).

Or rather, they might only be used under some config option - perhaps
the use is even syntactically there and parsed, but the compiler
notices that it's turned off under some

        if (IS_ENABLED(..))

option? Because yeah, we have a lot of those.

I think that's a common theme with a lot of compiler warnings: on the
face of it they sound "obviously sane" and nobody should ever write
code like that.

A conditional that is always true? Sounds idiotic, and sounds like a
reasonable thing for a compiler to warn about, since why would you
have a conditional in the first place for that?

But then you realize that maybe the conditional is a build config
option, and "always true" suddenly makes sense. Or it's a test for
something that is always true on _that_architecture_ but not in some
general sense (ie testing "sizeof()"). Or it's a purely syntactic
conditional, like "do { } while (0)".

It's why I'm often so down on a lot of the odd warnings that are
hiding under W=1 and friends. They all may make sense in the trivial
case ("That is insane") but then in the end they happen for sane code.

And yeah, -Wshadow has had tons of history with macro nesting, and
just being badly done in the first place (eg "strlen" can be a
perfectly fine local variable).

That said, maybe people could ask the gcc and clan people for a way to
_mark_ the places where we expect to validly see shadowing. For
example, that "local variable in a macro expression statement" thing
is absolutely horrendous to fix with preprocessor tricks to try to
make for unique identifiers.

But I think it would be much more syntactically reasonable to add (for
example) a "shadow" attribute to such a variable exactly to tell the
compiler "yeah, yeah, I know this identifier could shadow an outer
one" and turn it off that way.

               Linus

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

From: Matthew Wilcox <willy@infradead.org>
Date: 2022-03-01 19:01:41

On Tue, Mar 01, 2022 at 10:14:07AM -0800, Kees Cook wrote:
On Mon, Feb 28, 2022 at 04:45:11PM -0800, Linus Torvalds wrote:
quoted
Really. The "-Wshadow doesn't work on the kernel" is not some new
issue, because you have to do completely insane things to the source
code to enable it.
The first big glitch with -Wshadow was with shadowed global variables.
GCC 4.8 fixed that, but it still yells about shadowed functions. What
_almost_ works is -Wshadow=local. At first glace, all the warnings
look solvable, but then one will eventually discover __wait_event()
and associated macros that mix when and how deeply it intentionally
shadows variables. :)
Well, that's just disgusting.  Macros fundamentally shouldn't be
referring to things that aren't in their arguments.  The first step to
cleaning this up is ...

I'll take a look at the rest of cleaning this up soon.

From 28ffe35d56223d4242b915832299e5acc926737e Mon Sep 17 00:00:00 2001
From: "Matthew Wilcox (Oracle)" <willy@infradead.org>
Date: Tue, 1 Mar 2022 13:47:07 -0500
Subject: [PATCH] wait: Parameterize the return variable to ___wait_event()

Macros should not refer to variables which aren't in their arguments.
Pass the name from its callers.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 include/linux/swait.h    | 12 ++++++------
 include/linux/wait.h     | 32 ++++++++++++++++----------------
 include/linux/wait_bit.h |  4 ++--
 3 files changed, 24 insertions(+), 24 deletions(-)
diff --git a/include/linux/swait.h b/include/linux/swait.h
index 6a8c22b8c2a5..5e8e9b13be2d 100644
--- a/include/linux/swait.h
+++ b/include/linux/swait.h
@@ -191,14 +191,14 @@ do {									\
 } while (0)
 
 #define __swait_event_timeout(wq, condition, timeout)			\
-	___swait_event(wq, ___wait_cond_timeout(condition),		\
+	___swait_event(wq, ___wait_cond_timeout(condition, __ret),	\
 		      TASK_UNINTERRUPTIBLE, timeout,			\
 		      __ret = schedule_timeout(__ret))
 
 #define swait_event_timeout_exclusive(wq, condition, timeout)		\
 ({									\
 	long __ret = timeout;						\
-	if (!___wait_cond_timeout(condition))				\
+	if (!___wait_cond_timeout(condition, __ret))			\
 		__ret = __swait_event_timeout(wq, condition, timeout);	\
 	__ret;								\
 })
@@ -216,14 +216,14 @@ do {									\
 })
 
 #define __swait_event_interruptible_timeout(wq, condition, timeout)	\
-	___swait_event(wq, ___wait_cond_timeout(condition),		\
+	___swait_event(wq, ___wait_cond_timeout(condition, __ret),	\
 		      TASK_INTERRUPTIBLE, timeout,			\
 		      __ret = schedule_timeout(__ret))
 
 #define swait_event_interruptible_timeout_exclusive(wq, condition, timeout)\
 ({									\
 	long __ret = timeout;						\
-	if (!___wait_cond_timeout(condition))				\
+	if (!___wait_cond_timeout(condition, __ret))			\
 		__ret = __swait_event_interruptible_timeout(wq,		\
 						condition, timeout);	\
 	__ret;								\
@@ -252,7 +252,7 @@ do {									\
 } while (0)
 
 #define __swait_event_idle_timeout(wq, condition, timeout)		\
-	___swait_event(wq, ___wait_cond_timeout(condition),		\
+	___swait_event(wq, ___wait_cond_timeout(condition, __ret),	\
 		       TASK_IDLE, timeout,				\
 		       __ret = schedule_timeout(__ret))
 
@@ -278,7 +278,7 @@ do {									\
 #define swait_event_idle_timeout_exclusive(wq, condition, timeout)	\
 ({									\
 	long __ret = timeout;						\
-	if (!___wait_cond_timeout(condition))				\
+	if (!___wait_cond_timeout(condition, __ret))			\
 		__ret = __swait_event_idle_timeout(wq,			\
 						   condition, timeout);	\
 	__ret;								\
diff --git a/include/linux/wait.h b/include/linux/wait.h
index 851e07da2583..890cce3c0f2e 100644
--- a/include/linux/wait.h
+++ b/include/linux/wait.h
@@ -271,7 +271,7 @@ static inline void wake_up_pollfree(struct wait_queue_head *wq_head)
 		__wake_up_pollfree(wq_head);
 }
 
-#define ___wait_cond_timeout(condition)						\
+#define ___wait_cond_timeout(condition, __ret)					\
 ({										\
 	bool __cond = (condition);						\
 	if (__cond && !__ret)							\
@@ -386,7 +386,7 @@ do {										\
 })
 
 #define __wait_event_timeout(wq_head, condition, timeout)			\
-	___wait_event(wq_head, ___wait_cond_timeout(condition),			\
+	___wait_event(wq_head, ___wait_cond_timeout(condition, __ret),		\
 		      TASK_UNINTERRUPTIBLE, 0, timeout,				\
 		      __ret = schedule_timeout(__ret))
 
@@ -413,13 +413,13 @@ do {										\
 ({										\
 	long __ret = timeout;							\
 	might_sleep();								\
-	if (!___wait_cond_timeout(condition))					\
+	if (!___wait_cond_timeout(condition, __ret))				\
 		__ret = __wait_event_timeout(wq_head, condition, timeout);	\
 	__ret;									\
 })
 
 #define __wait_event_freezable_timeout(wq_head, condition, timeout)		\
-	___wait_event(wq_head, ___wait_cond_timeout(condition),			\
+	___wait_event(wq_head, ___wait_cond_timeout(condition, __ret),		\
 		      TASK_INTERRUPTIBLE, 0, timeout,				\
 		      __ret = freezable_schedule_timeout(__ret))
 
@@ -431,7 +431,7 @@ do {										\
 ({										\
 	long __ret = timeout;							\
 	might_sleep();								\
-	if (!___wait_cond_timeout(condition))					\
+	if (!___wait_cond_timeout(condition, __ret))				\
 		__ret = __wait_event_freezable_timeout(wq_head, condition, timeout); \
 	__ret;									\
 })
@@ -503,7 +503,7 @@ do {										\
 })
 
 #define __wait_event_interruptible_timeout(wq_head, condition, timeout)		\
-	___wait_event(wq_head, ___wait_cond_timeout(condition),			\
+	___wait_event(wq_head, ___wait_cond_timeout(condition, __ret),		\
 		      TASK_INTERRUPTIBLE, 0, timeout,				\
 		      __ret = schedule_timeout(__ret))
 
@@ -531,7 +531,7 @@ do {										\
 ({										\
 	long __ret = timeout;							\
 	might_sleep();								\
-	if (!___wait_cond_timeout(condition))					\
+	if (!___wait_cond_timeout(condition, __ret))				\
 		__ret = __wait_event_interruptible_timeout(wq_head,		\
 						condition, timeout);		\
 	__ret;									\
@@ -698,7 +698,7 @@ do {										\
 } while (0)
 
 #define __wait_event_idle_timeout(wq_head, condition, timeout)			\
-	___wait_event(wq_head, ___wait_cond_timeout(condition),			\
+	___wait_event(wq_head, ___wait_cond_timeout(condition, __ret),		\
 		      TASK_IDLE, 0, timeout,					\
 		      __ret = schedule_timeout(__ret))
 
@@ -725,13 +725,13 @@ do {										\
 ({										\
 	long __ret = timeout;							\
 	might_sleep();								\
-	if (!___wait_cond_timeout(condition))					\
+	if (!___wait_cond_timeout(condition, __ret))				\
 		__ret = __wait_event_idle_timeout(wq_head, condition, timeout);	\
 	__ret;									\
 })
 
 #define __wait_event_idle_exclusive_timeout(wq_head, condition, timeout)	\
-	___wait_event(wq_head, ___wait_cond_timeout(condition),			\
+	___wait_event(wq_head, ___wait_cond_timeout(condition, __ret),		\
 		      TASK_IDLE, 1, timeout,					\
 		      __ret = schedule_timeout(__ret))
 
@@ -762,7 +762,7 @@ do {										\
 ({										\
 	long __ret = timeout;							\
 	might_sleep();								\
-	if (!___wait_cond_timeout(condition))					\
+	if (!___wait_cond_timeout(condition, __ret))				\
 		__ret = __wait_event_idle_exclusive_timeout(wq_head, condition, timeout);\
 	__ret;									\
 })
@@ -932,7 +932,7 @@ extern int do_wait_intr_irq(wait_queue_head_t *, wait_queue_entry_t *);
 })
 
 #define __wait_event_killable_timeout(wq_head, condition, timeout)		\
-	___wait_event(wq_head, ___wait_cond_timeout(condition),			\
+	___wait_event(wq_head, ___wait_cond_timeout(condition, __ret),		\
 		      TASK_KILLABLE, 0, timeout,				\
 		      __ret = schedule_timeout(__ret))
 
@@ -962,7 +962,7 @@ extern int do_wait_intr_irq(wait_queue_head_t *, wait_queue_entry_t *);
 ({										\
 	long __ret = timeout;							\
 	might_sleep();								\
-	if (!___wait_cond_timeout(condition))					\
+	if (!___wait_cond_timeout(condition, __ret))				\
 		__ret = __wait_event_killable_timeout(wq_head,			\
 						condition, timeout);		\
 	__ret;									\
@@ -1107,7 +1107,7 @@ do {										\
 })
 
 #define __wait_event_lock_irq_timeout(wq_head, condition, lock, timeout, state)	\
-	___wait_event(wq_head, ___wait_cond_timeout(condition),			\
+	___wait_event(wq_head, ___wait_cond_timeout(condition, __ret),		\
 		      state, 0, timeout,					\
 		      spin_unlock_irq(&lock);					\
 		      __ret = schedule_timeout(__ret);				\
@@ -1141,7 +1141,7 @@ do {										\
 						  timeout)			\
 ({										\
 	long __ret = timeout;							\
-	if (!___wait_cond_timeout(condition))					\
+	if (!___wait_cond_timeout(condition, __ret))				\
 		__ret = __wait_event_lock_irq_timeout(				\
 					wq_head, condition, lock, timeout,	\
 					TASK_INTERRUPTIBLE);			\
@@ -1151,7 +1151,7 @@ do {										\
 #define wait_event_lock_irq_timeout(wq_head, condition, lock, timeout)		\
 ({										\
 	long __ret = timeout;							\
-	if (!___wait_cond_timeout(condition))					\
+	if (!___wait_cond_timeout(condition, __ret))				\
 		__ret = __wait_event_lock_irq_timeout(				\
 					wq_head, condition, lock, timeout,	\
 					TASK_UNINTERRUPTIBLE);			\
diff --git a/include/linux/wait_bit.h b/include/linux/wait_bit.h
index 7dec36aecbd9..227e6a20a978 100644
--- a/include/linux/wait_bit.h
+++ b/include/linux/wait_bit.h
@@ -292,7 +292,7 @@ do {									\
 })
 
 #define __wait_var_event_timeout(var, condition, timeout)		\
-	___wait_var_event(var, ___wait_cond_timeout(condition),		\
+	___wait_var_event(var, ___wait_cond_timeout(condition, __ret),	\
 			  TASK_UNINTERRUPTIBLE, 0, timeout,		\
 			  __ret = schedule_timeout(__ret))
 
@@ -300,7 +300,7 @@ do {									\
 ({									\
 	long __ret = timeout;						\
 	might_sleep();							\
-	if (!___wait_cond_timeout(condition))				\
+	if (!___wait_cond_timeout(condition, __ret))			\
 		__ret = __wait_var_event_timeout(var, condition, timeout); \
 	__ret;								\
 })
-- 
2.34.1

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

From: David Laight <hidden>
Date: 2022-03-01 03:04:05

From: Matthew Wilcox
Sent: 28 February 2022 20:16

On Mon, Feb 28, 2022 at 12:10:24PM -0800, Linus Torvalds wrote:
quoted
We can do

        typeof(pos) pos

in the 'for ()' loop, and never use __iter at all.

That means that inside the for-loop, we use a _different_ 'pos' than outside.
Then we can never use -Wshadow ;-(  I'd love to be able to turn it on;
it catches real bugs.
quoted
+#define list_for_each_entry(pos, head, member)					\
+	for (typeof(pos) pos = list_first_entry(head, typeof(*pos), member);	\
+	     !list_entry_is_head(pos, head, member);	\
 	     pos = list_next_entry(pos, member))
Actually can't you use 'pos' to temporarily hold the address of 'member'.
Something like:
	for (pos = (void *)head; \
		pos ? ((pos = (void *)pos - offsetof(member)), 1) : 0; \
		pos = (void *)pos->next)
So that 'pos' is NULL if the loop terminates.
No pointers outside structures are generated.
Probably need to kill list_entry_is_head() - or it just checks for NULL.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

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

From: Jakob Koschel <hidden>
Date: 2022-02-28 21:47:11

On 28. Feb 2022, at 21:10, Linus Torvalds [off-list ref] wrote:

On Mon, Feb 28, 2022 at 12:03 PM Linus Torvalds
[off-list ref] wrote:
quoted
Side note: we do need *some* way to do it.
Ooh.

This patch is a work of art.

And I mean that in the worst possible way.

We can do

       typeof(pos) pos

in the 'for ()' loop, and never use __iter at all.

That means that inside the for-loop, we use a _different_ 'pos' than outside.

And then the compiler will not see some "might be uninitialized", but
the outer 'pos' *will* be uninitialized.

Unless, of course, the outer 'pos' had that pointless explicit initializer.
The goal of this is to get compiler warnings right? This would indeed be great.

Changing the list_for_each_entry() macro first will break all of those cases
(e.g. the ones using 'list_entry_is_head()).
I assumed it is better to fix those cases first and then have a simple
coccinelle script changing the macro + moving the iterator into the scope
of the macro.
Here - can somebody poke holes in this "work of art" patch?
With this you are no longer able to set the 'outer' pos within the list
iterator loop body or am I missing something? Like this it stays
uninitialized but you'll probably want to set it from within the loop.

You would then yet again need a variable with another name to use
after the loop.

I fail to see how this will make most of the changes in this
patch obsolete (if that was the intention).
                    Linus
<patch.diff>
- Jakob

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

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: 2022-03-01 00:41:40

On Mon, Feb 28, 2022 at 1:47 PM Jakob Koschel [off-list ref] wrote:
The goal of this is to get compiler warnings right? This would indeed be great.
Yes, so I don't mind having a one-time patch that has been gathered
using some automated checker tool, but I don't think that works from a
long-term maintenance perspective.

So if we have the basic rule being "don't use the loop iterator after
the loop has finished, because it can cause all kinds of subtle
issues", then in _addition_ to fixing the existing code paths that
have this issue, I really would want to (a) get a compiler warning for
future cases and (b) make it not actually _work_ for future cases.

Because otherwise it will just happen again.
Changing the list_for_each_entry() macro first will break all of those cases
(e.g. the ones using 'list_entry_is_head()).
So I have no problems with breaking cases that we basically already
have a patch for due to  your automated tool. There were certainly
more than a handful, but it didn't look _too_ bad to just make the
rule be "don't use the iterator after the loop".

Of course, that's just based on that patch of yours. Maybe there are a
ton of other cases that your patch didn't change, because they didn't
match your trigger case, so I may just be overly optimistic here.

But basically to _me_, the important part is that the end result is
maintainable longer-term. I'm more than happy to have a one-time patch
to fix a lot of dubious cases if we can then have clean rules going
forward.
I assumed it is better to fix those cases first and then have a simple
coccinelle script changing the macro + moving the iterator into the scope
of the macro.
So that had been another plan of mine, until I actually looked at
changing the macro. In the one case I looked at, it was ugly beyond
belief.

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.

So yes, initially my idea had been to just move the iterator entirely
inside the macro. But specifying the type got so ugly that I think
that

        typeof (pos) pos

trick inside the macro really ends up giving us the best of all worlds:

 (a) let's us keep the existing syntax and code for all the nice cases
that did everything inside the loop anyway

 (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

 (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
With this you are no longer able to set the 'outer' pos within the list
iterator loop body or am I missing something?
Correct. Any assignment inside the loop will be entirely just to the
local loop case. So any "break;" out of the loop will have to set
another variable - like your updated patch did.
I fail to see how this will make most of the changes in this
patch obsolete (if that was the intention).
I hope my explanation above clarifies my thinking: I do not dislike
your patch, and in fact your patch is indeed required to make the new
semantics work.

What I disliked was always the maintainability of your patch - making
the rules be something that isn't actually visible in the source code,
and letting the old semantics still work as well as they ever did, and
having to basically run some verification pass to find bad users.

(I also disliked your original patch that mixed up the "CPU
speculation type safety" with the actual non-speculative problems, but
that was another issue).

                Linus

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

From: Jakub Kicinski <kuba@kernel.org>
Date: 2022-03-01 06:32:43

On Mon, 28 Feb 2022 16:41:04 -0800 Linus Torvalds wrote:
So yes, initially my idea had been to just move the iterator entirely
inside the macro. But specifying the type got so ugly that I think
that

        typeof (pos) pos

trick inside the macro really ends up giving us the best of all worlds:

 (a) let's us keep the existing syntax and code for all the nice cases
that did everything inside the loop anyway

 (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

 (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
I presume the goal is that we can do this without changing existing
code? Otherwise actually moving the iterator into the loop body would
be an option, by creating a different hidden variable:

#define list_iter(head)						\
	for (struct list head *_l = (head)->next; _l != (head); _l = _l->next)

#define list_iter_entry(var, member)		\
	list_entry(_l, typeof(*var), member)


	list_iter(&p->a_head) {
		struct entry *e = list_iter_entry(e, a_member);

		/* use e->... */
	}


Or we can slide into soft insanity and exploit one of Kees'es tricks
to encode the type of the entries "next to" the head:

#define LIST_HEAD_MEM(name, type)			\
	union {						\
		struct list_head name;			\
		type *name ## _entry;			\
	}

struct entry {
	struct list_head a_member;
};

struct parent {
	LIST_HEAD_MEM(a_head, struct entry);
};

#define list_for_each_magic(pos, head, member)				\
	for (typeof(**(head ## _entry)) *pos = list_first_entry(head, typeof(**(head ## _entry)), member); \
	     &pos->member != (head);					\
	     pos = list_next_entry(pos, member))


	list_for_each_magic(e, &p->a_head, a_member) {
		/* use e->... */
	}


I'll show myself out...

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

From: Jakob Koschel <hidden>
Date: 2022-03-01 11:28:24

On 1. Mar 2022, at 01:41, Linus Torvalds [off-list ref] wrote:

On Mon, Feb 28, 2022 at 1:47 PM Jakob Koschel [off-list ref] wrote:
quoted
The goal of this is to get compiler warnings right? This would indeed be great.
Yes, so I don't mind having a one-time patch that has been gathered
using some automated checker tool, but I don't think that works from a
long-term maintenance perspective.

So if we have the basic rule being "don't use the loop iterator after
the loop has finished, because it can cause all kinds of subtle
issues", then in _addition_ to fixing the existing code paths that
have this issue, I really would want to (a) get a compiler warning for
future cases and (b) make it not actually _work_ for future cases.

Because otherwise it will just happen again.
quoted
Changing the list_for_each_entry() macro first will break all of those cases
(e.g. the ones using 'list_entry_is_head()).
So I have no problems with breaking cases that we basically already
have a patch for due to  your automated tool. There were certainly
more than a handful, but it didn't look _too_ bad to just make the
rule be "don't use the iterator after the loop".

Of course, that's just based on that patch of yours. Maybe there are a
ton of other cases that your patch didn't change, because they didn't
match your trigger case, so I may just be overly optimistic here.
Based on the coccinelle script there are ~480 cases that need fixing
in total. I'll now finish all of them and then split them by
submodules as Greg suggested and repost a patch set per submodule.
Sounds good?
But basically to _me_, the important part is that the end result is
maintainable longer-term. I'm more than happy to have a one-time patch
to fix a lot of dubious cases if we can then have clean rules going
forward.
quoted
I assumed it is better to fix those cases first and then have a simple
coccinelle script changing the macro + moving the iterator into the scope
of the macro.
So that had been another plan of mine, until I actually looked at
changing the macro. In the one case I looked at, it was ugly beyond
belief.

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.

So yes, initially my idea had been to just move the iterator entirely
inside the macro. But specifying the type got so ugly that I think
that

       typeof (pos) pos

trick inside the macro really ends up giving us the best of all worlds:

(a) let's us keep the existing syntax and code for all the nice cases
that did everything inside the loop anyway

(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

(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
quoted
With this you are no longer able to set the 'outer' pos within the list
iterator loop body or am I missing something?
Correct. Any assignment inside the loop will be entirely just to the
local loop case. So any "break;" out of the loop will have to set
another variable - like your updated patch did.
quoted
I fail to see how this will make most of the changes in this
patch obsolete (if that was the intention).
I hope my explanation above clarifies my thinking: I do not dislike
your patch, and in fact your patch is indeed required to make the new
semantics work.
ok it's all clear now, thanks for clarifying.
I've defined all the 'tmp' iterator variables uninitialized so applying
your patch on top of that later will just give the nice compiler warning 
if they are used past the loop body.
What I disliked was always the maintainability of your patch - making
the rules be something that isn't actually visible in the source code,
and letting the old semantics still work as well as they ever did, and
having to basically run some verification pass to find bad users.
Since this patch is not a complete list of cases that need fixing (30%)
I haven't included the actual change of moving the iterator variable
into the loop and thought that would be a second step coming after this
is merged.

With these changes alone, yes you still rely on manual verification passes.
(I also disliked your original patch that mixed up the "CPU
speculation type safety" with the actual non-speculative problems, but
that was another issue).

               Linus
- Jakob

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

From: Greg KH <hidden>
Date: 2022-03-01 17:36:44

On Tue, Mar 01, 2022 at 12:28:15PM +0100, Jakob Koschel wrote:
quoted
On 1. Mar 2022, at 01:41, Linus Torvalds [off-list ref] wrote:

On Mon, Feb 28, 2022 at 1:47 PM Jakob Koschel [off-list ref] wrote:
quoted
The goal of this is to get compiler warnings right? This would indeed be great.
Yes, so I don't mind having a one-time patch that has been gathered
using some automated checker tool, but I don't think that works from a
long-term maintenance perspective.

So if we have the basic rule being "don't use the loop iterator after
the loop has finished, because it can cause all kinds of subtle
issues", then in _addition_ to fixing the existing code paths that
have this issue, I really would want to (a) get a compiler warning for
future cases and (b) make it not actually _work_ for future cases.

Because otherwise it will just happen again.
quoted
Changing the list_for_each_entry() macro first will break all of those cases
(e.g. the ones using 'list_entry_is_head()).
So I have no problems with breaking cases that we basically already
have a patch for due to  your automated tool. There were certainly
more than a handful, but it didn't look _too_ bad to just make the
rule be "don't use the iterator after the loop".

Of course, that's just based on that patch of yours. Maybe there are a
ton of other cases that your patch didn't change, because they didn't
match your trigger case, so I may just be overly optimistic here.
Based on the coccinelle script there are ~480 cases that need fixing
in total. I'll now finish all of them and then split them by
submodules as Greg suggested and repost a patch set per submodule.
Sounds good?
Sounds good to me!

If you need help carving these up and maintaining them over time as
different subsystem maintainers accept/ignore them, just let me know.
Doing large patchsets like this can be tough without a lot of
experience.

thanks,

greg k-h

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

From: Jakob Koschel <hidden>
Date: 2022-03-01 17:40:51

On 1. Mar 2022, at 18:36, Greg KH [off-list ref] wrote:

On Tue, Mar 01, 2022 at 12:28:15PM +0100, Jakob Koschel wrote:
quoted
quoted
On 1. Mar 2022, at 01:41, Linus Torvalds [off-list ref] wrote:

On Mon, Feb 28, 2022 at 1:47 PM Jakob Koschel [off-list ref] wrote:
quoted
The goal of this is to get compiler warnings right? This would indeed be great.
Yes, so I don't mind having a one-time patch that has been gathered
using some automated checker tool, but I don't think that works from a
long-term maintenance perspective.

So if we have the basic rule being "don't use the loop iterator after
the loop has finished, because it can cause all kinds of subtle
issues", then in _addition_ to fixing the existing code paths that
have this issue, I really would want to (a) get a compiler warning for
future cases and (b) make it not actually _work_ for future cases.

Because otherwise it will just happen again.
quoted
Changing the list_for_each_entry() macro first will break all of those cases
(e.g. the ones using 'list_entry_is_head()).
So I have no problems with breaking cases that we basically already
have a patch for due to  your automated tool. There were certainly
more than a handful, but it didn't look _too_ bad to just make the
rule be "don't use the iterator after the loop".

Of course, that's just based on that patch of yours. Maybe there are a
ton of other cases that your patch didn't change, because they didn't
match your trigger case, so I may just be overly optimistic here.
Based on the coccinelle script there are ~480 cases that need fixing
in total. I'll now finish all of them and then split them by
submodules as Greg suggested and repost a patch set per submodule.
Sounds good?
Sounds good to me!

If you need help carving these up and maintaining them over time as
different subsystem maintainers accept/ignore them, just let me know.
Doing large patchsets like this can be tough without a lot of
experience.
Very much appreciated!

There will probably be some cases that do not match one of the pattern
we already discussed and need separate attention.

I was planning to start with one subsystem and adjust the coming ones
according to the feedback gather there instead of posting all of them
in one go.
thanks,

greg k-h
- Jakob

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

From: Greg KH <hidden>
Date: 2022-03-01 17:58:13

On Tue, Mar 01, 2022 at 06:40:04PM +0100, Jakob Koschel wrote:
quoted
On 1. Mar 2022, at 18:36, Greg KH [off-list ref] wrote:

On Tue, Mar 01, 2022 at 12:28:15PM +0100, Jakob Koschel wrote:
quoted
quoted
On 1. Mar 2022, at 01:41, Linus Torvalds [off-list ref] wrote:

On Mon, Feb 28, 2022 at 1:47 PM Jakob Koschel [off-list ref] wrote:
quoted
The goal of this is to get compiler warnings right? This would indeed be great.
Yes, so I don't mind having a one-time patch that has been gathered
using some automated checker tool, but I don't think that works from a
long-term maintenance perspective.

So if we have the basic rule being "don't use the loop iterator after
the loop has finished, because it can cause all kinds of subtle
issues", then in _addition_ to fixing the existing code paths that
have this issue, I really would want to (a) get a compiler warning for
future cases and (b) make it not actually _work_ for future cases.

Because otherwise it will just happen again.
quoted
Changing the list_for_each_entry() macro first will break all of those cases
(e.g. the ones using 'list_entry_is_head()).
So I have no problems with breaking cases that we basically already
have a patch for due to  your automated tool. There were certainly
more than a handful, but it didn't look _too_ bad to just make the
rule be "don't use the iterator after the loop".

Of course, that's just based on that patch of yours. Maybe there are a
ton of other cases that your patch didn't change, because they didn't
match your trigger case, so I may just be overly optimistic here.
Based on the coccinelle script there are ~480 cases that need fixing
in total. I'll now finish all of them and then split them by
submodules as Greg suggested and repost a patch set per submodule.
Sounds good?
Sounds good to me!

If you need help carving these up and maintaining them over time as
different subsystem maintainers accept/ignore them, just let me know.
Doing large patchsets like this can be tough without a lot of
experience.
Very much appreciated!

There will probably be some cases that do not match one of the pattern
we already discussed and need separate attention.

I was planning to start with one subsystem and adjust the coming ones
according to the feedback gather there instead of posting all of them
in one go.
That seems wise.  Feel free to use USB as a testing ground for this if
you want to :)

thanks,

greg k-h

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

From: Kees Cook <hidden>
Date: 2022-03-01 18:21:32

On Tue, Mar 01, 2022 at 12:28:15PM +0100, Jakob Koschel wrote:
Based on the coccinelle script there are ~480 cases that need fixing
in total. I'll now finish all of them and then split them by
submodules as Greg suggested and repost a patch set per submodule.
Sounds good?
To help with this splitting, see:
https://github.com/kees/kernel-tools/blob/trunk/split-on-maintainer

It's not perfect, but it'll get you really close. For example, if you
had a single big tree-wide patch applied to your tree:

$ rm 0*.patch
$ git format-patch -1 HEAD
$ mv 0*.patch treewide.patch
$ split-on-maintainer treewide.patch
$ ls 0*.patch

If you have a build log before the patch that spits out warnings, the
--build-log argument can extract those warnings on a per-file basis, too
(though this can be fragile).

-- 
Kees Cook

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

From: Xiaomeng Tong <hidden>
Date: 2022-03-02 09:31:35

On Mon, 28 Feb 2022 16:41:04 -0800, Linus Torvalds
[off-list ref] 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.tong@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: [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr

From: David Laight <hidden>
Date: 2022-03-02 14:04:36

From: Xiaomeng Tong
Sent: 02 March 2022 09:31

On Mon, 28 Feb 2022 16:41:04 -0800, Linus Torvalds
[off-list ref] wrote:
quoted
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)

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

From: Xiaomeng Tong <hidden>
Date: 2022-03-03 02:28:03

On Wed, 2 Mar 2022 14:04:06 +0000, David Laight
[off-list ref] 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: [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr

From: David Laight <hidden>
Date: 2022-03-03 04:58:36

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

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

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

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

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

From: Xiaomeng Tong <hidden>
Date: 2022-03-03 07:27:28

On Thu, 3 Mar 2022 04:58:23 +0000, David Laight wrote:
on 3 Mar 2022 10:27:29 +0800, Xiaomeng Tong wrote:
quoted
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.
quoted
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.
quoted
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."
quoted
quoted
OTOH there may be alternative definitions that can be used to get
the compiler (or other compiler-like tools) to detect broken code.
Even if the definition can't possibly generate a working kerrnel.
The "list_for_each_entry_inside(pos, type, head, member)" way makes
the iterator invisiable outside the loop, and would be catched by
compiler if use-after-loop things happened.
It is also a compete PITA for anything doing a search.
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

35 further messages

Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help