linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC PATCH 00/13] Proposal for speculative safe list iterator
@ 2022-02-17 18:48 Jakob Koschel
  2022-02-17 18:48 ` [RFC PATCH 01/13] list: introduce speculative safe list_for_each_entry() Jakob Koschel
                   ` (12 more replies)
  0 siblings, 13 replies; 70+ messages in thread
From: Jakob Koschel @ 2022-02-17 18:48 UTC (permalink / raw)
  To: Linus Torvalds, linux-kernel
  Cc: linux-arch, Greg Kroah-Hartman, Thomas Gleixner, Arnd Bergman,
	Andy Shevchenko, Andrew Morton, Kees Cook, Mike Rapoport,
	Gustavo A. R. Silva, Brian Johannesmeyer, Cristiano Giuffrida,
	Bos, H.J.,
	Jakob Koschel

Kasper (https://vusec.net/projects/kasper/) has shown that using the
list_for_each_entry() iterator is security critical with transient
execution in mind.

list_for_each_entry() iterates through a list until the terminating
condition is met where pos, the iterator value, reaches the head element.
The head is usually not within the struct of the list elements and either
stands on its own or is included within a struct of an other type.
Therefore using `container_of` on the head element to retrieve the iterator
value is invalid and therefore a type confusion.

If the terminating condition does not hold in transient execution and is
mispredicted, the iterator infered from the head is used in the additional
transient iteration of the loop body. Depending on the struct members
accessed within the loop an attacker can inject own values turning it into
a transient gadget. In the paper we have showed such a case in
find_keyring_by_name().

We patched list_for_each_entry() to ensure that pos is not pointing to an
invalid head element in the transient iteration of the loop. It uses
branchless logic to set pos to NULL when the terminating condition is met,
making it unusable in transient execution.

Unfortunately there are several locations in the kernel where the list
iterator is used after the loop that break with such a change. Luckily
there is the use_after_iter.cocci script that can be used to identify such
code locations. I had to adapt the script slightly since it reduces false
positives in the original use case but those are relevant for this patch.

A large range of reported code locations only use the list iterator after
the loop if there was an early exit (break/goto) and are therefore not
relevant.

However there is still ~200 occurrences that would break with the nospec
implementation of list_for_each_entry().

I have categorized them into certain categories where some are more trivial
to patch than others. I have included reference patches for the first case
of the categories that are trivial or almost trivial to patch. When
everything is clear I will go through the list to fix the other cases
according to the reference patch.

head pos will reference to the case where pos is not actually pointing to
the element of the list but is derived from the head.

I'd especially appreciate comments on how to deal with category 10.
For those cases it is not clear that head pos is not used incorrectly. It
either relies on implict constraints that ensure that the list iteration
exits early or is an actual bug.
Since non of those cases should use pos if the break was not hit they
should not be affected by the speculative list iterator.
I just want to make sure, since changing the list iterator will turn them
from a type confusion into a null pointer dereference (+ some offset
depending on the struct member).

All line numbers are based on commit f71077a4d84bbe8c7b91b7db7c4ef815755ac5e3.


Category 1: &pos->other_member is used to determine if a certain element was found
drivers/usb/gadget/udc/gr_udc.c:1717
drivers/usb/gadget/udc/net2280.c:1273
drivers/usb/gadget/udc/atmel_usba_udc.c:877
drivers/usb/gadget/udc/lpc32xx_udc.c:1847
drivers/usb/gadget/udc/pxa25x_udc.c:983
drivers/usb/gadget/udc/aspeed-vhub/epn.c:481
drivers/usb/gadget/udc/fsl_qe_udc.c:1793
drivers/usb/gadget/udc/net2272.c:946
drivers/usb/gadget/udc/s3c-hsudc.c:893
drivers/usb/gadget/udc/mv_udc_core.c:800
drivers/usb/gadget/udc/at91_udc.c:724
drivers/usb/gadget/udc/mv_u3d_core.c:868
drivers/usb/gadget/udc/udc-xilinx.c:1149
drivers/usb/gadget/udc/bdc/bdc_ep.c:1779
drivers/usb/gadget/udc/fsl_udc_core.c:947
drivers/usb/gadget/udc/omap_udc.c:1019

Category 2: pos is used to determine if a certain element was found
drivers/vfio/mdev/mdev_core.c:349
drivers/usb/gadget/udc/tegra-xudc.c:1427
drivers/usb/gadget/udc/max3420_udc.c:1062
drivers/thermal/thermal_core.c:1085
drivers/thermal/thermal_core.c:645
drivers/thermal/thermal_core.c:1349
drivers/usb/gadget/configfs.c:435
drivers/usb/gadget/configfs.c:893
drivers/usb/mtu3/mtu3_gadget.c:343
drivers/usb/musb/musb_gadget.c:1285
arch/x86/kernel/cpu/sgx/encl.c:466
drivers/scsi/scsi_transport_sas.c:1075

Category 3: pos is used to determine if the list is empty (shouldn't need fixing)
drivers/perf/xgene_pmu.c:1487
drivers/media/pci/saa7134/saa7134-alsa.c:1232
arch/powerpc/sysdev/fsl_gtm.c:106
drivers/staging/greybus/audio_helper.c:135
drivers/misc/fastrpc.c:1363
drivers/dma/ppc4xx/adma.c:3048
drivers/dma/ppc4xx/adma.c:3060

Category 4: pos is used to determine if the break/goto was hit and the list is not empty
arch/arm/mach-mmp/sram.c:54
arch/arm/plat-pxa/ssp.c:54
arch/arm/plat-pxa/ssp.c:78
drivers/block/drbd/drbd_req.c:344
drivers/block/drbd/drbd_req.c:370
drivers/block/drbd/drbd_req.c:396
drivers/counter/counter-chrdev.c:145
drivers/counter/counter-chrdev.c:555
drivers/crypto/cavium/nitrox/nitrox_main.c:280
drivers/dma/ppc4xx/adma.c:954
drivers/firewire/core-transaction.c:93
drivers/firewire/core-transaction.c:963
drivers/firewire/sbp2.c:446
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:2458
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:2466
drivers/gpu/drm/drm_memory.c:79
drivers/gpu/drm/drm_mm.c:938
drivers/gpu/drm/drm_vm.c:160
drivers/gpu/drm/i915/gem/i915_gem_context.c:128
drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c:2471
drivers/gpu/drm/i915/gt/intel_ring.c:211
drivers/gpu/drm/nouveau/nvkm/subdev/fb/ramgk104.c:1168
drivers/gpu/drm/panfrost/panfrost_mmu.c:200
drivers/gpu/drm/scheduler/sched_main.c:1111
drivers/gpu/drm/ttm/ttm_bo.c:702
drivers/gpu/drm/vmwgfx/vmwgfx_kms.c:2600
drivers/gpu/drm/vmwgfx/vmwgfx_kms.c:2624
drivers/infiniband/hw/hfi1/tid_rdma.c:1280
drivers/infiniband/hw/hfi1/tid_rdma.c:1280
drivers/infiniband/hw/mlx4/main.c:1933
drivers/media/dvb-frontends/mxl5xx.c:505
drivers/media/v4l2-core/v4l2-ctrls-api.c:1002
drivers/media/v4l2-core/v4l2-ctrls-api.c:986
drivers/misc/mei/interrupt.c:432
drivers/net/ethernet/mellanox/mlx4/alloc.c:273
drivers/net/ethernet/mellanox/mlx4/alloc.c:273
drivers/net/wireless/intel/ipw2x00/libipw_rx.c:1569
drivers/scsi/lpfc/lpfc_bsg.c:1218
drivers/staging/rtl8192e/rtl819x_TSProc.c:256
drivers/staging/rtl8192e/rtllib_rx.c:2638
drivers/staging/rtl8192u/ieee80211/ieee80211_rx.c:2370
drivers/staging/rtl8192u/ieee80211/rtl819x_TSProc.c:255
drivers/usb/gadget/composite.c:2050
fs/proc/kcore.c:495
kernel/power/snapshot.c:637
kernel/power/snapshot.c:637
kernel/trace/ftrace.c:4570
kernel/trace/ftrace.c:4722
kernel/trace/trace_events.c:2285

drivers/net/ethernet/qlogic/qede/qede_filter.c:843
drivers/gpu/drm/gma500/oaktrail_lvds.c:120
drivers/power/supply/cpcap-battery.c:802
fs/cifs/smb2misc.c:161
kernel/debug/kdb/kdb_main.c:1031
kernel/debug/kdb/kdb_main.c:1038
kernel/debug/kdb/kdb_main.c:795
kernel/trace/trace_eprobe.c:670
net/9p/trans_xen.c:131
net/xfrm/xfrm_ipcomp.c:244
sound/soc/intel/catpt/pcm.c:362
sound/soc/sprd/sprd-mcdt.c:880

Category 5: legitimately uses head pos for cmp
net/ipv4/udp_tunnel_nic.c:849
drivers/gpu/drm/nouveau/nvkm/subdev/clk/base.c:487
drivers/scsi/wd719x.c:691
fs/f2fs/segment.c:368
net/tipc/socket.c:3752
net/tipc/name_table.c:970

Category 6: legitimately uses pos->member
drivers/net/dsa/sja1105/sja1105_vl.c:43
drivers/staging/android/ashmem.c:730
net/core/gro.c:38
fs/fs-writeback.c:437
mm/hugetlb.c:446
mm/compaction.c:1473
fs/locks.c:1130
drivers/net/ethernet/mellanox/mlx4/alloc.c:373
drivers/dma/at_xdmac.c:1583

arch/arm/mm/ioremap.c:107
arch/x86/kvm/mtrr.c:368
block/blk-mq.c:4466
drivers/dma/mv_xor.c:316
drivers/dma/mv_xor_v2.c:366
drivers/gpu/drm/nouveau/nvkm/core/mm.c:77
drivers/gpu/drm/nouveau/nvkm/subdev/timer/base.c:127
drivers/infiniband/hw/usnic/usnic_uiom.c:527
drivers/iommu/iommu.c:448
drivers/iommu/virtio-iommu.c:509
drivers/irqchip/irq-gic-v3-its.c:2093
drivers/md/dm-snap.c:542
drivers/net/ethernet/mellanox/mlx4/alloc.c:393
drivers/net/ethernet/sfc/rx_common.c:601
drivers/net/ethernet/ti/netcp_core.c:491
drivers/net/ethernet/ti/netcp_core.c:491
drivers/net/ethernet/ti/netcp_core.c:540
drivers/net/ethernet/ti/netcp_core.c:540
drivers/nvdimm/namespace_devs.c:1933
drivers/s390/char/tape_core.c:349
drivers/s390/cio/cmf.c:469
drivers/scsi/fcoe/fcoe.c:1885
drivers/scsi/lpfc/lpfc_sli.c:3498
drivers/target/target_core_user.c:400
drivers/usb/host/uhci-q.c:463
rivers/video/fbdev/core/fb_defio.c:139
drivers/xen/events/events_base.c:602
fs/dlm/lock.c:1315
fs/dlm/lock.c:1315
fs/f2fs/segment.c:4185
fs/locks.c:1238
fs/locks.c:1250
kernel/padata.c:397
kernel/trace/trace_output.c:717
net/tipc/group.c:392
net/tipc/monitor.c:416
sound/core/seq/seq_ports.c:152
sound/core/timer.c:1052


drivers/block/drbd/drbd_main.c:230
fs/locks.c:1130
drivers/gpu/drm/vc4/vc4_dsi.c:768

drivers/dma/ppc4xx/adma.c:2733
drivers/iio/industrialio-buffer.c:1128
drivers/net/ethernet/mellanox/mlxsw/spectrum_fid.c:535
drivers/net/ethernet/mellanox/mlxsw/spectrum_mr.c:655
drivers/net/ipvlan/ipvlan_main.c:47
drivers/staging/media/atomisp/pci/atomisp_acc.c:514
fs/gfs2/lops.c:681
fs/gfs2/lops.c:696
kernel/padata.c:656
net/netfilter/nf_tables_api.c:8291


arch/powerpc/platforms/cell/spufs/sched.c:387
drivers/dma/ppc4xx/adma.c:1436


drivers/net/wireless/ath/ath6kl/htc_mbox.c:107
drivers/dma/dw-edma/dw-edma-core.c:139
drivers/dma/dw-edma/dw-edma-core.c:159
drivers/net/ethernet/intel/i40e/i40e_ethtool.c:3966
drivers/scsi/lpfc/lpfc_sli.c:21042


drivers/net/ethernet/intel/ice/ice_sched.c:2808
drivers/net/ethernet/intel/ice/ice_sched.c:2811
drivers/net/wireless/st/cw1200/queue.c:120
arch/powerpc/kvm/book3s_hv_uvmem.c:370
drivers/gpu/drm/tilcdc/tilcdc_external.c:69
drivers/gpu/drm/stm/ltdc.c:559
sound/isa/cs423x/cs4236.c:520
drivers/media/usb/uvc/uvc_v4l2.c:896
drivers/iommu/msm_iommu.c:627

drivers/firmware/stratix10-svc.c:953
drivers/opp/debugfs.c:200

drivers/md/dm-mpath.c:1503
drivers/dma/ppc4xx/adma.c:3071
drivers/gpu/drm/nouveau/nvkm/engine/device/ctrl.c:111
drivers/net/wireless/ath/ath10k/mac.c:510
drivers/s390/char/tty3270.c:1151
drivers/gpu/drm/nouveau/nvkm/subdev/clk/base.c:281

drivers/net/ethernet/intel/i40e/i40e_main.c:7590
drivers/staging/media/atomisp/pci/atomisp_cmd.c:959
drivers/staging/media/atomisp/pci/atomisp_cmd.c:979
drivers/staging/media/atomisp/pci/atomisp_cmd.c:998
drivers/watchdog/watchdog_pretimeout.c:215
fs/jfs/jfs_logmgr.c:892
kernel/workqueue.c:2946
drivers/media/usb/uvc/uvc_v4l2.c:885
drivers/gpu/drm/gma500/oaktrail_crtc.c:428
drivers/virt/acrn/ioreq.c:243
drivers/net/ethernet/mellanox/mlx5/core/eq.c:986
drivers/gpu/drm/omapdrm/omap_encoder.c:109
drivers/scsi/dc395x.c:3598
drivers/staging/media/atomisp/pci/atomisp_acc.c:508

drivers/net/dsa/bcm_sf2_cfp.c:577
drivers/perf/qcom_l2_pmu.c:764
drivers/gpu/drm/gma500/psb_intel_display.c:545
drivers/firmware/efi/vars.c:1092


drivers/staging/greybus/audio_codec.c:603
drivers/scsi/mpt3sas/mpt3sas_base.c:2016

Jakob Koschel (13):
  list: introduce speculative safe list_for_each_entry()
  scripts: coccinelle: adapt to find list_for_each_entry nospec issues
  usb: remove the usage of the list iterator after the loop
  vfio/mdev: remove the usage of the list iterator after the loop
  drivers/perf: remove the usage of the list iterator after the loop
  ARM: mmp: remove the usage of the list iterator after the loop
  udp_tunnel: remove the usage of the list iterator after the loop
  net: dsa: future proof usage of list iterator after the loop
  drbd: future proof usage of list iterator after the loop
  powerpc/spufs: future proof usage of list iterator after the loop
  ath6kl: remove use of list iterator after the loop
  staging: greybus: audio: Remove usage of list iterator after the loop
  scsi: mpt3sas: comment about invalid usage of the list iterator

 arch/arm/mach-mmp/sram.c                      |  7 ++++--
 arch/powerpc/platforms/cell/spufs/sched.c     |  2 ++
 arch/x86/include/asm/barrier.h                | 12 ++++++++++
 drivers/block/drbd/drbd_main.c                |  2 ++
 drivers/net/dsa/sja1105/sja1105_vl.c          |  2 ++
 drivers/net/wireless/ath/ath6kl/htc_mbox.c    |  2 +-
 drivers/perf/xgene_pmu.c                      |  5 ++--
 drivers/scsi/mpt3sas/mpt3sas_base.c           |  2 +-
 drivers/staging/greybus/audio_codec.c         |  4 ++--
 drivers/usb/gadget/udc/gr_udc.c               |  7 ++++--
 drivers/vfio/mdev/mdev_core.c                 |  7 ++++--
 include/linux/list.h                          |  3 ++-
 include/linux/nospec.h                        | 16 +++++++++++++
 net/ipv4/udp_tunnel_nic.c                     |  7 ++++--
 .../coccinelle/iterators/use_after_iter.cocci | 24 -------------------
 15 files changed, 63 insertions(+), 39 deletions(-)


base-commit: f71077a4d84bbe8c7b91b7db7c4ef815755ac5e3
--
2.25.1


^ permalink raw reply	[flat|nested] 70+ messages in thread

* [RFC PATCH 01/13] list: introduce speculative safe list_for_each_entry()
  2022-02-17 18:48 [RFC PATCH 00/13] Proposal for speculative safe list iterator Jakob Koschel
@ 2022-02-17 18:48 ` Jakob Koschel
  2022-02-17 19:29   ` Greg Kroah-Hartman
                     ` (2 more replies)
  2022-02-17 18:48 ` [RFC PATCH 02/13] scripts: coccinelle: adapt to find list_for_each_entry nospec issues Jakob Koschel
                   ` (11 subsequent siblings)
  12 siblings, 3 replies; 70+ messages in thread
From: Jakob Koschel @ 2022-02-17 18:48 UTC (permalink / raw)
  To: Linus Torvalds, linux-kernel
  Cc: linux-arch, Greg Kroah-Hartman, Thomas Gleixner, Arnd Bergman,
	Andy Shevchenko, Andrew Morton, Kees Cook, Mike Rapoport,
	Gustavo A. R. Silva, Brian Johannesmeyer, Cristiano Giuffrida,
	Bos, H.J.,
	Jakob Koschel

list_for_each_entry() selects either the correct value (pos) or a safe
value for the additional mispredicted iteration (NULL) for the list
iterator.
list_for_each_entry() calls select_nospec(), which performs
a branch-less select.

On x86, this select is performed via a cmov. Otherwise, it's performed
via various shift/mask/etc. operations.

Kasper Acknowledgements: Jakob Koschel, Brian Johannesmeyer, Kaveh
Razavi, Herbert Bos, Cristiano Giuffrida from the VUSec group at VU
Amsterdam.

Co-developed-by: Brian Johannesmeyer <bjohannesmeyer@gmail.com>
Signed-off-by: Brian Johannesmeyer <bjohannesmeyer@gmail.com>
Signed-off-by: Jakob Koschel <jakobkoschel@gmail.com>
---
 arch/x86/include/asm/barrier.h | 12 ++++++++++++
 include/linux/list.h           |  3 ++-
 include/linux/nospec.h         | 16 ++++++++++++++++
 3 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/barrier.h b/arch/x86/include/asm/barrier.h
index 35389b2af88e..722797ad74e2 100644
--- a/arch/x86/include/asm/barrier.h
+++ b/arch/x86/include/asm/barrier.h
@@ -48,6 +48,18 @@ static inline unsigned long array_index_mask_nospec(unsigned long index,
 /* Override the default implementation from linux/nospec.h. */
 #define array_index_mask_nospec array_index_mask_nospec
 
+/* Override the default implementation from linux/nospec.h. */
+#define select_nospec(cond, exptrue, expfalse)				\
+({									\
+	typeof(exptrue) _out = (exptrue);				\
+									\
+	asm volatile("test %1, %1\n\t"					\
+	    "cmove %2, %0"						\
+	    : "+r" (_out)						\
+	    : "r" (cond), "r" (expfalse));				\
+	_out;								\
+})
+
 /* Prevent speculative execution past this barrier. */
 #define barrier_nospec() alternative("", "lfence", X86_FEATURE_LFENCE_RDTSC)
 
diff --git a/include/linux/list.h b/include/linux/list.h
index dd6c2041d09c..1a1b39fdd122 100644
--- a/include/linux/list.h
+++ b/include/linux/list.h
@@ -636,7 +636,8 @@ static inline void list_splice_tail_init(struct list_head *list,
  */
 #define list_for_each_entry(pos, head, member)				\
 	for (pos = list_first_entry(head, typeof(*pos), member);	\
-	     !list_entry_is_head(pos, head, member);			\
+	    ({ bool _cond = !list_entry_is_head(pos, head, member);	\
+	     pos = select_nospec(_cond, pos, NULL); _cond; }); \
 	     pos = list_next_entry(pos, member))
 
 /**
diff --git a/include/linux/nospec.h b/include/linux/nospec.h
index c1e79f72cd89..ca8ed81e4f9e 100644
--- a/include/linux/nospec.h
+++ b/include/linux/nospec.h
@@ -67,4 +67,20 @@ int arch_prctl_spec_ctrl_set(struct task_struct *task, unsigned long which,
 /* Speculation control for seccomp enforced mitigation */
 void arch_seccomp_spec_mitigate(struct task_struct *task);
 
+/**
+ * select_nospec - select a value without using a branch; equivalent to:
+ * cond ? exptrue : expfalse;
+ */
+#ifndef select_nospec
+#define select_nospec(cond, exptrue, expfalse)				\
+({									\
+	unsigned long _t = (unsigned long) (exptrue);			\
+	unsigned long _f = (unsigned long) (expfalse);			\
+	unsigned long _c = (unsigned long) (cond);			\
+	OPTIMIZER_HIDE_VAR(_c);						\
+	unsigned long _m = -((_c | -_c) >> (BITS_PER_LONG - 1));	\
+	(typeof(exptrue)) ((_t & _m) | (_f & ~_m));			\
+})
+#endif
+
 #endif /* _LINUX_NOSPEC_H */
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 70+ messages in thread

* [RFC PATCH 02/13] scripts: coccinelle: adapt to find list_for_each_entry nospec issues
  2022-02-17 18:48 [RFC PATCH 00/13] Proposal for speculative safe list iterator Jakob Koschel
  2022-02-17 18:48 ` [RFC PATCH 01/13] list: introduce speculative safe list_for_each_entry() Jakob Koschel
@ 2022-02-17 18:48 ` Jakob Koschel
  2022-02-17 18:48 ` [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop Jakob Koschel
                   ` (10 subsequent siblings)
  12 siblings, 0 replies; 70+ messages in thread
From: Jakob Koschel @ 2022-02-17 18:48 UTC (permalink / raw)
  To: Linus Torvalds, linux-kernel
  Cc: linux-arch, Greg Kroah-Hartman, Thomas Gleixner, Arnd Bergman,
	Andy Shevchenko, Andrew Morton, Kees Cook, Mike Rapoport,
	Gustavo A. R. Silva, Brian Johannesmeyer, Cristiano Giuffrida,
	Bos, H.J.,
	Jakob Koschel

These changes are just temporary to illustrate how I composed the list
of code locations mentioned here.
While for example the usage of &c->member is currently safe to use,
it is an issue if c is set to NULL when the terminating condition is
met.

Signed-off-by: Jakob Koschel <jakobkoschel@gmail.com>
---
 .../coccinelle/iterators/use_after_iter.cocci | 24 -------------------
 1 file changed, 24 deletions(-)

diff --git a/scripts/coccinelle/iterators/use_after_iter.cocci b/scripts/coccinelle/iterators/use_after_iter.cocci
index 676edd562eef..01563a242f00 100644
--- a/scripts/coccinelle/iterators/use_after_iter.cocci
+++ b/scripts/coccinelle/iterators/use_after_iter.cocci
@@ -91,43 +91,19 @@ list_for_each_entry(c,...) S
 |
 list_for_each_entry_reverse(c,...) S
 |
-list_for_each_entry_continue(c,...) S
-|
-list_for_each_entry_continue_reverse(c,...) S
-|
-list_for_each_entry_from(c,...) S
-|
 list_for_each_entry_safe(c,...) S
 |
 list_for_each_entry_safe(x,c,...) S
 |
-list_for_each_entry_safe_continue(c,...) S
-|
-list_for_each_entry_safe_continue(x,c,...) S
-|
-list_for_each_entry_safe_from(c,...) S
-|
-list_for_each_entry_safe_from(x,c,...) S
-|
 list_for_each_entry_safe_reverse(c,...) S
 |
 list_for_each_entry_safe_reverse(x,c,...) S
 |
 hlist_for_each_entry(c,...) S
 |
-hlist_for_each_entry_continue(c,...) S
-|
-hlist_for_each_entry_from(c,...) S
-|
 hlist_for_each_entry_safe(c,...) S
 |
-list_remove_head(x,c,...)
-|
-list_entry_is_head(c,...)
-|
 sizeof(<+...c...+>)
-|
- &c->member
 |
 T c;
 |
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 70+ messages in thread

* [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop
  2022-02-17 18:48 [RFC PATCH 00/13] Proposal for speculative safe list iterator Jakob Koschel
  2022-02-17 18:48 ` [RFC PATCH 01/13] list: introduce speculative safe list_for_each_entry() Jakob Koschel
  2022-02-17 18:48 ` [RFC PATCH 02/13] scripts: coccinelle: adapt to find list_for_each_entry nospec issues Jakob Koschel
@ 2022-02-17 18:48 ` Jakob Koschel
  2022-02-17 19:28   ` Linus Torvalds
  2022-02-17 18:48 ` [RFC PATCH 04/13] vfio/mdev: " Jakob Koschel
                   ` (9 subsequent siblings)
  12 siblings, 1 reply; 70+ messages in thread
From: Jakob Koschel @ 2022-02-17 18:48 UTC (permalink / raw)
  To: Linus Torvalds, linux-kernel
  Cc: linux-arch, Greg Kroah-Hartman, Thomas Gleixner, Arnd Bergman,
	Andy Shevchenko, Andrew Morton, Kees Cook, Mike Rapoport,
	Gustavo A. R. Silva, Brian Johannesmeyer, Cristiano Giuffrida,
	Bos, H.J.,
	Jakob Koschel

It is unsafe to assume that &req->req != _req can only evaluate
to false if the break within the list iterator is hit.

When the break is not hit, req is set to an address derived from the
head element. If _req would match with that value of req it would
allow continuing beyond the safety check even if the _req was never
found within the list.

Signed-off-by: Jakob Koschel <jakobkoschel@gmail.com>
---
 drivers/usb/gadget/udc/gr_udc.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/udc/gr_udc.c b/drivers/usb/gadget/udc/gr_udc.c
index 4b35739d3695..18ae2c7a1656 100644
--- a/drivers/usb/gadget/udc/gr_udc.c
+++ b/drivers/usb/gadget/udc/gr_udc.c
@@ -1695,6 +1695,7 @@ static int gr_dequeue(struct usb_ep *_ep, struct usb_request *_req)
 	struct gr_udc *dev;
 	int ret = 0;
 	unsigned long flags;
+	bool found = false;
 
 	ep = container_of(_ep, struct gr_ep, ep);
 	if (!_ep || !_req || (!ep->ep.desc && ep->num != 0))
@@ -1711,10 +1712,12 @@ static int gr_dequeue(struct usb_ep *_ep, struct usb_request *_req)
 
 	/* Make sure it's actually queued on this endpoint */
 	list_for_each_entry(req, &ep->queue, queue) {
-		if (&req->req == _req)
+		if (&req->req == _req) {
+			found = true;
 			break;
+		}
 	}
-	if (&req->req != _req) {
+	if (!found) {
 		ret = -EINVAL;
 		goto out;
 	}
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 70+ messages in thread

* [RFC PATCH 04/13] vfio/mdev: remove the usage of the list iterator after the loop
  2022-02-17 18:48 [RFC PATCH 00/13] Proposal for speculative safe list iterator Jakob Koschel
                   ` (2 preceding siblings ...)
  2022-02-17 18:48 ` [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop Jakob Koschel
@ 2022-02-17 18:48 ` Jakob Koschel
  2022-02-18 15:12   ` Jason Gunthorpe
  2022-02-17 18:48 ` [RFC PATCH 05/13] drivers/perf: " Jakob Koschel
                   ` (8 subsequent siblings)
  12 siblings, 1 reply; 70+ messages in thread
From: Jakob Koschel @ 2022-02-17 18:48 UTC (permalink / raw)
  To: Linus Torvalds, linux-kernel
  Cc: linux-arch, Greg Kroah-Hartman, Thomas Gleixner, Arnd Bergman,
	Andy Shevchenko, Andrew Morton, Kees Cook, Mike Rapoport,
	Gustavo A. R. Silva, Brian Johannesmeyer, Cristiano Giuffrida,
	Bos, H.J.,
	Jakob Koschel

It is unsafe to assume that tmp != mdev can only evaluate to false
if the break within the list iterator is hit.

When the break is not hit, tmp is set to an address derived from the
head element. If mdev would match with that value of tmp it would allow
continuing beyond the safety check even if mdev was never found within
the list

Signed-off-by: Jakob Koschel <jakobkoschel@gmail.com>
---
 drivers/vfio/mdev/mdev_core.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
index b314101237fe..e646ba5036f4 100644
--- a/drivers/vfio/mdev/mdev_core.c
+++ b/drivers/vfio/mdev/mdev_core.c
@@ -339,14 +339,17 @@ int mdev_device_remove(struct mdev_device *mdev)
 {
 	struct mdev_device *tmp;
 	struct mdev_parent *parent = mdev->type->parent;
+	bool found = false;
 
 	mutex_lock(&mdev_list_lock);
 	list_for_each_entry(tmp, &mdev_list, next) {
-		if (tmp == mdev)
+		if (tmp == mdev) {
+			found = true;
 			break;
+		}
 	}
 
-	if (tmp != mdev) {
+	if (!found) {
 		mutex_unlock(&mdev_list_lock);
 		return -ENODEV;
 	}
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 70+ messages in thread

* [RFC PATCH 05/13] drivers/perf: remove the usage of the list iterator after the loop
  2022-02-17 18:48 [RFC PATCH 00/13] Proposal for speculative safe list iterator Jakob Koschel
                   ` (3 preceding siblings ...)
  2022-02-17 18:48 ` [RFC PATCH 04/13] vfio/mdev: " Jakob Koschel
@ 2022-02-17 18:48 ` Jakob Koschel
  2022-02-17 18:48 ` [RFC PATCH 06/13] ARM: mmp: " Jakob Koschel
                   ` (7 subsequent siblings)
  12 siblings, 0 replies; 70+ messages in thread
From: Jakob Koschel @ 2022-02-17 18:48 UTC (permalink / raw)
  To: Linus Torvalds, linux-kernel
  Cc: linux-arch, Greg Kroah-Hartman, Thomas Gleixner, Arnd Bergman,
	Andy Shevchenko, Andrew Morton, Kees Cook, Mike Rapoport,
	Gustavo A. R. Silva, Brian Johannesmeyer, Cristiano Giuffrida,
	Bos, H.J.,
	Jakob Koschel

To introduce a speculative safe list iterator, the iterator variable will
be set to NULL when the terminating condition of the loop is hit.

The code before assumed rentry would only be NULL if the break is
hit, this assumption no longer holds. Once the speculative safe list
iterator is merged the condition could be replace with if (!entry)
instead.

Signed-off-by: Jakob Koschel <jakobkoschel@gmail.com>
---
 drivers/perf/xgene_pmu.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/perf/xgene_pmu.c b/drivers/perf/xgene_pmu.c
index 2b6d476bd213..523ff5c53103 100644
--- a/drivers/perf/xgene_pmu.c
+++ b/drivers/perf/xgene_pmu.c
@@ -1463,6 +1463,7 @@ xgene_pmu_dev_ctx *acpi_get_pmu_hw_inf(struct xgene_pmu *xgene_pmu,
 	struct resource_entry *rentry;
 	int enable_bit;
 	int rc;
+	bool found = false;
 
 	ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
 	if (!ctx)
@@ -1478,13 +1479,13 @@ xgene_pmu_dev_ctx *acpi_get_pmu_hw_inf(struct xgene_pmu *xgene_pmu,
 	list_for_each_entry(rentry, &resource_list, node) {
 		if (resource_type(rentry->res) == IORESOURCE_MEM) {
 			res = *rentry->res;
-			rentry = NULL;
+			found = true;
 			break;
 		}
 	}
 	acpi_dev_free_resource_list(&resource_list);
 
-	if (rentry) {
+	if (!found) {
 		dev_err(dev, "PMU type %d: No memory resource found\n", type);
 		return NULL;
 	}
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 70+ messages in thread

* [RFC PATCH 06/13] ARM: mmp: remove the usage of the list iterator after the loop
  2022-02-17 18:48 [RFC PATCH 00/13] Proposal for speculative safe list iterator Jakob Koschel
                   ` (4 preceding siblings ...)
  2022-02-17 18:48 ` [RFC PATCH 05/13] drivers/perf: " Jakob Koschel
@ 2022-02-17 18:48 ` Jakob Koschel
  2022-02-17 18:48 ` [RFC PATCH 07/13] udp_tunnel: " Jakob Koschel
                   ` (6 subsequent siblings)
  12 siblings, 0 replies; 70+ messages in thread
From: Jakob Koschel @ 2022-02-17 18:48 UTC (permalink / raw)
  To: Linus Torvalds, linux-kernel
  Cc: linux-arch, Greg Kroah-Hartman, Thomas Gleixner, Arnd Bergman,
	Andy Shevchenko, Andrew Morton, Kees Cook, Mike Rapoport,
	Gustavo A. R. Silva, Brian Johannesmeyer, Cristiano Giuffrida,
	Bos, H.J.,
	Jakob Koschel

To introduce a speculative safe list iterator, the iterator variable
will be set to NULL when the terminating condition of the loop is
hit.

The code before assumed info would be derived from the head if
the break did not hit, this assumption no longer holds.
Once the speculative safe list iterator is merged the condition could
be replace with if (!info) instead.

Signed-off-by: Jakob Koschel <jakobkoschel@gmail.com>
---
 arch/arm/mach-mmp/sram.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-mmp/sram.c b/arch/arm/mach-mmp/sram.c
index 6794e2db1ad5..b163d2da53e7 100644
--- a/arch/arm/mach-mmp/sram.c
+++ b/arch/arm/mach-mmp/sram.c
@@ -39,6 +39,7 @@ static LIST_HEAD(sram_bank_list);
 struct gen_pool *sram_get_gpool(char *pool_name)
 {
 	struct sram_bank_info *info = NULL;
+	bool found = false;
 
 	if (!pool_name)
 		return NULL;
@@ -46,12 +47,14 @@ struct gen_pool *sram_get_gpool(char *pool_name)
 	mutex_lock(&sram_lock);
 
 	list_for_each_entry(info, &sram_bank_list, node)
-		if (!strcmp(pool_name, info->pool_name))
+		if (!strcmp(pool_name, info->pool_name)) {
+			found = true;
 			break;
+		}
 
 	mutex_unlock(&sram_lock);
 
-	if (&info->node == &sram_bank_list)
+	if (!found)
 		return NULL;
 
 	return info->gpool;
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 70+ messages in thread

* [RFC PATCH 07/13] udp_tunnel: remove the usage of the list iterator after the loop
  2022-02-17 18:48 [RFC PATCH 00/13] Proposal for speculative safe list iterator Jakob Koschel
                   ` (5 preceding siblings ...)
  2022-02-17 18:48 ` [RFC PATCH 06/13] ARM: mmp: " Jakob Koschel
@ 2022-02-17 18:48 ` Jakob Koschel
  2022-02-23 20:00   ` Christophe JAILLET
  2022-02-17 18:48 ` [RFC PATCH 08/13] net: dsa: future proof usage of " Jakob Koschel
                   ` (5 subsequent siblings)
  12 siblings, 1 reply; 70+ messages in thread
From: Jakob Koschel @ 2022-02-17 18:48 UTC (permalink / raw)
  To: Linus Torvalds, linux-kernel
  Cc: linux-arch, Greg Kroah-Hartman, Thomas Gleixner, Arnd Bergman,
	Andy Shevchenko, Andrew Morton, Kees Cook, Mike Rapoport,
	Gustavo A. R. Silva, Brian Johannesmeyer, Cristiano Giuffrida,
	Bos, H.J.,
	Jakob Koschel

The usage of node->dev after the loop body is a legitimate type
confusion if the break was not hit. It will compare an undefined
memory location with dev that could potentially be equal. The value
of node->dev in this case could either be a random struct member of the
head element or an out-of-bounds value.

Therefore it is more safe to use the found variable. With the
introduction of speculative safe list iterator this check could be
replaced with if (!node).

Signed-off-by: Jakob Koschel <jakobkoschel@gmail.com>
---
 net/ipv4/udp_tunnel_nic.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/udp_tunnel_nic.c b/net/ipv4/udp_tunnel_nic.c
index b91003538d87..c47f9fb36d29 100644
--- a/net/ipv4/udp_tunnel_nic.c
+++ b/net/ipv4/udp_tunnel_nic.c
@@ -842,11 +842,14 @@ udp_tunnel_nic_unregister(struct net_device *dev, struct udp_tunnel_nic *utn)
 	 */
 	if (info->shared) {
 		struct udp_tunnel_nic_shared_node *node, *first;
+		bool found = false;
 
 		list_for_each_entry(node, &info->shared->devices, list)
-			if (node->dev == dev)
+			if (node->dev == dev) {
+				found = true;
 				break;
-		if (node->dev != dev)
+			}
+		if (!found)
 			return;
 
 		list_del(&node->list);
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 70+ messages in thread

* [RFC PATCH 08/13] net: dsa: future proof usage of list iterator after the loop
  2022-02-17 18:48 [RFC PATCH 00/13] Proposal for speculative safe list iterator Jakob Koschel
                   ` (6 preceding siblings ...)
  2022-02-17 18:48 ` [RFC PATCH 07/13] udp_tunnel: " Jakob Koschel
@ 2022-02-17 18:48 ` Jakob Koschel
  2022-02-17 18:48 ` [RFC PATCH 09/13] drbd: " Jakob Koschel
                   ` (4 subsequent siblings)
  12 siblings, 0 replies; 70+ messages in thread
From: Jakob Koschel @ 2022-02-17 18:48 UTC (permalink / raw)
  To: Linus Torvalds, linux-kernel
  Cc: linux-arch, Greg Kroah-Hartman, Thomas Gleixner, Arnd Bergman,
	Andy Shevchenko, Andrew Morton, Kees Cook, Mike Rapoport,
	Gustavo A. R. Silva, Brian Johannesmeyer, Cristiano Giuffrida,
	Bos, H.J.,
	Jakob Koschel

With the speculative safe version of the list iterator p will be NULL if
the terminating condition was hit and needs to be reset to p derived
from the head, before using p->list.

Signed-off-by: Jakob Koschel <jakobkoschel@gmail.com>
---
 drivers/net/dsa/sja1105/sja1105_vl.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/dsa/sja1105/sja1105_vl.c b/drivers/net/dsa/sja1105/sja1105_vl.c
index f5dca6a9b0f9..a4f2b95b09c4 100644
--- a/drivers/net/dsa/sja1105/sja1105_vl.c
+++ b/drivers/net/dsa/sja1105/sja1105_vl.c
@@ -40,6 +40,8 @@ static int sja1105_insert_gate_entry(struct sja1105_gating_config *gating_cfg,
 			if (e->interval < p->interval)
 				break;
 		}
+		if (!p)
+			p = list_entry(p, &gating_cfg->entries, list);
 		list_add(&e->list, p->list.prev);
 	}
 
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 70+ messages in thread

* [RFC PATCH 09/13] drbd: future proof usage of list iterator after the loop
  2022-02-17 18:48 [RFC PATCH 00/13] Proposal for speculative safe list iterator Jakob Koschel
                   ` (7 preceding siblings ...)
  2022-02-17 18:48 ` [RFC PATCH 08/13] net: dsa: future proof usage of " Jakob Koschel
@ 2022-02-17 18:48 ` Jakob Koschel
  2022-02-17 18:48 ` [RFC PATCH 10/13] powerpc/spufs: " Jakob Koschel
                   ` (3 subsequent siblings)
  12 siblings, 0 replies; 70+ messages in thread
From: Jakob Koschel @ 2022-02-17 18:48 UTC (permalink / raw)
  To: Linus Torvalds, linux-kernel
  Cc: linux-arch, Greg Kroah-Hartman, Thomas Gleixner, Arnd Bergman,
	Andy Shevchenko, Andrew Morton, Kees Cook, Mike Rapoport,
	Gustavo A. R. Silva, Brian Johannesmeyer, Cristiano Giuffrida,
	Bos, H.J.,
	Jakob Koschel

With the speculative safe version of the list iterator req will be NULL
if the terminating condition was hit and needs to be reset to req
derived from the head, before calling list_for_each_entry_safe_from().

Signed-off-by: Jakob Koschel <jakobkoschel@gmail.com>
---
 drivers/block/drbd/drbd_main.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/block/drbd/drbd_main.c b/drivers/block/drbd/drbd_main.c
index 6f450816c4fa..d98205b46f59 100644
--- a/drivers/block/drbd/drbd_main.c
+++ b/drivers/block/drbd/drbd_main.c
@@ -227,6 +227,8 @@ void tl_release(struct drbd_connection *connection, unsigned int barrier_nr,
 	list_for_each_entry(req, &connection->transfer_log, tl_requests)
 		if (req->epoch == expect_epoch)
 			break;
+	if (!req)
+		req = list_entry(req, &connection->transfer_log, tl_requests);
 	list_for_each_entry_safe_from(req, r, &connection->transfer_log, tl_requests) {
 		if (req->epoch != expect_epoch)
 			break;
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 70+ messages in thread

* [RFC PATCH 10/13] powerpc/spufs: future proof usage of list iterator after the loop
  2022-02-17 18:48 [RFC PATCH 00/13] Proposal for speculative safe list iterator Jakob Koschel
                   ` (8 preceding siblings ...)
  2022-02-17 18:48 ` [RFC PATCH 09/13] drbd: " Jakob Koschel
@ 2022-02-17 18:48 ` Jakob Koschel
  2022-02-17 18:48 ` [RFC PATCH 11/13] ath6kl: remove use " Jakob Koschel
                   ` (2 subsequent siblings)
  12 siblings, 0 replies; 70+ messages in thread
From: Jakob Koschel @ 2022-02-17 18:48 UTC (permalink / raw)
  To: Linus Torvalds, linux-kernel
  Cc: linux-arch, Greg Kroah-Hartman, Thomas Gleixner, Arnd Bergman,
	Andy Shevchenko, Andrew Morton, Kees Cook, Mike Rapoport,
	Gustavo A. R. Silva, Brian Johannesmeyer, Cristiano Giuffrida,
	Bos, H.J.,
	Jakob Koschel

With the speculative safe version of the list iterator spu will be NULL
if the terminating condition was hit and needs to be reset to spu
derived from the head, before returning spu.

Signed-off-by: Jakob Koschel <jakobkoschel@gmail.com>
---
 arch/powerpc/platforms/cell/spufs/sched.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/powerpc/platforms/cell/spufs/sched.c b/arch/powerpc/platforms/cell/spufs/sched.c
index 369206489895..5b2afda1653d 100644
--- a/arch/powerpc/platforms/cell/spufs/sched.c
+++ b/arch/powerpc/platforms/cell/spufs/sched.c
@@ -384,6 +384,8 @@ static struct spu *ctx_location(struct spu *ref, int offset, int node)
 		}
 	}
 
+	if (!spu)
+		spu = list_entry(spu, ref->aff_list.prev, aff_list);
 	return spu;
 }
 
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 70+ messages in thread

* [RFC PATCH 11/13] ath6kl: remove use of list iterator after the loop
  2022-02-17 18:48 [RFC PATCH 00/13] Proposal for speculative safe list iterator Jakob Koschel
                   ` (9 preceding siblings ...)
  2022-02-17 18:48 ` [RFC PATCH 10/13] powerpc/spufs: " Jakob Koschel
@ 2022-02-17 18:48 ` Jakob Koschel
  2022-02-17 18:48 ` [RFC PATCH 12/13] staging: greybus: audio: Remove usage " Jakob Koschel
  2022-02-17 18:48 ` [RFC PATCH 13/13] scsi: mpt3sas: comment about invalid usage of the list iterator Jakob Koschel
  12 siblings, 0 replies; 70+ messages in thread
From: Jakob Koschel @ 2022-02-17 18:48 UTC (permalink / raw)
  To: Linus Torvalds, linux-kernel
  Cc: linux-arch, Greg Kroah-Hartman, Thomas Gleixner, Arnd Bergman,
	Andy Shevchenko, Andrew Morton, Kees Cook, Mike Rapoport,
	Gustavo A. R. Silva, Brian Johannesmeyer, Cristiano Giuffrida,
	Bos, H.J.,
	Jakob Koschel

Since the list iteration cannot abort early, cur_ep_dist->list will
always reference to ep_list and can therefore be replaced.

Signed-off-by: Jakob Koschel <jakobkoschel@gmail.com>
---
 drivers/net/wireless/ath/ath6kl/htc_mbox.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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


^ permalink raw reply related	[flat|nested] 70+ messages in thread

* [RFC PATCH 12/13] staging: greybus: audio: Remove usage of list iterator after the loop
  2022-02-17 18:48 [RFC PATCH 00/13] Proposal for speculative safe list iterator Jakob Koschel
                   ` (10 preceding siblings ...)
  2022-02-17 18:48 ` [RFC PATCH 11/13] ath6kl: remove use " Jakob Koschel
@ 2022-02-17 18:48 ` Jakob Koschel
  2022-02-17 18:48 ` [RFC PATCH 13/13] scsi: mpt3sas: comment about invalid usage of the list iterator Jakob Koschel
  12 siblings, 0 replies; 70+ messages in thread
From: Jakob Koschel @ 2022-02-17 18:48 UTC (permalink / raw)
  To: Linus Torvalds, linux-kernel
  Cc: linux-arch, Greg Kroah-Hartman, Thomas Gleixner, Arnd Bergman,
	Andy Shevchenko, Andrew Morton, Kees Cook, Mike Rapoport,
	Gustavo A. R. Silva, Brian Johannesmeyer, Cristiano Giuffrida,
	Bos, H.J.,
	Jakob Koschel

The list iterator module should not be used if data == NULL.
module can only old a legitimate value if data != NULL.

Signed-off-by: Jakob Koschel <jakobkoschel@gmail.com>
---
 drivers/staging/greybus/audio_codec.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/greybus/audio_codec.c b/drivers/staging/greybus/audio_codec.c
index b589cf6b1d03..e19b91e7a72e 100644
--- a/drivers/staging/greybus/audio_codec.c
+++ b/drivers/staging/greybus/audio_codec.c
@@ -599,8 +599,8 @@ static int gbcodec_mute_stream(struct snd_soc_dai *dai, int mute, int stream)
 			break;
 	}
 	if (!data) {
-		dev_err(dai->dev, "%s:%s DATA connection missing\n",
-			dai->name, module->name);
+		dev_err(dai->dev, "%s DATA connection missing\n",
+			dai->name);
 		mutex_unlock(&codec->lock);
 		return -ENODEV;
 	}
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 70+ messages in thread

* [RFC PATCH 13/13] scsi: mpt3sas: comment about invalid usage of the list iterator
  2022-02-17 18:48 [RFC PATCH 00/13] Proposal for speculative safe list iterator Jakob Koschel
                   ` (11 preceding siblings ...)
  2022-02-17 18:48 ` [RFC PATCH 12/13] staging: greybus: audio: Remove usage " Jakob Koschel
@ 2022-02-17 18:48 ` Jakob Koschel
  12 siblings, 0 replies; 70+ messages in thread
From: Jakob Koschel @ 2022-02-17 18:48 UTC (permalink / raw)
  To: Linus Torvalds, linux-kernel
  Cc: linux-arch, Greg Kroah-Hartman, Thomas Gleixner, Arnd Bergman,
	Andy Shevchenko, Andrew Morton, Kees Cook, Mike Rapoport,
	Gustavo A. R. Silva, Brian Johannesmeyer, Cristiano Giuffrida,
	Bos, H.J.,
	Jakob Koschel

Since the list iteration never exists early, reply_q is guaranteed to
be an invalid variable and should not be used within
_base_process_reply_queue(). Since I'm not sure what this code was
supposed to do, I just marked this with this comment.

Signed-off-by: Jakob Koschel <jakobkoschel@gmail.com>
---
 drivers/scsi/mpt3sas/mpt3sas_base.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c b/drivers/scsi/mpt3sas/mpt3sas_base.c
index 511726f92d9a..a6746e124226 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
@@ -2013,7 +2013,7 @@ mpt3sas_base_sync_reply_irqs(struct MPT3SAS_ADAPTER *ioc, u8 poll)
 		}
 	}
 	if (poll)
-		_base_process_reply_queue(reply_q);
+		_base_process_reply_queue(reply_q); // COMMENT
 }
 
 /**
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 70+ messages in thread

* Re: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop
  2022-02-17 18:48 ` [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop Jakob Koschel
@ 2022-02-17 19:28   ` Linus Torvalds
  2022-02-23 14:13     ` Jakob
  0 siblings, 1 reply; 70+ messages in thread
From: Linus Torvalds @ 2022-02-17 19:28 UTC (permalink / raw)
  To: Jakob Koschel
  Cc: Linux Kernel Mailing List, linux-arch, Greg Kroah-Hartman,
	Thomas Gleixner, Arnd Bergman, Andy Shevchenko, Andrew Morton,
	Kees Cook, Mike Rapoport, Gustavo A. R. Silva,
	Brian Johannesmeyer, Cristiano Giuffrida, Bos, H.J.

On Thu, Feb 17, 2022 at 10:50 AM Jakob Koschel <jakobkoschel@gmail.com> wrote:
>
> It is unsafe to assume that &req->req != _req can only evaluate
> to false if the break within the list iterator is hit.

I don't understand what problem you are trying to fix.

Since "req" absolutely *has* to be stable for any of this code to be
valid, then "&req->req" is stable and unambiguous too. The *only* way
_req can point to it would be if we finished the iteration properly.

So I don't see the unsafeness.

Note that all this work with "speculative" execution fundamentally CAN
NOT affect semantics of the code, yet this patch makes statements
about exactly that.

That's not how CPU speculation works.

CPU speculation can expose hidden information that is not
"semantically important" (typically through cache access patterns, but
that's not the only way). So it might be exposing information it
shouldn't.

But if speculation is actually changing semantics, then it's no longer
"speculation" - it's just a bug, plain and simple (either a software
bug due to insufficient serialization, or an actual hardware bug).

IOW, I don't want to see these kinds of apparently pointless changes
to list walking. The patches should explain what that SECONDARY hidden
value you try to protect actually is for each case.

This patch is basically not sensible. It just moves code around in a
way that the compiler could have done anyway (or the compiler could
decide to undo). It doesn't explain what the magic protected value is
that shouldn't be leaked, and it leaves the code just looking odd and
pointless.

                   Linus

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [RFC PATCH 01/13] list: introduce speculative safe list_for_each_entry()
  2022-02-17 18:48 ` [RFC PATCH 01/13] list: introduce speculative safe list_for_each_entry() Jakob Koschel
@ 2022-02-17 19:29   ` Greg Kroah-Hartman
  2022-02-18 16:29     ` Jann Horn
  2022-02-18 16:29   ` Jann Horn
  2022-02-19 19:44   ` Jann Horn
  2 siblings, 1 reply; 70+ messages in thread
From: Greg Kroah-Hartman @ 2022-02-17 19:29 UTC (permalink / raw)
  To: Jakob Koschel
  Cc: Linus Torvalds, linux-kernel, linux-arch, Thomas Gleixner,
	Arnd Bergman, Andy Shevchenko, Andrew Morton, Kees Cook,
	Mike Rapoport, Gustavo A. R. Silva, Brian Johannesmeyer,
	Cristiano Giuffrida, Bos, H.J.

On Thu, Feb 17, 2022 at 07:48:17PM +0100, Jakob Koschel wrote:
> list_for_each_entry() selects either the correct value (pos) or a safe
> value for the additional mispredicted iteration (NULL) for the list
> iterator.
> list_for_each_entry() calls select_nospec(), which performs
> a branch-less select.
> 
> On x86, this select is performed via a cmov. Otherwise, it's performed
> via various shift/mask/etc. operations.
> 
> Kasper Acknowledgements: Jakob Koschel, Brian Johannesmeyer, Kaveh
> Razavi, Herbert Bos, Cristiano Giuffrida from the VUSec group at VU
> Amsterdam.
> 
> Co-developed-by: Brian Johannesmeyer <bjohannesmeyer@gmail.com>
> Signed-off-by: Brian Johannesmeyer <bjohannesmeyer@gmail.com>
> Signed-off-by: Jakob Koschel <jakobkoschel@gmail.com>
> ---
>  arch/x86/include/asm/barrier.h | 12 ++++++++++++
>  include/linux/list.h           |  3 ++-
>  include/linux/nospec.h         | 16 ++++++++++++++++
>  3 files changed, 30 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/include/asm/barrier.h b/arch/x86/include/asm/barrier.h
> index 35389b2af88e..722797ad74e2 100644
> --- a/arch/x86/include/asm/barrier.h
> +++ b/arch/x86/include/asm/barrier.h
> @@ -48,6 +48,18 @@ static inline unsigned long array_index_mask_nospec(unsigned long index,
>  /* Override the default implementation from linux/nospec.h. */
>  #define array_index_mask_nospec array_index_mask_nospec
>  
> +/* Override the default implementation from linux/nospec.h. */
> +#define select_nospec(cond, exptrue, expfalse)				\
> +({									\
> +	typeof(exptrue) _out = (exptrue);				\
> +									\
> +	asm volatile("test %1, %1\n\t"					\
> +	    "cmove %2, %0"						\
> +	    : "+r" (_out)						\
> +	    : "r" (cond), "r" (expfalse));				\
> +	_out;								\
> +})
> +
>  /* Prevent speculative execution past this barrier. */
>  #define barrier_nospec() alternative("", "lfence", X86_FEATURE_LFENCE_RDTSC)
>  
> diff --git a/include/linux/list.h b/include/linux/list.h
> index dd6c2041d09c..1a1b39fdd122 100644
> --- a/include/linux/list.h
> +++ b/include/linux/list.h
> @@ -636,7 +636,8 @@ static inline void list_splice_tail_init(struct list_head *list,
>   */
>  #define list_for_each_entry(pos, head, member)				\
>  	for (pos = list_first_entry(head, typeof(*pos), member);	\
> -	     !list_entry_is_head(pos, head, member);			\
> +	    ({ bool _cond = !list_entry_is_head(pos, head, member);	\
> +	     pos = select_nospec(_cond, pos, NULL); _cond; }); \
>  	     pos = list_next_entry(pos, member))
>  

You are not "introducing" a new macro for this, you are modifying the
existing one such that all users of it now have the select_nospec() call
in it.

Is that intentional?  This is going to hit a _lot_ of existing entries
that probably do not need it at all.

Why not just create list_for_each_entry_nospec()?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [RFC PATCH 04/13] vfio/mdev: remove the usage of the list iterator after the loop
  2022-02-17 18:48 ` [RFC PATCH 04/13] vfio/mdev: " Jakob Koschel
@ 2022-02-18 15:12   ` Jason Gunthorpe
  2022-02-23 14:18     ` Jakob
  0 siblings, 1 reply; 70+ messages in thread
From: Jason Gunthorpe @ 2022-02-18 15:12 UTC (permalink / raw)
  To: Jakob Koschel
  Cc: Linus Torvalds, linux-kernel, linux-arch, Greg Kroah-Hartman,
	Thomas Gleixner, Arnd Bergman, Andy Shevchenko, Andrew Morton,
	Kees Cook, Mike Rapoport, Gustavo A. R. Silva,
	Brian Johannesmeyer, Cristiano Giuffrida, Bos, H.J.

On Thu, Feb 17, 2022 at 07:48:20PM +0100, Jakob Koschel wrote:
> It is unsafe to assume that tmp != mdev can only evaluate to false
> if the break within the list iterator is hit.
> 
> When the break is not hit, tmp is set to an address derived from the
> head element. If mdev would match with that value of tmp it would allow
> continuing beyond the safety check even if mdev was never found within
> the list

I think due to other construction this is not actually possible, but I
guess it is technically correct

This seems like just a straight up style fix with nothing to do with
speculative execution though. Why not just send it as a proper patch?

Jason

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [RFC PATCH 01/13] list: introduce speculative safe list_for_each_entry()
  2022-02-17 18:48 ` [RFC PATCH 01/13] list: introduce speculative safe list_for_each_entry() Jakob Koschel
  2022-02-17 19:29   ` Greg Kroah-Hartman
@ 2022-02-18 16:29   ` Jann Horn
  2022-02-23 14:32     ` Jakob
  2022-02-19 19:44   ` Jann Horn
  2 siblings, 1 reply; 70+ messages in thread
From: Jann Horn @ 2022-02-18 16:29 UTC (permalink / raw)
  To: Jakob Koschel
  Cc: Linus Torvalds, linux-kernel, linux-arch, Greg Kroah-Hartman,
	Thomas Gleixner, Arnd Bergman, Andy Shevchenko, Andrew Morton,
	Kees Cook, Mike Rapoport, Gustavo A. R. Silva,
	Brian Johannesmeyer, Cristiano Giuffrida, Bos, H.J.

On Thu, Feb 17, 2022 at 7:48 PM Jakob Koschel <jakobkoschel@gmail.com> wrote:
> list_for_each_entry() selects either the correct value (pos) or a safe
> value for the additional mispredicted iteration (NULL) for the list
> iterator.
> list_for_each_entry() calls select_nospec(), which performs
> a branch-less select.
>
> On x86, this select is performed via a cmov. Otherwise, it's performed
> via various shift/mask/etc. operations.
>
> Kasper Acknowledgements: Jakob Koschel, Brian Johannesmeyer, Kaveh
> Razavi, Herbert Bos, Cristiano Giuffrida from the VUSec group at VU
> Amsterdam.
>
> Co-developed-by: Brian Johannesmeyer <bjohannesmeyer@gmail.com>
> Signed-off-by: Brian Johannesmeyer <bjohannesmeyer@gmail.com>
> Signed-off-by: Jakob Koschel <jakobkoschel@gmail.com>

Yeah, I think this is the best way to do this without deeply intrusive
changes to how lists are represented in memory.

Some notes on the specific implementation:

>  arch/x86/include/asm/barrier.h | 12 ++++++++++++
>  include/linux/list.h           |  3 ++-
>  include/linux/nospec.h         | 16 ++++++++++++++++
>  3 files changed, 30 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/include/asm/barrier.h b/arch/x86/include/asm/barrier.h
> index 35389b2af88e..722797ad74e2 100644
> --- a/arch/x86/include/asm/barrier.h
> +++ b/arch/x86/include/asm/barrier.h
> @@ -48,6 +48,18 @@ static inline unsigned long array_index_mask_nospec(unsigned long index,
>  /* Override the default implementation from linux/nospec.h. */
>  #define array_index_mask_nospec array_index_mask_nospec
>
> +/* Override the default implementation from linux/nospec.h. */
> +#define select_nospec(cond, exptrue, expfalse)                         \
> +({                                                                     \
> +       typeof(exptrue) _out = (exptrue);                               \
> +                                                                       \
> +       asm volatile("test %1, %1\n\t"                                  \

This shouldn't need "volatile", because it is only necessary if _out
is actually used. Using "volatile" here could prevent optimizing out
useless code. OPTIMIZER_HIDE_VAR() also doesn't use "volatile".

> +           "cmove %2, %0"                                              \
> +           : "+r" (_out)                                               \
> +           : "r" (cond), "r" (expfalse));                              \
> +       _out;                                                           \
> +})

I guess the idea is probably to also add code like this for other
important architectures, in particular arm64?


It might also be a good idea to rename the arch-overridable macro to
something like "arch_select_nospec" and then have a wrapper macro in
include/linux/nospec.h that takes care of type safety issues.

The current definition of the macro doesn't warn if you pass in
incompatible pointer types, like this:

int *bogus_pointer_mix(int cond, int *a, long *b) {
  return select_nospec(cond, a, b);
}

and if you pass in integers of different sizes, it may silently
truncate to the size of the smaller one - this C code:

long wrong_int_conversion(int cond, int a, long b) {
  return select_nospec(cond, a, b);
}

generates this assembly:

wrong_int_conversion:
  test %edi, %edi
  cmove %rdx, %esi
  movslq %esi, %rax
  ret

It might be a good idea to add something like a
static_assert(__same_type(...), ...) to protect against that.

>  /* Prevent speculative execution past this barrier. */
>  #define barrier_nospec() alternative("", "lfence", X86_FEATURE_LFENCE_RDTSC)
>
> diff --git a/include/linux/list.h b/include/linux/list.h
> index dd6c2041d09c..1a1b39fdd122 100644
> --- a/include/linux/list.h
> +++ b/include/linux/list.h
> @@ -636,7 +636,8 @@ static inline void list_splice_tail_init(struct list_head *list,
>   */
>  #define list_for_each_entry(pos, head, member)                         \
>         for (pos = list_first_entry(head, typeof(*pos), member);        \
> -            !list_entry_is_head(pos, head, member);                    \
> +           ({ bool _cond = !list_entry_is_head(pos, head, member);     \
> +            pos = select_nospec(_cond, pos, NULL); _cond; }); \
>              pos = list_next_entry(pos, member))

I wonder if it'd look nicer to write it roughly like this:

#define NOSPEC_TYPE_CHECK(_guarded_var, _cond)                  \
({                                                              \
  bool __cond = (_cond);                                        \
  typeof(_guarded_var) *__guarded_var = &(_guarded_var);        \
  *__guarded_var = select_nospec(__cond, *__guarded_var, NULL); \
  __cond;                                                       \
})

#define list_for_each_entry(pos, head, member)                                \
        for (pos = list_first_entry(head, typeof(*pos), member);              \
             NOSPEC_TYPE_CHECK(head, !list_entry_is_head(pos, head, member)); \
             pos = list_next_entry(pos, member))

I think having a NOSPEC_TYPE_CHECK() like this makes it semantically
clearer, and easier to add in other places? But I don't know if the
others agree...

>  /**
> diff --git a/include/linux/nospec.h b/include/linux/nospec.h
> index c1e79f72cd89..ca8ed81e4f9e 100644
> --- a/include/linux/nospec.h
> +++ b/include/linux/nospec.h
> @@ -67,4 +67,20 @@ int arch_prctl_spec_ctrl_set(struct task_struct *task, unsigned long which,
>  /* Speculation control for seccomp enforced mitigation */
>  void arch_seccomp_spec_mitigate(struct task_struct *task);
>
> +/**
> + * select_nospec - select a value without using a branch; equivalent to:
> + * cond ? exptrue : expfalse;
> + */
> +#ifndef select_nospec
> +#define select_nospec(cond, exptrue, expfalse)                         \
> +({                                                                     \
> +       unsigned long _t = (unsigned long) (exptrue);                   \
> +       unsigned long _f = (unsigned long) (expfalse);                  \
> +       unsigned long _c = (unsigned long) (cond);                      \
> +       OPTIMIZER_HIDE_VAR(_c);                                         \
> +       unsigned long _m = -((_c | -_c) >> (BITS_PER_LONG - 1));        \
> +       (typeof(exptrue)) ((_t & _m) | (_f & ~_m));                     \
> +})
> +#endif

(As a sidenote, it might be easier to implement a conditional zeroing
primitive than a generic conditional select primitive if that's all
you need, something like:

#define cond_nullptr_nospec(_cond, _exp)          \
({                                             \
  unsigned long __exp = (unsigned long)(_exp); \
  unsigned long _mask = 0UL - !(_cond);       \
  OPTIMIZER_HIDE_VAR(_mask);                   \
  (typeof(_exp)) (_mask & __exp);              \
})

)

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [RFC PATCH 01/13] list: introduce speculative safe list_for_each_entry()
  2022-02-17 19:29   ` Greg Kroah-Hartman
@ 2022-02-18 16:29     ` Jann Horn
  0 siblings, 0 replies; 70+ messages in thread
From: Jann Horn @ 2022-02-18 16:29 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Jakob Koschel, Linus Torvalds, linux-kernel, linux-arch,
	Thomas Gleixner, Arnd Bergman, Andy Shevchenko, Andrew Morton,
	Kees Cook, Mike Rapoport, Gustavo A. R. Silva,
	Brian Johannesmeyer, Cristiano Giuffrida, Bos, H.J.

On Thu, Feb 17, 2022 at 8:29 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Thu, Feb 17, 2022 at 07:48:17PM +0100, Jakob Koschel wrote:
> > list_for_each_entry() selects either the correct value (pos) or a safe
> > value for the additional mispredicted iteration (NULL) for the list
> > iterator.
> > list_for_each_entry() calls select_nospec(), which performs
> > a branch-less select.
[...]
> >  #define list_for_each_entry(pos, head, member)                               \
> >       for (pos = list_first_entry(head, typeof(*pos), member);        \
> > -          !list_entry_is_head(pos, head, member);                    \
> > +         ({ bool _cond = !list_entry_is_head(pos, head, member);     \
> > +          pos = select_nospec(_cond, pos, NULL); _cond; }); \
> >            pos = list_next_entry(pos, member))
> >
>
> You are not "introducing" a new macro for this, you are modifying the
> existing one such that all users of it now have the select_nospec() call
> in it.
>
> Is that intentional?  This is going to hit a _lot_ of existing entries
> that probably do not need it at all.
>
> Why not just create list_for_each_entry_nospec()?

My understanding is that almost all uses of `list_for_each_entry()`
currently create type-confused "pos" pointers when they terminate.

(As a sidenote, I've actually seen this lead to a bug in some
out-of-tree code in the past, where someone had a construct like this:

list_for_each_entry(element, ...) {
  if (...)
    break; /* found the element we were looking for */
}
/* use element here */

and then got a "real" type confusion bug from that when no matching
element was found.)

*Every time* you have a list_for_each_entry() iteration over some list
where the list_head that you start from is not embedded in the same
struct as the element list_heads (which is the normal case), and you
don't break from the iteration early, a bogus type-confused pointer
(which might not even be part of the same object as the real list
head, but instead some random out-of-bounds memory in front of it) is
assigned to "pos" (which I think is probably already a violation of
the C standard, but whatever), and this means that almost every
list_for_each_entry() loop ends with a branch that, when
misspeculated, leads to speculative accesses to a type-confused
pointer.

And once you're speculatively accessing type-confused pointers, and
especially if you start writing to them or loading more pointers from
them, it's really hard to reason about what might happen, just like
with "normal" type confusion bugs.


If we don't want to keep this performance hit, then in the long term
it might be a good idea to refactor away the (hideous) idea that the
head of a list and its elements are exactly the same type and
everything's just one big circular thing. Then we could change the
data structures so that this speculative confusion can't happen
anymore and avoid this explicit speculation avoidance on list
iteration.

But for now, I think we probably need this.

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [RFC PATCH 01/13] list: introduce speculative safe list_for_each_entry()
  2022-02-17 18:48 ` [RFC PATCH 01/13] list: introduce speculative safe list_for_each_entry() Jakob Koschel
  2022-02-17 19:29   ` Greg Kroah-Hartman
  2022-02-18 16:29   ` Jann Horn
@ 2022-02-19 19:44   ` Jann Horn
  2 siblings, 0 replies; 70+ messages in thread
From: Jann Horn @ 2022-02-19 19:44 UTC (permalink / raw)
  To: Jakob Koschel
  Cc: Linus Torvalds, linux-kernel, linux-arch, Greg Kroah-Hartman,
	Thomas Gleixner, Arnd Bergman, Andy Shevchenko, Andrew Morton,
	Kees Cook, Mike Rapoport, Gustavo A. R. Silva,
	Brian Johannesmeyer, Cristiano Giuffrida, Bos, H.J.

On Thu, Feb 17, 2022 at 7:48 PM Jakob Koschel <jakobkoschel@gmail.com> wrote:
> list_for_each_entry() selects either the correct value (pos) or a safe
> value for the additional mispredicted iteration (NULL) for the list
> iterator.
> list_for_each_entry() calls select_nospec(), which performs
> a branch-less select.
>
> On x86, this select is performed via a cmov. Otherwise, it's performed
> via various shift/mask/etc. operations.
[...]
>  #define list_for_each_entry(pos, head, member)                         \
>         for (pos = list_first_entry(head, typeof(*pos), member);        \
> -            !list_entry_is_head(pos, head, member);                    \
> +           ({ bool _cond = !list_entry_is_head(pos, head, member);     \
> +            pos = select_nospec(_cond, pos, NULL); _cond; }); \
>              pos = list_next_entry(pos, member))

Actually I do have one ugly question about this:

Is NULL a good/safe choice here?

We already know that CPUs try very aggressively to do store-to-load
forwarding. Until now, my mental model of store-to-load forwarding was
basically: "The CPU has to guess whether the linear addresses will be
the same, and once it knows the linear addresses, it can verify
whether that guess was correct."

But of course that can't really be the whole mechanism, because many
architectures guarantee that if you access the same physical page
through multiple linear addresses, everything stays coherent. So I'm
wondering: Are there basically two stages of speculation based on
address guesses? A first stage where the CPU guesses whether the
linear addresses are the same, and a second stage where it assumes
that different linear addresses also map to different physical
addresses, or something like that?

And so, if we don't have a TLB entry for NULL, and we misspeculate
through a speculative write to an object of type A at NULL and then a
speculative read (at the same offset) from an object of type B at
NULL, will we get speculative type confusion through the nonexistent
object at NULL that lasts until either the branches are resolved or
the page walk for NULL reports back that there is no page at NULL?

(Also, it's been known for a long time that speculative accesses to
NULL can be a performance problem, too:
https://lwn.net/Articles/444336/)

So I'm wondering whether, on 64-bit architectures that have canonical
address bits, it would be safer and also reduce the amount of useless
pagetable walks to try to butcher up the canonical bits of the address
somehow so that the CPU can quickly see that the access is bogus,
without potentially having to do a pagetable walk first.

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop
  2022-02-17 19:28   ` Linus Torvalds
@ 2022-02-23 14:13     ` Jakob
  2022-02-23 14:16       ` Jakob
  2022-02-23 18:47       ` Linus Torvalds
  0 siblings, 2 replies; 70+ messages in thread
From: Jakob @ 2022-02-23 14:13 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Linux Kernel Mailing List, linux-arch, Greg Kroah-Hartman,
	Thomas Gleixner, Arnd Bergman, Andy Shevchenko, Andrew Morton,
	Kees Cook, Mike Rapoport, Gustavo A. R. Silva,
	Brian Johannesmeyer, Cristiano Giuffrida, Bos, H.J.



> On 17. Feb 2022, at 20:28, Linus Torvalds <torvalds@linux-foundation.org> wrote:
> 
> On Thu, Feb 17, 2022 at 10:50 AM Jakob Koschel <jakobkoschel@gmail.com> wrote:
>> 
>> It is unsafe to assume that &req->req != _req can only evaluate
>> to false if the break within the list iterator is hit.
> 
> I don't understand what problem you are trying to fix.
> 
> Since "req" absolutely *has* to be stable for any of this code to be
> valid, then "&req->req" is stable and unambiguous too. The *only* way
> _req can point to it would be if we finished the iteration properly.
> 
> So I don't see the unsafeness.
> 
> Note that all this work with "speculative" execution fundamentally CAN
> NOT affect semantics of the code, yet this patch makes statements
> about exactly that.

I'm sorry for having created the confusion. I made this patch to support
the speculative safe list_for_each_entry() version but it is not actually
related to that. I do believe that this an actual bug and *could*
*potentially* be misused. I'll follow up with an example to illustrate that.

I agree that this has nothing to do with the speculative execution iterator
(apart from making it work) and should best be discussed separately.

I'll attach an example on how I think this code *can* become a problem.
Note that this highly depends on the used compiler and how the struct
offsets are laid out.

> 
> That's not how CPU speculation works.
> 
> CPU speculation can expose hidden information that is not
> "semantically important" (typically through cache access patterns, but
> that's not the only way). So it might be exposing information it
> shouldn't.
> 
> But if speculation is actually changing semantics, then it's no longer
> "speculation" - it's just a bug, plain and simple (either a software
> bug due to insufficient serialization, or an actual hardware bug).
> 
> IOW, I don't want to see these kinds of apparently pointless changes
> to list walking. The patches should explain what that SECONDARY hidden
> value you try to protect actually is for each case.
> 
> This patch is basically not sensible. It just moves code around in a
> way that the compiler could have done anyway (or the compiler could
> decide to undo). It doesn't explain what the magic protected value is
> that shouldn't be leaked, and it leaves the code just looking odd and
> pointless.
> 
>                   Linus


^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop
  2022-02-23 14:13     ` Jakob
@ 2022-02-23 14:16       ` Jakob
  2022-02-24 10:33         ` Greg Kroah-Hartman
       [not found]         ` <6d191223d93249a98511177d4af08420@pexch012b.vu.local>
  2022-02-23 18:47       ` Linus Torvalds
  1 sibling, 2 replies; 70+ messages in thread
From: Jakob @ 2022-02-23 14:16 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Linux Kernel Mailing List, linux-arch, Greg Kroah-Hartman,
	Thomas Gleixner, Arnd Bergman, Andy Shevchenko, Andrew Morton,
	Kees Cook, Mike Rapoport, Gustavo A. R. Silva,
	Brian Johannesmeyer, Cristiano Giuffrida, Bos, H.J.

Note that I changed the location of the struct member 'req' in gr_request
to make this work. Instead of reshuffling struct members this can also be
introduced by simply adding new struct members in certain spots.
In other code locations with the same pattern I didn't have to do that.

Assuming '_req' passed to gr_dequeue() is located just past 'ep' on the
heap, the check could pass even though the list searched is completely
empty.

&req->req for the head element will be an out-of-bounds pointer
that by coincidence or heap massaging is where '_req' is located.

Even if the list is empty the list_for_each_entry() macro will do:

	pos = list_first_entry(head, typeof(*pos), member);

resolving all the macros (list_first_entry() etc) it will look like this:

	pos = container_of(head->next, typeof(*pos), member)

Since the list is empty head->next == head and container_of() is called on something
that is *not* actually of type gr_request.

Next, the check if the end of the loop is hit is evaluated:

	!list_entry_is_head(pos, head, member);

which will stop the loop directly before executing a single iteration.

then using '&req->req' is some bogus pointer pointing just past the struct gr_ep,
where in this case '_req' is located.

The point I'm trying to make: it's probably not safe to rely on the compiler and
that everyone is aware of this risk when adding/removing/reordering struct members.


Signed-off-by: Jakob Koschel <jakobkoschel@gmail.com>
---
drivers/usb/gadget/udc/gr_udc.c | 25 +++++++++++++++++++++++++
drivers/usb/gadget/udc/gr_udc.h |  2 +-
2 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/udc/gr_udc.c b/drivers/usb/gadget/udc/gr_udc.c
index 4b35739d3695..29c662f28428 100644
--- a/drivers/usb/gadget/udc/gr_udc.c
+++ b/drivers/usb/gadget/udc/gr_udc.c
@@ -1718,6 +1718,7 @@ static int gr_dequeue(struct usb_ep *_ep, struct usb_request *_req)
		ret = -EINVAL;
		goto out;
	}
+	printk(KERN_INFO "JKL: This does print, but shouldn't");

	if (list_first_entry(&ep->queue, struct gr_request, queue) == req) {
		/* This request is currently being processed */
@@ -1739,6 +1740,30 @@ static int gr_dequeue(struct usb_ep *_ep, struct usb_request *_req)
	return ret;
}

+static int __init init_test_jkl3(void)
+{
+	struct gr_ep *ep;
+	struct gr_udc *dev;
+	struct usb_request *_req;
+	void *buffer;
+
+	/* assume the usb_request struct is located just after the 'ep' on the heap */
+	buffer = kzalloc(sizeof(struct gr_ep)+sizeof(struct usb_request), GFP_KERNEL);
+	ep = buffer;
+	_req = buffer+sizeof(struct gr_ep);
+
+	/* setup to call gr_dequeue() */
+	dev = kzalloc(sizeof(struct gr_udc), GFP_KERNEL);
+	ep->dev = dev;
+	ep->dev->driver = 1;
+	INIT_LIST_HEAD(&ep->queue); /* list is empty */
+
+	gr_dequeue(&ep->ep, _req);
+
+	return 0;
+}
+__initcall(init_test_jkl3);
+
/* Helper for gr_set_halt and gr_set_wedge */
static int gr_set_halt_wedge(struct usb_ep *_ep, int halt, int wedge)
{
diff --git a/drivers/usb/gadget/udc/gr_udc.h b/drivers/usb/gadget/udc/gr_udc.h
index 70134239179e..14a18d5b5cf8 100644
--- a/drivers/usb/gadget/udc/gr_udc.h
+++ b/drivers/usb/gadget/udc/gr_udc.h
@@ -159,7 +159,6 @@ struct gr_ep {
};

struct gr_request {
-	struct usb_request req;
	struct list_head queue;

	/* Chain of dma descriptors */
@@ -171,6 +170,7 @@ struct gr_request {
	u16 oddlen; /* Size of odd length tail if buffer length is "odd" */

	u8 setup; /* Setup packet */
+	struct usb_request req;
};

enum gr_ep0state {
-- 
2.25.1

^ permalink raw reply related	[flat|nested] 70+ messages in thread

* Re: [RFC PATCH 04/13] vfio/mdev: remove the usage of the list iterator after the loop
  2022-02-18 15:12   ` Jason Gunthorpe
@ 2022-02-23 14:18     ` Jakob
  2022-02-23 19:06       ` Linus Torvalds
  0 siblings, 1 reply; 70+ messages in thread
From: Jakob @ 2022-02-23 14:18 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: Linus Torvalds, Linux Kernel Mailing List, linux-arch,
	Greg Kroah-Hartman, Thomas Gleixner, Arnd Bergman,
	Andy Shevchenko, Andrew Morton, Kees Cook, Mike Rapoport,
	Gustavo A. R. Silva, Brian Johannesmeyer, Cristiano Giuffrida,
	Bos, H.J.



> On 18. Feb 2022, at 16:12, Jason Gunthorpe <jgg@ziepe.ca> wrote:
> 
> On Thu, Feb 17, 2022 at 07:48:20PM +0100, Jakob Koschel wrote:
>> It is unsafe to assume that tmp != mdev can only evaluate to false
>> if the break within the list iterator is hit.
>> 
>> When the break is not hit, tmp is set to an address derived from the
>> head element. If mdev would match with that value of tmp it would allow
>> continuing beyond the safety check even if mdev was never found within
>> the list
> 
> I think due to other construction this is not actually possible, but I
> guess it is technically correct
> 
> This seems like just a straight up style fix with nothing to do with
> speculative execution though. Why not just send it as a proper patch?
> 
> Jason

Thank you for your feedback.

I've raised some confusion here, I'm sorry about that.
The idea was to change list_for_each_entry() to set 'pos' to NULL
when the list terminates to avoid invalid usage in speculation.

This will break this code and I therefore included the suggested change
in this RFC. This RFC was not intended to be merged as is.

However, in this example, 'tmp' will be a out-of-bounds pointer
if the loop did finish without hitting the break, so the check past the
loop *could* match 'mdev' even though no break was ever met.

I've now realized that this is probably not realistic iff mdev always
points to a valid struct mdev_device.
(It's a slightly different scenario on PATCH 03/13).

Jakob

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [RFC PATCH 01/13] list: introduce speculative safe list_for_each_entry()
  2022-02-18 16:29   ` Jann Horn
@ 2022-02-23 14:32     ` Jakob
  0 siblings, 0 replies; 70+ messages in thread
From: Jakob @ 2022-02-23 14:32 UTC (permalink / raw)
  To: Jann Horn
  Cc: Linus Torvalds, Linux Kernel Mailing List, linux-arch,
	Greg Kroah-Hartman, Thomas Gleixner, Arnd Bergman,
	Andy Shevchenko, Andrew Morton, Kees Cook, Mike Rapoport,
	Gustavo A. R. Silva, Brian Johannesmeyer, Cristiano Giuffrida,
	Bos, H.J.



> On 18. Feb 2022, at 17:29, Jann Horn <jannh@google.com> wrote:
> 
> On Thu, Feb 17, 2022 at 7:48 PM Jakob Koschel <jakobkoschel@gmail.com> wrote:
>> list_for_each_entry() selects either the correct value (pos) or a safe
>> value for the additional mispredicted iteration (NULL) for the list
>> iterator.
>> list_for_each_entry() calls select_nospec(), which performs
>> a branch-less select.
>> 
>> On x86, this select is performed via a cmov. Otherwise, it's performed
>> via various shift/mask/etc. operations.
>> 
>> Kasper Acknowledgements: Jakob Koschel, Brian Johannesmeyer, Kaveh
>> Razavi, Herbert Bos, Cristiano Giuffrida from the VUSec group at VU
>> Amsterdam.
>> 
>> Co-developed-by: Brian Johannesmeyer <bjohannesmeyer@gmail.com>
>> Signed-off-by: Brian Johannesmeyer <bjohannesmeyer@gmail.com>
>> Signed-off-by: Jakob Koschel <jakobkoschel@gmail.com>
> 
> Yeah, I think this is the best way to do this without deeply intrusive
> changes to how lists are represented in memory.
> 
> Some notes on the specific implementation:
> 
>> arch/x86/include/asm/barrier.h | 12 ++++++++++++
>> include/linux/list.h           |  3 ++-
>> include/linux/nospec.h         | 16 ++++++++++++++++
>> 3 files changed, 30 insertions(+), 1 deletion(-)
>> 
>> diff --git a/arch/x86/include/asm/barrier.h b/arch/x86/include/asm/barrier.h
>> index 35389b2af88e..722797ad74e2 100644
>> --- a/arch/x86/include/asm/barrier.h
>> +++ b/arch/x86/include/asm/barrier.h
>> @@ -48,6 +48,18 @@ static inline unsigned long array_index_mask_nospec(unsigned long index,
>> /* Override the default implementation from linux/nospec.h. */
>> #define array_index_mask_nospec array_index_mask_nospec
>> 
>> +/* Override the default implementation from linux/nospec.h. */
>> +#define select_nospec(cond, exptrue, expfalse)                         \
>> +({                                                                     \
>> +       typeof(exptrue) _out = (exptrue);                               \
>> +                                                                       \
>> +       asm volatile("test %1, %1\n\t"                                  \
> 
> This shouldn't need "volatile", because it is only necessary if _out
> is actually used. Using "volatile" here could prevent optimizing out
> useless code. OPTIMIZER_HIDE_VAR() also doesn't use "volatile".
> 
>> +           "cmove %2, %0"                                              \
>> +           : "+r" (_out)                                               \
>> +           : "r" (cond), "r" (expfalse));                              \
>> +       _out;                                                           \
>> +})
> 
> I guess the idea is probably to also add code like this for other
> important architectures, in particular arm64?

yes indeed, with a fallback of using the shifting/masking mechanism for
other archs.

> 
> 
> It might also be a good idea to rename the arch-overridable macro to
> something like "arch_select_nospec" and then have a wrapper macro in
> include/linux/nospec.h that takes care of type safety issues.
> 
> The current definition of the macro doesn't warn if you pass in
> incompatible pointer types, like this:
> 
> int *bogus_pointer_mix(int cond, int *a, long *b) {
>  return select_nospec(cond, a, b);
> }
> 
> and if you pass in integers of different sizes, it may silently
> truncate to the size of the smaller one - this C code:
> 
> long wrong_int_conversion(int cond, int a, long b) {
>  return select_nospec(cond, a, b);
> }
> 
> generates this assembly:
> 
> wrong_int_conversion:
>  test %edi, %edi
>  cmove %rdx, %esi
>  movslq %esi, %rax
>  ret
> 
> It might be a good idea to add something like a
> static_assert(__same_type(...), ...) to protect against that.

These are good points, thank you for your input. Will be good to incorporate.
> 
>> /* Prevent speculative execution past this barrier. */
>> #define barrier_nospec() alternative("", "lfence", X86_FEATURE_LFENCE_RDTSC)
>> 
>> diff --git a/include/linux/list.h b/include/linux/list.h
>> index dd6c2041d09c..1a1b39fdd122 100644
>> --- a/include/linux/list.h
>> +++ b/include/linux/list.h
>> @@ -636,7 +636,8 @@ static inline void list_splice_tail_init(struct list_head *list,
>>  */
>> #define list_for_each_entry(pos, head, member)                         \
>>        for (pos = list_first_entry(head, typeof(*pos), member);        \
>> -            !list_entry_is_head(pos, head, member);                    \
>> +           ({ bool _cond = !list_entry_is_head(pos, head, member);     \
>> +            pos = select_nospec(_cond, pos, NULL); _cond; }); \
>>             pos = list_next_entry(pos, member))
> 
> I wonder if it'd look nicer to write it roughly like this:
> 
> #define NOSPEC_TYPE_CHECK(_guarded_var, _cond)                  \
> ({                                                              \
>  bool __cond = (_cond);                                        \
>  typeof(_guarded_var) *__guarded_var = &(_guarded_var);        \
>  *__guarded_var = select_nospec(__cond, *__guarded_var, NULL); \
>  __cond;                                                       \
> })
> 
> #define list_for_each_entry(pos, head, member)                                \
>        for (pos = list_first_entry(head, typeof(*pos), member);              \
>             NOSPEC_TYPE_CHECK(head, !list_entry_is_head(pos, head, member)); \
>             pos = list_next_entry(pos, member))
> 
> I think having a NOSPEC_TYPE_CHECK() like this makes it semantically
> clearer, and easier to add in other places? But I don't know if the
> others agree...

That sounds like a good idea. I wonder if the pointer and dereference in 
NOSPEC_TYPE_CHECK() simply get optimized away. Or why you can't simply
use _guarded_var directly instead of a pointer to it.

> 
>> /**
>> diff --git a/include/linux/nospec.h b/include/linux/nospec.h
>> index c1e79f72cd89..ca8ed81e4f9e 100644
>> --- a/include/linux/nospec.h
>> +++ b/include/linux/nospec.h
>> @@ -67,4 +67,20 @@ int arch_prctl_spec_ctrl_set(struct task_struct *task, unsigned long which,
>> /* Speculation control for seccomp enforced mitigation */
>> void arch_seccomp_spec_mitigate(struct task_struct *task);
>> 
>> +/**
>> + * select_nospec - select a value without using a branch; equivalent to:
>> + * cond ? exptrue : expfalse;
>> + */
>> +#ifndef select_nospec
>> +#define select_nospec(cond, exptrue, expfalse)                         \
>> +({                                                                     \
>> +       unsigned long _t = (unsigned long) (exptrue);                   \
>> +       unsigned long _f = (unsigned long) (expfalse);                  \
>> +       unsigned long _c = (unsigned long) (cond);                      \
>> +       OPTIMIZER_HIDE_VAR(_c);                                         \
>> +       unsigned long _m = -((_c | -_c) >> (BITS_PER_LONG - 1));        \
>> +       (typeof(exptrue)) ((_t & _m) | (_f & ~_m));                     \
>> +})
>> +#endif
> 
> (As a sidenote, it might be easier to implement a conditional zeroing
> primitive than a generic conditional select primitive if that's all
> you need, something like:
> 
> #define cond_nullptr_nospec(_cond, _exp)          \
> ({                                             \
>  unsigned long __exp = (unsigned long)(_exp); \
>  unsigned long _mask = 0UL - !(_cond);       \
>  OPTIMIZER_HIDE_VAR(_mask);                   \
>  (typeof(_exp)) (_mask & __exp);              \
> })
> 
> )

Ah yes, if NULL is actually the value to choose, this might be good enough.


^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop
  2022-02-23 14:13     ` Jakob
  2022-02-23 14:16       ` Jakob
@ 2022-02-23 18:47       ` Linus Torvalds
  2022-02-23 19:23         ` Linus Torvalds
  1 sibling, 1 reply; 70+ messages in thread
From: Linus Torvalds @ 2022-02-23 18:47 UTC (permalink / raw)
  To: Jakob, Arnd Bergmann
  Cc: Linux Kernel Mailing List, linux-arch, Greg Kroah-Hartman,
	Thomas Gleixner, Andy Shevchenko, Andrew Morton, Kees Cook,
	Mike Rapoport, Gustavo A. R. Silva, Brian Johannesmeyer,
	Cristiano Giuffrida, Bos, H.J.

[ Arnd was already on the participants, but I moved him from 'Cc:' to
'To:', just because I think this is once again tangentially related to
the whole "c99 base" thing ]

On Wed, Feb 23, 2022 at 6:13 AM Jakob <jakobkoschel@gmail.com> wrote:
>
> I'm sorry for having created the confusion. I made this patch to support
> the speculative safe list_for_each_entry() version but it is not actually
> related to that. I do believe that this an actual bug and *could*
> *potentially* be misused. I'll follow up with an example to illustrate that.

Ok, so this is just a regular bug, plain and simple.

The problem being that the list_for_each_entry() will iterate over
each list entry - but at the end of the loop it will not point at any
entry at all (it will have a pointer value that is related to the
*HEAD* of the list, but that is not necessarily the same kind of entry
that the list members are.

Honestly, I think this kind of fix should have been done entirely separately.

In fact, I think the change to list_for_each_entry() should have been
done not as "fix type speculation", but as a much more interesting
"fix the list iterators".

The whole reason this kind of non-speculative bug can happen is that
we historically didn't have C99-style "declare variables in loops". So
list_for_each_entry() - and all the other ones - fundamentally always
leaks the last HEAD entry out of the loop, simply because we couldn't
declare the iterator variable in the loop itself.

(And by "couldn't", I mean "without making for special syntax": we do
exactly that in "for_each_thread ()" and friends, but they have an
"end_for_each_thread()" thing at the end).

So what I'd personally *really* like to see would be for us to - once
again - look at using "-std=gnu99", and fix the whole "leak final
invalid pointer outside the loop".

Then the type speculation thing would be an entirely separate patch.

Because honestly, I kind of hate the completely random type
speculation patch. It fixes one particular type of loop, and not even
one that seems all that special.

But we still don't do "gnu99", because we had some odd problem with
some ancient gcc versions that broke documented initializers.

I honestly _thought_ we had gotten over that already. I think the
problem cases were gcc-4.9 and older, and now we require gcc-5.1, and
we could just use "--std=gnu99" for the kernel, and we could finally
start using variable declarations in for-statements.

Arnd - remind me, please.. Was there some other problem than just gcc-4.9?

                 Linus

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [RFC PATCH 04/13] vfio/mdev: remove the usage of the list iterator after the loop
  2022-02-23 14:18     ` Jakob
@ 2022-02-23 19:06       ` Linus Torvalds
  2022-02-23 19:12         ` Jason Gunthorpe
  0 siblings, 1 reply; 70+ messages in thread
From: Linus Torvalds @ 2022-02-23 19:06 UTC (permalink / raw)
  To: Jakob
  Cc: Jason Gunthorpe, Linux Kernel Mailing List, linux-arch,
	Greg Kroah-Hartman, Thomas Gleixner, Arnd Bergman,
	Andy Shevchenko, Andrew Morton, Kees Cook, Mike Rapoport,
	Gustavo A. R. Silva, Brian Johannesmeyer, Cristiano Giuffrida,
	Bos, H.J.

On Wed, Feb 23, 2022 at 6:18 AM Jakob <jakobkoschel@gmail.com> wrote:
>
> However, in this example, 'tmp' will be a out-of-bounds pointer
> if the loop did finish without hitting the break, so the check past the
> loop *could* match 'mdev' even though no break was ever met.

So just as context for others, since I was hit with the same confusion
and didn't see what the relevance was for type speculation, when these
patches seemed to be not about speculation at all.

The background for this is that the list_for_each_entry() will set the
iterator variable (here 'tmp') to be not the actual internal list
pointer, but the pointer to the containing type (which is the whole
'entry' part of the name, of course).

And that is all good and true, but it's true only *WITHIN* that loop.
At the exit condition, it will have hit the 'head' of the list, and
the type that contains the list head is *not* necessarily the same
type as the list entries.

So that's where the type confusion comes from: if you access the list
iterator outside the loop, and it could have fallen off the end of the
loop, the list iterator pointer is now not actually really a valid
pointer of that 'entry' type at all.

And as such, you not only can't dereference it, but you also shouldn't
even compare pointer values - because the pointer arithmetic that was
valid for loop entries is not valid for the HEAD entry that is
embedded in another type. So the pointer arithmetic might have turned
it into a pointer outside the real container of the HEAD, and might
actually match something else.

Now, there are a number of reasons I really dislike the current RFC
patch series, so I'm not claiming the patch is something we should
apply as-is, but I'm trying to clarify why Jakob is doing what he's
doing (because clearly I wasn't the only one taken  by surprise by
it).

The reasons I don't like it is:

 - patches like these are very random. And very hard to read (and very
easy to re-introduce the bug).

 - I think it conflates the non-speculative "use pointer of the wrong
type" issue like in this patch with the speculation

 - I'm not even convinced that 'list_for_each_entry()' is that special
wrt speculative type accesses, considering that we have other uses of
doubly linked list *everywhere* - and it can happen in a lot of other
situations anyway, so it all seems to be a bit ad hoc.

but I do think the problem is real.

So elsewhere I suggested that the fix to "you can't use the pointer
outside the loop" be made to literally disallow it (using C99 for-loop
variables seems the cleanest model), and have the compiler refuse to
touch code that tries to use the loop iterator outside.

And that is then entirely separate from the issue of actual
speculative accesses (but honestly, I think that's a "you have to
teach the compiler not to do them" issue, not a "let's randomly change
*one* of our loop walkers).

                Linus

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [RFC PATCH 04/13] vfio/mdev: remove the usage of the list iterator after the loop
  2022-02-23 19:06       ` Linus Torvalds
@ 2022-02-23 19:12         ` Jason Gunthorpe
  2022-02-23 19:31           ` Linus Torvalds
  0 siblings, 1 reply; 70+ messages in thread
From: Jason Gunthorpe @ 2022-02-23 19:12 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Jakob, Linux Kernel Mailing List, linux-arch, Greg Kroah-Hartman,
	Thomas Gleixner, Arnd Bergman, Andy Shevchenko, Andrew Morton,
	Kees Cook, Mike Rapoport, Gustavo A. R. Silva,
	Brian Johannesmeyer, Cristiano Giuffrida, Bos, H.J.

On Wed, Feb 23, 2022 at 11:06:03AM -0800, Linus Torvalds wrote:

> And as such, you not only can't dereference it, but you also shouldn't
> even compare pointer values - because the pointer arithmetic that was
> valid for loop entries is not valid for the HEAD entry that is
> embedded in another type. So the pointer arithmetic might have turned
> it into a pointer outside the real container of the HEAD, and might
> actually match something else.

Yes, this is what I had put together as well about this patch, and I
think it is OK as-is. In this case the list head is in the .bss of a
module so I don't think it is very likely that the type confused
container_of() will alias a kalloc result, but it is certainly
technically wrong as-is.

> So elsewhere I suggested that the fix to "you can't use the pointer
> outside the loop" be made to literally disallow it (using C99 for-loop
> variables seems the cleanest model), and have the compiler refuse to
> touch code that tries to use the loop iterator outside.

Oh yes, that would be really nice solution.

Jason 

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop
  2022-02-23 18:47       ` Linus Torvalds
@ 2022-02-23 19:23         ` Linus Torvalds
  2022-02-23 19:43           ` Linus Torvalds
                             ` (2 more replies)
  0 siblings, 3 replies; 70+ messages in thread
From: Linus Torvalds @ 2022-02-23 19:23 UTC (permalink / raw)
  To: Jakob, Arnd Bergmann
  Cc: Linux Kernel Mailing List, linux-arch, Greg Kroah-Hartman,
	Thomas Gleixner, Andy Shevchenko, Andrew Morton, Kees Cook,
	Mike Rapoport, Gustavo A. R. Silva, Brian Johannesmeyer,
	Cristiano Giuffrida, Bos, H.J.

On Wed, Feb 23, 2022 at 10:47 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Arnd - remind me, please.. Was there some other problem than just gcc-4.9?

Hmm. Interesting. I decided to just try it and see for the compiler I
have, and changing the gnu89 to gnu99 I get new warnings
(-Werror=shift-negative-value).

Very annoying.  Especially since negative values are in many contexts
actually *safer* than positive ones when used as a mask, because as
long as the top bit is set in 'int', if the end result is then
expanded to some wider type, the top bit stays set.

Example:

  unsigned long mask(unsigned long x)
  { return x & (~0 << 5); }

  unsigned long badmask(unsigned long x)
  { return x & (~0u << 5); }

One does it properly, the other is buggy.

Now, with an explicit "unsigned long" like this, some clueless
compiler person  might say "just use "~0ul", but that's completely
wrong - because quite often the type is *not* this visible, and the
signed version works *regardless* of type.

So this Werror=shift-negative-value warning seems to be actively
detrimental, and I'm not seeing the reason for it. Can somebody
explain the thinking for that stupid warning?

That said, we seem to only have two cases of it in the kernel, at
least by a x86-64 allmodconfig build. So we could examine the types
there, or we could just add '-Wno-shift-negative-value".

               Linus

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [RFC PATCH 04/13] vfio/mdev: remove the usage of the list iterator after the loop
  2022-02-23 19:12         ` Jason Gunthorpe
@ 2022-02-23 19:31           ` Linus Torvalds
  2022-02-23 20:15             ` Jakob
  2022-02-23 20:19             ` Rasmus Villemoes
  0 siblings, 2 replies; 70+ messages in thread
From: Linus Torvalds @ 2022-02-23 19:31 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: Jakob, Linux Kernel Mailing List, linux-arch, Greg Kroah-Hartman,
	Thomas Gleixner, Arnd Bergman, Andy Shevchenko, Andrew Morton,
	Kees Cook, Mike Rapoport, Gustavo A. R. Silva,
	Brian Johannesmeyer, Cristiano Giuffrida, Bos, H.J.

On Wed, Feb 23, 2022 at 11:12 AM Jason Gunthorpe <jgg@ziepe.ca> wrote:
>
> Yes, this is what I had put together as well about this patch, and I
> think it is OK as-is. In this case the list head is in the .bss of a
> module so I don't think it is very likely that the type confused
> container_of() will alias a kalloc result, but it is certainly
> technically wrong as-is.

I think that the pattern we should strive to use is not top use a
'bool' with the

 - initialize to false, and then in loop: do 'found = true;' if found

 - use the iterator variable if 'found'.

but instead strive to

 - either only use the iterator variable inside the loop

 - if you want to use it after the loop, have a externally visible
separate pointer initialized to NULL, and set it to the iterator
variable inside the loop

IOW, instead of having a variable that is a 'bool', just make that
variable _be_ the pointer you want. It's clearer, and it avoids using
the iterator variable outside the loop.

It also is likely to generate better code, because there are fewer
live variables - outside the loop now only uses that one variable,
rather than using the 'bool' variable _and_ the iterator variable.

               Linus

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop
  2022-02-23 19:23         ` Linus Torvalds
@ 2022-02-23 19:43           ` Linus Torvalds
  2022-02-23 20:24           ` Arnd Bergmann
  2022-02-26 12:42           ` Segher Boessenkool
  2 siblings, 0 replies; 70+ messages in thread
From: Linus Torvalds @ 2022-02-23 19:43 UTC (permalink / raw)
  To: Jakob, Arnd Bergmann
  Cc: Linux Kernel Mailing List, linux-arch, Greg Kroah-Hartman,
	Thomas Gleixner, Andy Shevchenko, Andrew Morton, Kees Cook,
	Mike Rapoport, Gustavo A. R. Silva, Brian Johannesmeyer,
	Cristiano Giuffrida, Bos, H.J.

On Wed, Feb 23, 2022 at 11:23 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> That said, we seem to only have two cases of it in the kernel, at
> least by a x86-64 allmodconfig build.

No, there's more of them, it's just that the build broke early enough
that I didn't see it.

Doing

        git grep '\(\(~0\)\|\(-1\)\) <<'

finds a number of them. Some of them have casts in front, so they
wouldn't necessarily trigger this issue, but it's not an entirely
uncommon pattern.

And as mentioned, I think it's a *good* pattern, in that it takes
advantage of the sign-extension of the top bit in any widening use,
when the type might not be obvious (in macros, or when accessing
members of unions or structures, or when using typedefs that hide the
actual type).

So I still think that warning is actively detrimental, and I'm
wondering why it was added (and why 'gnu99' enables it, but 'gnu89'
does not). There's presumably _some_ reason.

              Linus

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [RFC PATCH 07/13] udp_tunnel: remove the usage of the list iterator after the loop
  2022-02-17 18:48 ` [RFC PATCH 07/13] udp_tunnel: " Jakob Koschel
@ 2022-02-23 20:00   ` Christophe JAILLET
  2022-02-24  6:20     ` Dan Carpenter
  0 siblings, 1 reply; 70+ messages in thread
From: Christophe JAILLET @ 2022-02-23 20:00 UTC (permalink / raw)
  To: Jakob Koschel, Linus Torvalds, linux-kernel
  Cc: linux-arch, Greg Kroah-Hartman, Thomas Gleixner, Arnd Bergman,
	Andy Shevchenko, Andrew Morton, Kees Cook, Mike Rapoport,
	Gustavo A. R. Silva, Brian Johannesmeyer, Cristiano Giuffrida,
	Bos, H.J.,
	Dan Carpenter

Le 17/02/2022 à 19:48, Jakob Koschel a écrit :
> The usage of node->dev after the loop body is a legitimate type
> confusion if the break was not hit. It will compare an undefined
> memory location with dev that could potentially be equal. The value
> of node->dev in this case could either be a random struct member of the
> head element or an out-of-bounds value.
> 
> Therefore it is more safe to use the found variable. With the
> introduction of speculative safe list iterator this check could be
> replaced with if (!node).
> 
> Signed-off-by: Jakob Koschel <jakobkoschel@gmail.com>
> ---
>   net/ipv4/udp_tunnel_nic.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/net/ipv4/udp_tunnel_nic.c b/net/ipv4/udp_tunnel_nic.c
> index b91003538d87..c47f9fb36d29 100644
> --- a/net/ipv4/udp_tunnel_nic.c
> +++ b/net/ipv4/udp_tunnel_nic.c
> @@ -842,11 +842,14 @@ udp_tunnel_nic_unregister(struct net_device *dev, struct udp_tunnel_nic *utn)
>   	 */
>   	if (info->shared) {
>   		struct udp_tunnel_nic_shared_node *node, *first;
> +		bool found = false;
>   
>   		list_for_each_entry(node, &info->shared->devices, list)
> -			if (node->dev == dev)
> +			if (node->dev == dev) {
> +				found = true;
>   				break;
> -		if (node->dev != dev)
> +			}
> +		if (!found)
>   			return;
>   
>   		list_del(&node->list);

Hi,

just in case, see Dan Carpeter's patch for the same issue with another 
fix at:
https://lore.kernel.org/kernel-janitors/20220222134251.GA2271@kili/

CJ

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [RFC PATCH 04/13] vfio/mdev: remove the usage of the list iterator after the loop
  2022-02-23 19:31           ` Linus Torvalds
@ 2022-02-23 20:15             ` Jakob
  2022-02-23 20:22               ` Linus Torvalds
  2022-02-23 20:19             ` Rasmus Villemoes
  1 sibling, 1 reply; 70+ messages in thread
From: Jakob @ 2022-02-23 20:15 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Jason Gunthorpe, Linux Kernel Mailing List, linux-arch,
	Greg Kroah-Hartman, Thomas Gleixner, Arnd Bergman,
	Andy Shevchenko, Andrew Morton, Kees Cook, Mike Rapoport,
	Gustavo A. R. Silva, Brian Johannesmeyer, Cristiano Giuffrida,
	Bos, H.J.



> On 23. Feb 2022, at 20:31, Linus Torvalds <torvalds@linux-foundation.org> wrote:
> 
> On Wed, Feb 23, 2022 at 11:12 AM Jason Gunthorpe <jgg@ziepe.ca> wrote:
>> 
>> Yes, this is what I had put together as well about this patch, and I
>> think it is OK as-is. In this case the list head is in the .bss of a
>> module so I don't think it is very likely that the type confused
>> container_of() will alias a kalloc result, but it is certainly
>> technically wrong as-is.
> 
> I think that the pattern we should strive to use is not top use a
> 'bool' with the
> 
> - initialize to false, and then in loop: do 'found = true;' if found
> 
> - use the iterator variable if 'found'.
> 
> but instead strive to
> 
> - either only use the iterator variable inside the loop
> 
> - if you want to use it after the loop, have a externally visible
> separate pointer initialized to NULL, and set it to the iterator
> variable inside the loop

in such a case you would still have to set the iterator value to
NULL when reaching the terminating condition or am I missing something?

Since the iterator value will always be set to *something* when
calling list_for_each_entry().

> 
> IOW, instead of having a variable that is a 'bool', just make that
> variable _be_ the pointer you want. It's clearer, and it avoids using
> the iterator variable outside the loop.

I completely agree and was intending to do it in such a way.
However I couldn't find a way to do that without breaking the kernel
between commits. 

Either you need to first set the iterator to NULL when terminating
which breaks the current code here

or

patch the code location first to do if(iterator), which does not
work with the current list_for_each_entry().


> 
> It also is likely to generate better code, because there are fewer
> live variables - outside the loop now only uses that one variable,
> rather than using the 'bool' variable _and_ the iterator variable.
> 
>               Linus


^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [RFC PATCH 04/13] vfio/mdev: remove the usage of the list iterator after the loop
  2022-02-23 19:31           ` Linus Torvalds
  2022-02-23 20:15             ` Jakob
@ 2022-02-23 20:19             ` Rasmus Villemoes
  2022-02-23 20:34               ` Linus Torvalds
  1 sibling, 1 reply; 70+ messages in thread
From: Rasmus Villemoes @ 2022-02-23 20:19 UTC (permalink / raw)
  To: Linus Torvalds, Jason Gunthorpe
  Cc: Jakob, Linux Kernel Mailing List, linux-arch, Greg Kroah-Hartman,
	Thomas Gleixner, Arnd Bergman, Andy Shevchenko, Andrew Morton,
	Kees Cook, Mike Rapoport, Gustavo A. R. Silva,
	Brian Johannesmeyer, Cristiano Giuffrida, Bos, H.J.

On 23/02/2022 20.31, Linus Torvalds wrote:
> On Wed, Feb 23, 2022 at 11:12 AM Jason Gunthorpe <jgg@ziepe.ca> wrote:
>>
>> Yes, this is what I had put together as well about this patch, and I
>> think it is OK as-is. In this case the list head is in the .bss of a
>> module so I don't think it is very likely that the type confused
>> container_of() will alias a kalloc result, but it is certainly
>> technically wrong as-is.
> 
> I think that the pattern we should strive to use is not top use a
> 'bool' with the
> 
>  - initialize to false, and then in loop: do 'found = true;' if found
> 
>  - use the iterator variable if 'found'.
> 
> but instead strive to
> 
>  - either only use the iterator variable inside the loop
> 
>  - if you want to use it after the loop, have a externally visible
> separate pointer initialized to NULL, and set it to the iterator
> variable inside the loop
> 
> IOW, instead of having a variable that is a 'bool', just make that
> variable _be_ the pointer you want. It's clearer, and it avoids using
> the iterator variable outside the loop.
> 
> It also is likely to generate better code, because there are fewer
> live variables - outside the loop now only uses that one variable,
> rather than using the 'bool' variable _and_ the iterator variable.

I have often wished that the iterator macros would consistently set the
loop variable to NULL upon reaching the end. It could be done by
changing the stop condition from

  i != HEAD

to

  i != HEAD || (i = NULL, 0)

[the comma operator mostly for clarity, we want the side effect of the
assignment but the whole RHS to still evaluate to false, which "i=NULL"
by itself of course would].

In the vast majority of cases where the iterator variable is not used
after the loop, the compiler should see through it and just drop the
dead assignment. And it would make the test for "did we break out early
finding what we looked for, or did we iterate till the end" quite
natural, without any auxiliary bool or extra 'struct foo*' variable. But
it will probably break too much existing code.

Rasmus

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [RFC PATCH 04/13] vfio/mdev: remove the usage of the list iterator after the loop
  2022-02-23 20:15             ` Jakob
@ 2022-02-23 20:22               ` Linus Torvalds
  2022-02-23 22:08                 ` Jakob
  0 siblings, 1 reply; 70+ messages in thread
From: Linus Torvalds @ 2022-02-23 20:22 UTC (permalink / raw)
  To: Jakob
  Cc: Jason Gunthorpe, Linux Kernel Mailing List, linux-arch,
	Greg Kroah-Hartman, Thomas Gleixner, Arnd Bergman,
	Andy Shevchenko, Andrew Morton, Kees Cook, Mike Rapoport,
	Gustavo A. R. Silva, Brian Johannesmeyer, Cristiano Giuffrida,
	Bos, H.J.

On Wed, Feb 23, 2022 at 12:15 PM Jakob <jakobkoschel@gmail.com> wrote:
>
> in such a case you would still have to set the iterator value to
> NULL when reaching the terminating condition or am I missing something?

No.

Make the rule be "you never use the iterator outside the loop".

IOW, the code sequence is

        some_struct *ptr, *iter;

        ptr = NULL;
        list_for_each_entry(iter, ...) {
                if (iter_matches_condition(iter)) {
                        ptr = iter;
                        break;
                }
        }

        .. never use 'iter' here - you use 'ptr' and check it for NULL ..

See? Same number of variables as using a separate 'bool found' flag,
but simpler code, and it matches the rule of 'don't use iter outside
the loop'.

This is how you'd have to do it anyway if we start using a C99 style
'declare iter _in_ the loop' model.

And as mentioned, it actually tends to lead to better code, since the
code outside the loop only has one variable live, not two.

Of course, compilers can do a lot of optimizations, so a 'found'
variable can be made to generate good code too - if the compiler just
tracks it and notices, and turns the 'break' into a 'goto found', and
the fallthrough into the 'goto not_found'.

So 'better code generation' is debatable, but even if the compiler can
do as good a job with a separate 'bool' variable and some cleverness,
I think we should strive for code where we make it easy for the
compiler to DTRT - and where the generated code is easier to match up
with what we wrote.

                  Linus

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop
  2022-02-23 19:23         ` Linus Torvalds
  2022-02-23 19:43           ` Linus Torvalds
@ 2022-02-23 20:24           ` Arnd Bergmann
  2022-02-23 20:43             ` Linus Torvalds
  2022-02-26 12:42           ` Segher Boessenkool
  2 siblings, 1 reply; 70+ messages in thread
From: Arnd Bergmann @ 2022-02-23 20:24 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Jakob, Arnd Bergmann, Linux Kernel Mailing List, linux-arch,
	Greg Kroah-Hartman, Thomas Gleixner, Andy Shevchenko,
	Andrew Morton, Kees Cook, Mike Rapoport, Gustavo A. R. Silva,
	Brian Johannesmeyer, Cristiano Giuffrida, Bos, H.J.

On Wed, Feb 23, 2022 at 8:23 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Wed, Feb 23, 2022 at 10:47 AM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > Arnd - remind me, please.. Was there some other problem than just gcc-4.9?


I'm pretty sure this was the only thing. And not  even because gcc-4.9 didn't
support later standards, but because it caused some false-postivee
warnings like

In file included from ../drivers/usb/core/notify.c:16:0:
../include/linux/notifier.h:117:9: error: initializer element is not constant
  struct blocking_notifier_head name =   \
         ^
../drivers/usb/core/notify.c:21:8: note: in expansion of macro
'BLOCKING_NOTIFIER_HEAD'
 static BLOCKING_NOTIFIER_HEAD(usb_notifier_list);
        ^
../include/linux/notifier.h:117:9: error: (near initialization for
'usb_notifier_list.rwsem.wait_lock')
  struct blocking_notifier_head name =   \
         ^
../drivers/usb/core/notify.c:21:8: note: in expansion of macro
'BLOCKING_NOTIFIER_HEAD'
 static BLOCKING_NOTIFIER_HEAD(usb_notifier_list);
        ^

> Hmm. Interesting. I decided to just try it and see for the compiler I
> have, and changing the gnu89 to gnu99 I get new warnings
> (-Werror=shift-negative-value).

FWIW, I think we can go straight to gnu11 in this case even with gcc-5, no
need to take gnu99 as an intermediate step. I don't know if there is a
practical difference between the two though.

gcc-8 and higher also support --std=gnu18 and --std=gnu2x, which also
work for building the kernel but would break gcc-5/6/7 support

> Very annoying.  Especially since negative values are in many contexts
> actually *safer* than positive ones when used as a mask, because as
> long as the top bit is set in 'int', if the end result is then
> expanded to some wider type, the top bit stays set.
>
> Example:
>
>   unsigned long mask(unsigned long x)
>   { return x & (~0 << 5); }
>
>   unsigned long badmask(unsigned long x)
>   { return x & (~0u << 5); }
>
> One does it properly, the other is buggy.
>
> Now, with an explicit "unsigned long" like this, some clueless
> compiler person  might say "just use "~0ul", but that's completely
> wrong - because quite often the type is *not* this visible, and the
> signed version works *regardless* of type.
>
> So this Werror=shift-negative-value warning seems to be actively
> detrimental, and I'm not seeing the reason for it. Can somebody
> explain the thinking for that stupid warning?
>
> That said, we seem to only have two cases of it in the kernel, at
> least by a x86-64 allmodconfig build. So we could examine the types
> there, or we could just add '-Wno-shift-negative-value".

I looked at the gcc documentation for this flag, and it tells me that
it's default-enabled for std=c99 or higher. Turning it on for --std=gnu89
shows the same warning, so at least it doesn't sound like the actual
behavior changed, only the warning output. clang does not warn
for this code at all, regardless of the --std= flag.

        Arnd

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [RFC PATCH 04/13] vfio/mdev: remove the usage of the list iterator after the loop
  2022-02-23 20:19             ` Rasmus Villemoes
@ 2022-02-23 20:34               ` Linus Torvalds
  0 siblings, 0 replies; 70+ messages in thread
From: Linus Torvalds @ 2022-02-23 20:34 UTC (permalink / raw)
  To: Rasmus Villemoes
  Cc: Jason Gunthorpe, Jakob, Linux Kernel Mailing List, linux-arch,
	Greg Kroah-Hartman, Thomas Gleixner, Arnd Bergman,
	Andy Shevchenko, Andrew Morton, Kees Cook, Mike Rapoport,
	Gustavo A. R. Silva, Brian Johannesmeyer, Cristiano Giuffrida,
	Bos, H.J.

On Wed, Feb 23, 2022 at 12:19 PM Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
>
> I have often wished that the iterator macros would consistently set the
> loop variable to NULL upon reaching the end.

I really think the rule should be that to a 99% approximation, we
should strive only ever use the iterated-upon value *inside* the loop.

No, that's now how we do it now. But I think the "break out and do the
work outside the loop" case is kind of broken anyway. It makes you
test the condition twice - and while a compiler might be smart enough
to optimize the second test away, it's still just plain ugly.

             Linus

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop
  2022-02-23 20:24           ` Arnd Bergmann
@ 2022-02-23 20:43             ` Linus Torvalds
  2022-02-23 20:48               ` Arnd Bergmann
  2022-02-23 20:54               ` Linus Torvalds
  0 siblings, 2 replies; 70+ messages in thread
From: Linus Torvalds @ 2022-02-23 20:43 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Jakob, Linux Kernel Mailing List, linux-arch, Greg Kroah-Hartman,
	Thomas Gleixner, Andy Shevchenko, Andrew Morton, Kees Cook,
	Mike Rapoport, Gustavo A. R. Silva, Brian Johannesmeyer,
	Cristiano Giuffrida, Bos, H.J.

On Wed, Feb 23, 2022 at 12:25 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> I looked at the gcc documentation for this flag, and it tells me that
> it's default-enabled for std=c99 or higher. Turning it on for --std=gnu89
> shows the same warning, so at least it doesn't sound like the actual
> behavior changed, only the warning output. clang does not warn
> for this code at all, regardless of the --std= flag.

Ok, so we should be able to basically convert '--std=gnu89' into
'--std=gnu11 -Wno-shift-negative-value' with no expected change of
behavior.

Of course, maybe we need to make -Wno-shift-negative-value be
conditional on the compiler supporting it in the first place?

I really would love to finally move forward on this, considering that
it's been brewing for many many years.

I think the loop iterators are the biggest user-visible thing, but
there might be others.

And some googling seems to show that the reason for
-Wshift-negative-value is that with C99 the semantics changed for
targets that weren't two's complement. We *really* don't care.

Of course, the C standard being the bunch of incompetents they are,
they in the process apparently made left-shifts undefined (rather than
implementation-defined). Christ, they keep on making the same mistakes
over and over. What was the definition of insanity again?

                  Linus

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop
  2022-02-23 20:43             ` Linus Torvalds
@ 2022-02-23 20:48               ` Arnd Bergmann
  2022-02-23 21:53                 ` Linus Torvalds
  2022-02-23 20:54               ` Linus Torvalds
  1 sibling, 1 reply; 70+ messages in thread
From: Arnd Bergmann @ 2022-02-23 20:48 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Arnd Bergmann, Jakob, Linux Kernel Mailing List, linux-arch,
	Greg Kroah-Hartman, Thomas Gleixner, Andy Shevchenko,
	Andrew Morton, Kees Cook, Mike Rapoport, Gustavo A. R. Silva,
	Brian Johannesmeyer, Cristiano Giuffrida, Bos, H.J.,
	Nathan Chancellor

On Wed, Feb 23, 2022 at 9:43 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Wed, Feb 23, 2022 at 12:25 PM Arnd Bergmann <arnd@arndb.de> wrote:
> >
> > I looked at the gcc documentation for this flag, and it tells me that
> > it's default-enabled for std=c99 or higher. Turning it on for --std=gnu89
> > shows the same warning, so at least it doesn't sound like the actual
> > behavior changed, only the warning output. clang does not warn
> > for this code at all, regardless of the --std= flag.
>
> Ok, so we should be able to basically convert '--std=gnu89' into
> '--std=gnu11 -Wno-shift-negative-value' with no expected change of
> behavior.

Yes, I think that is correct.

> Of course, maybe we need to make -Wno-shift-negative-value be
> conditional on the compiler supporting it in the first place?

I think they all do. I discussed this with Nathan Chancellor on IRC, to
see what clang does, and he points out that the warning was made
conditional on -fwrapv there a while ago:

https://github.com/llvm/llvm-project/commit/59802321785b4b9fc31b10456c62ba3a06d3a631

So the normal behavior on clang is to always warn about it, but since
we explicitly ask for sane signed integer behavior, it doesn't warn for
the kernel.

        Arnd

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop
  2022-02-23 20:43             ` Linus Torvalds
  2022-02-23 20:48               ` Arnd Bergmann
@ 2022-02-23 20:54               ` Linus Torvalds
  2022-02-23 22:21                 ` David Laight
  2022-02-25 21:36                 ` Uecker, Martin
  1 sibling, 2 replies; 70+ messages in thread
From: Linus Torvalds @ 2022-02-23 20:54 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Jakob, Linux Kernel Mailing List, linux-arch, Greg Kroah-Hartman,
	Thomas Gleixner, Andy Shevchenko, Andrew Morton, Kees Cook,
	Mike Rapoport, Gustavo A. R. Silva, Brian Johannesmeyer,
	Cristiano Giuffrida, Bos, H.J.

On Wed, Feb 23, 2022 at 12:43 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Of course, the C standard being the bunch of incompetents they are,
> they in the process apparently made left-shifts undefined (rather than
> implementation-defined). Christ, they keep on making the same mistakes
> over and over. What was the definition of insanity again?

Hey, some more googling on my part seems to say that somebody saw the
light, and it's likely getting fixed in newer C standard version.

So it was just a mistake, not actual malice. Maybe we can hope that
the tide is turning against the "undefined" crowd that used to rule
the roost in the C standards bodies. Maybe the fundamental security
issues with undefined behavior finally convinced people how bad it
was?

            Linus

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop
  2022-02-23 20:48               ` Arnd Bergmann
@ 2022-02-23 21:53                 ` Linus Torvalds
  2022-02-24 16:04                   ` Nathan Chancellor
  0 siblings, 1 reply; 70+ messages in thread
From: Linus Torvalds @ 2022-02-23 21:53 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Jakob, Linux Kernel Mailing List, linux-arch, Greg Kroah-Hartman,
	Thomas Gleixner, Andy Shevchenko, Andrew Morton, Kees Cook,
	Mike Rapoport, Gustavo A. R. Silva, Brian Johannesmeyer,
	Cristiano Giuffrida, Bos, H.J.,
	Nathan Chancellor

On Wed, Feb 23, 2022 at 1:46 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> > Ok, so we should be able to basically convert '--std=gnu89' into
> > '--std=gnu11 -Wno-shift-negative-value' with no expected change of
> > behavior.
>
> Yes, I think that is correct.

Ok, somebody please remind me, and let's just try this early in the
5.18 merge window.

Because at least for me, doing

-                  -std=gnu89
+                  -std=gnu11 -Wno-shift-negative-value

for KBUILD_CFLAGS works fine both in my gcc and clang builds. But
that's obviously just one version of each.

(I left the host compiler flags alone - I have this memory of us
having had issues with people having old host compilers and wanting
headers for tools still build with gcc-4)

                Linus

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [RFC PATCH 04/13] vfio/mdev: remove the usage of the list iterator after the loop
  2022-02-23 20:22               ` Linus Torvalds
@ 2022-02-23 22:08                 ` Jakob
  0 siblings, 0 replies; 70+ messages in thread
From: Jakob @ 2022-02-23 22:08 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Jason Gunthorpe, Linux Kernel Mailing List, linux-arch,
	Greg Kroah-Hartman, Thomas Gleixner, Arnd Bergman,
	Andy Shevchenko, Andrew Morton, Kees Cook, Mike Rapoport,
	Gustavo A. R. Silva, Brian Johannesmeyer, Cristiano Giuffrida,
	Bos, H.J.



> On 23. Feb 2022, at 21:22, Linus Torvalds <torvalds@linux-foundation.org> wrote:
> 
> On Wed, Feb 23, 2022 at 12:15 PM Jakob <jakobkoschel@gmail.com> wrote:
>> 
>> in such a case you would still have to set the iterator value to
>> NULL when reaching the terminating condition or am I missing something?
> 
> No.
> 
> Make the rule be "you never use the iterator outside the loop".
> 
> IOW, the code sequence is
> 
>        some_struct *ptr, *iter;

with C99 iter would be defined within the loop instead right?

> 
>        ptr = NULL;
>        list_for_each_entry(iter, ...) {
>                if (iter_matches_condition(iter)) {
>                        ptr = iter;
>                        break;
>                }
>        }
> 
>        .. never use 'iter' here - you use 'ptr' and check it for NULL ..
> 
> See? Same number of variables as using a separate 'bool found' flag,
> but simpler code, and it matches the rule of 'don't use iter outside
> the loop'.

ah yes this does make sense. I missed the part of using a separate
'ptr' variable. Thanks for clarifying.
I think this is a great idea.

There are cases where pos->member is used (the only legitimate way to
use it right now). I suppose those turn into something like this
(this example is inspired by dev_add_offload() (net/core/gro.c:38)):

       some_struct *ptr, *iter;
       list_head *list_ptr;

       ptr = NULL;
       list_for_each_entry(iter, head, list) {
               if (iter_matches_condition(iter)) {
                       ptr = iter;
                       break;
               }
       }
       

       if (ptr)
               list_ptr = head->prev;
       else
               list_ptr = iter->list.prev;
       list_add(..., list_ptr);

before it was simply
       list_add(..., iter->list.prev);


The other possibility I suppose would be:

       if (!ptr)
               ptr = container_of(head, typeof(*ptr), list)
       list_add(..., ptr->list.prev);

which leaves you with the same type confusion as before, being far from
ideal.

> This is how you'd have to do it anyway if we start using a C99 style
> 'declare iter _in_ the loop' model.
> 
> And as mentioned, it actually tends to lead to better code, since the
> code outside the loop only has one variable live, not two.
> 
> Of course, compilers can do a lot of optimizations, so a 'found'
> variable can be made to generate good code too - if the compiler just
> tracks it and notices, and turns the 'break' into a 'goto found', and
> the fallthrough into the 'goto not_found'.
> 
> So 'better code generation' is debatable, but even if the compiler can
> do as good a job with a separate 'bool' variable and some cleverness,
> I think we should strive for code where we make it easy for the
> compiler to DTRT - and where the generated code is easier to match up
> with what we wrote.
> 
>                  Linus

If there is interest, I'm happy to send a new patch set once the fixes are clear.

	Jakob


^ permalink raw reply	[flat|nested] 70+ messages in thread

* RE: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop
  2022-02-23 20:54               ` Linus Torvalds
@ 2022-02-23 22:21                 ` David Laight
  2022-02-25 21:36                 ` Uecker, Martin
  1 sibling, 0 replies; 70+ messages in thread
From: David Laight @ 2022-02-23 22:21 UTC (permalink / raw)
  To: 'Linus Torvalds', Arnd Bergmann
  Cc: Jakob, Linux Kernel Mailing List, linux-arch, Greg Kroah-Hartman,
	Thomas Gleixner, Andy Shevchenko, Andrew Morton, Kees Cook,
	Mike Rapoport, Gustavo A. R. Silva, Brian Johannesmeyer,
	Cristiano Giuffrida, Bos, H.J.

From: Linus Torvalds
> Sent: 23 February 2022 20:55
> 
> On Wed, Feb 23, 2022 at 12:43 PM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > Of course, the C standard being the bunch of incompetents they are,
> > they in the process apparently made left-shifts undefined (rather than
> > implementation-defined). Christ, they keep on making the same mistakes
> > over and over. What was the definition of insanity again?
> 
> Hey, some more googling on my part seems to say that somebody saw the
> light, and it's likely getting fixed in newer C standard version.
> 
> So it was just a mistake, not actual malice. Maybe we can hope that
> the tide is turning against the "undefined" crowd that used to rule
> the roost in the C standards bodies. Maybe the fundamental security
> issues with undefined behavior finally convinced people how bad it
> was?

IIRC UB includes 'fire an ICBM at the writer of the standards document'.
There isn't an 'undefined value' or even 'undefined value or program trap'
option.

It also seems to me that the compiler people are picking on things
in the standard that are there to let 'obscure machines conform'
and then using them to break perfectly reasonable programs.

Signed arithmetic is not required to wrap so that cpu (eg DSP)
can do saturating maths - not so the compiler can remove some
conditionals.

	David

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

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [RFC PATCH 07/13] udp_tunnel: remove the usage of the list iterator after the loop
  2022-02-23 20:00   ` Christophe JAILLET
@ 2022-02-24  6:20     ` Dan Carpenter
  0 siblings, 0 replies; 70+ messages in thread
From: Dan Carpenter @ 2022-02-24  6:20 UTC (permalink / raw)
  To: Christophe JAILLET
  Cc: Jakob Koschel, Linus Torvalds, linux-kernel, linux-arch,
	Greg Kroah-Hartman, Thomas Gleixner, Arnd Bergman,
	Andy Shevchenko, Andrew Morton, Kees Cook, Mike Rapoport,
	Gustavo A. R. Silva, Brian Johannesmeyer, Cristiano Giuffrida,
	Bos, H.J.

On Wed, Feb 23, 2022 at 09:00:36PM +0100, Christophe JAILLET wrote:
> Le 17/02/2022 à 19:48, Jakob Koschel a écrit :
> > The usage of node->dev after the loop body is a legitimate type
> > confusion if the break was not hit. It will compare an undefined
> > memory location with dev that could potentially be equal. The value
> > of node->dev in this case could either be a random struct member of the
> > head element or an out-of-bounds value.
> > 
> > Therefore it is more safe to use the found variable. With the
> > introduction of speculative safe list iterator this check could be
> > replaced with if (!node).
> > 
> > Signed-off-by: Jakob Koschel <jakobkoschel@gmail.com>
> > ---
> >   net/ipv4/udp_tunnel_nic.c | 7 +++++--
> >   1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/net/ipv4/udp_tunnel_nic.c b/net/ipv4/udp_tunnel_nic.c
> > index b91003538d87..c47f9fb36d29 100644
> > --- a/net/ipv4/udp_tunnel_nic.c
> > +++ b/net/ipv4/udp_tunnel_nic.c
> > @@ -842,11 +842,14 @@ udp_tunnel_nic_unregister(struct net_device *dev, struct udp_tunnel_nic *utn)
> >   	 */
> >   	if (info->shared) {
> >   		struct udp_tunnel_nic_shared_node *node, *first;
> > +		bool found = false;
> >   		list_for_each_entry(node, &info->shared->devices, list)
> > -			if (node->dev == dev)
> > +			if (node->dev == dev) {
> > +				found = true;
> >   				break;
> > -		if (node->dev != dev)
> > +			}
> > +		if (!found)
> >   			return;
> >   		list_del(&node->list);
> 
> Hi,
> 
> just in case, see Dan Carpeter's patch for the same issue with another fix
> at:
> https://lore.kernel.org/kernel-janitors/20220222134251.GA2271@kili/

Yeah.  My patch was already applied.

I've had an unpublished Smatch check for this for a while but I've been
re-writing it recently to make it more generic so that it worked for
all the different list_for_each type macros.  I'm going to publish it
soon.

Of course, all the real bugs are fixed so the remaining warnings are
false positives.

regards,
dan carpenter

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop
  2022-02-23 14:16       ` Jakob
@ 2022-02-24 10:33         ` Greg Kroah-Hartman
  2022-02-24 17:56           ` Linus Torvalds
       [not found]         ` <6d191223d93249a98511177d4af08420@pexch012b.vu.local>
  1 sibling, 1 reply; 70+ messages in thread
From: Greg Kroah-Hartman @ 2022-02-24 10:33 UTC (permalink / raw)
  To: Jakob
  Cc: Linus Torvalds, Linux Kernel Mailing List, linux-arch,
	Thomas Gleixner, Arnd Bergman, Andy Shevchenko, Andrew Morton,
	Kees Cook, Mike Rapoport, Gustavo A. R. Silva,
	Brian Johannesmeyer, Cristiano Giuffrida, Bos, H.J.

On Wed, Feb 23, 2022 at 03:16:09PM +0100, Jakob wrote:
> Note that I changed the location of the struct member 'req' in gr_request
> to make this work. Instead of reshuffling struct members this can also be
> introduced by simply adding new struct members in certain spots.
> In other code locations with the same pattern I didn't have to do that.
> 
> Assuming '_req' passed to gr_dequeue() is located just past 'ep' on the
> heap, the check could pass even though the list searched is completely
> empty.
> 
> &req->req for the head element will be an out-of-bounds pointer
> that by coincidence or heap massaging is where '_req' is located.
> 
> Even if the list is empty the list_for_each_entry() macro will do:
> 
> 	pos = list_first_entry(head, typeof(*pos), member);
> 
> resolving all the macros (list_first_entry() etc) it will look like this:
> 
> 	pos = container_of(head->next, typeof(*pos), member)
> 
> Since the list is empty head->next == head and container_of() is called on something
> that is *not* actually of type gr_request.
> 
> Next, the check if the end of the loop is hit is evaluated:
> 
> 	!list_entry_is_head(pos, head, member);
> 
> which will stop the loop directly before executing a single iteration.
> 
> then using '&req->req' is some bogus pointer pointing just past the struct gr_ep,
> where in this case '_req' is located.
> 
> The point I'm trying to make: it's probably not safe to rely on the compiler and
> that everyone is aware of this risk when adding/removing/reordering struct members.
> 
> 
> Signed-off-by: Jakob Koschel <jakobkoschel@gmail.com>
> ---
> drivers/usb/gadget/udc/gr_udc.c | 25 +++++++++++++++++++++++++
> drivers/usb/gadget/udc/gr_udc.h |  2 +-
> 2 files changed, 26 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/gadget/udc/gr_udc.c b/drivers/usb/gadget/udc/gr_udc.c
> index 4b35739d3695..29c662f28428 100644
> --- a/drivers/usb/gadget/udc/gr_udc.c
> +++ b/drivers/usb/gadget/udc/gr_udc.c
> @@ -1718,6 +1718,7 @@ static int gr_dequeue(struct usb_ep *_ep, struct usb_request *_req)
> 		ret = -EINVAL;
> 		goto out;
> 	}
> +	printk(KERN_INFO "JKL: This does print, but shouldn't");
> 
> 	if (list_first_entry(&ep->queue, struct gr_request, queue) == req) {
> 		/* This request is currently being processed */
> @@ -1739,6 +1740,30 @@ static int gr_dequeue(struct usb_ep *_ep, struct usb_request *_req)
> 	return ret;
> }
> 
> +static int __init init_test_jkl3(void)
> +{
> +	struct gr_ep *ep;
> +	struct gr_udc *dev;
> +	struct usb_request *_req;
> +	void *buffer;
> +
> +	/* assume the usb_request struct is located just after the 'ep' on the heap */
> +	buffer = kzalloc(sizeof(struct gr_ep)+sizeof(struct usb_request), GFP_KERNEL);
> +	ep = buffer;
> +	_req = buffer+sizeof(struct gr_ep);
> +
> +	/* setup to call gr_dequeue() */
> +	dev = kzalloc(sizeof(struct gr_udc), GFP_KERNEL);
> +	ep->dev = dev;
> +	ep->dev->driver = 1;
> +	INIT_LIST_HEAD(&ep->queue); /* list is empty */
> +
> +	gr_dequeue(&ep->ep, _req);
> +
> +	return 0;
> +}
> +__initcall(init_test_jkl3);
> +
> /* Helper for gr_set_halt and gr_set_wedge */
> static int gr_set_halt_wedge(struct usb_ep *_ep, int halt, int wedge)
> {
> diff --git a/drivers/usb/gadget/udc/gr_udc.h b/drivers/usb/gadget/udc/gr_udc.h
> index 70134239179e..14a18d5b5cf8 100644
> --- a/drivers/usb/gadget/udc/gr_udc.h
> +++ b/drivers/usb/gadget/udc/gr_udc.h
> @@ -159,7 +159,6 @@ struct gr_ep {
> };
> 
> struct gr_request {
> -	struct usb_request req;
> 	struct list_head queue;
> 
> 	/* Chain of dma descriptors */
> @@ -171,6 +170,7 @@ struct gr_request {
> 	u16 oddlen; /* Size of odd length tail if buffer length is "odd" */
> 
> 	u8 setup; /* Setup packet */
> +	struct usb_request req;
> };
> 
> enum gr_ep0state {
> -- 
> 2.25.1

So, to follow the proposed solution in this thread, the following change
is the "correct" one to make, right?


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;
 	}

^ permalink raw reply related	[flat|nested] 70+ messages in thread

* Re: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop
       [not found]         ` <6d191223d93249a98511177d4af08420@pexch012b.vu.local>
@ 2022-02-24 10:46           ` Cristiano Giuffrida
  2022-02-24 11:26             ` Greg Kroah-Hartman
  0 siblings, 1 reply; 70+ messages in thread
From: Cristiano Giuffrida @ 2022-02-24 10:46 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Jakob, Linus Torvalds, Linux Kernel Mailing List, linux-arch,
	Thomas Gleixner, Arnd Bergman, Andy Shevchenko, Andrew Morton,
	Kees Cook, Mike Rapoport, Gustavo A. R. Silva,
	Brian Johannesmeyer, Bos, H.J.

I think the "problem" with this solution is that it doesn't prevent
`tmp` from being used outside the loop still (and people getting it
wrong again)? It would be good to have 'struct gr_request *tmp;' being
visible only inside the loop (i.e., declared in the macro).

On Thu, Feb 24, 2022 at 11:34 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Wed, Feb 23, 2022 at 03:16:09PM +0100, Jakob wrote:
> > Note that I changed the location of the struct member 'req' in gr_request
> > to make this work. Instead of reshuffling struct members this can also be
> > introduced by simply adding new struct members in certain spots.
> > In other code locations with the same pattern I didn't have to do that.
> >
> > Assuming '_req' passed to gr_dequeue() is located just past 'ep' on the
> > heap, the check could pass even though the list searched is completely
> > empty.
> >
> > &req->req for the head element will be an out-of-bounds pointer
> > that by coincidence or heap massaging is where '_req' is located.
> >
> > Even if the list is empty the list_for_each_entry() macro will do:
> >
> >       pos = list_first_entry(head, typeof(*pos), member);
> >
> > resolving all the macros (list_first_entry() etc) it will look like this:
> >
> >       pos = container_of(head->next, typeof(*pos), member)
> >
> > Since the list is empty head->next == head and container_of() is called on something
> > that is *not* actually of type gr_request.
> >
> > Next, the check if the end of the loop is hit is evaluated:
> >
> >       !list_entry_is_head(pos, head, member);
> >
> > which will stop the loop directly before executing a single iteration.
> >
> > then using '&req->req' is some bogus pointer pointing just past the struct gr_ep,
> > where in this case '_req' is located.
> >
> > The point I'm trying to make: it's probably not safe to rely on the compiler and
> > that everyone is aware of this risk when adding/removing/reordering struct members.
> >
> >
> > Signed-off-by: Jakob Koschel <jakobkoschel@gmail.com>
> > ---
> > drivers/usb/gadget/udc/gr_udc.c | 25 +++++++++++++++++++++++++
> > drivers/usb/gadget/udc/gr_udc.h |  2 +-
> > 2 files changed, 26 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/gadget/udc/gr_udc.c b/drivers/usb/gadget/udc/gr_udc.c
> > index 4b35739d3695..29c662f28428 100644
> > --- a/drivers/usb/gadget/udc/gr_udc.c
> > +++ b/drivers/usb/gadget/udc/gr_udc.c
> > @@ -1718,6 +1718,7 @@ static int gr_dequeue(struct usb_ep *_ep, struct usb_request *_req)
> >               ret = -EINVAL;
> >               goto out;
> >       }
> > +     printk(KERN_INFO "JKL: This does print, but shouldn't");
> >
> >       if (list_first_entry(&ep->queue, struct gr_request, queue) == req) {
> >               /* This request is currently being processed */
> > @@ -1739,6 +1740,30 @@ static int gr_dequeue(struct usb_ep *_ep, struct usb_request *_req)
> >       return ret;
> > }
> >
> > +static int __init init_test_jkl3(void)
> > +{
> > +     struct gr_ep *ep;
> > +     struct gr_udc *dev;
> > +     struct usb_request *_req;
> > +     void *buffer;
> > +
> > +     /* assume the usb_request struct is located just after the 'ep' on the heap */
> > +     buffer = kzalloc(sizeof(struct gr_ep)+sizeof(struct usb_request), GFP_KERNEL);
> > +     ep = buffer;
> > +     _req = buffer+sizeof(struct gr_ep);
> > +
> > +     /* setup to call gr_dequeue() */
> > +     dev = kzalloc(sizeof(struct gr_udc), GFP_KERNEL);
> > +     ep->dev = dev;
> > +     ep->dev->driver = 1;
> > +     INIT_LIST_HEAD(&ep->queue); /* list is empty */
> > +
> > +     gr_dequeue(&ep->ep, _req);
> > +
> > +     return 0;
> > +}
> > +__initcall(init_test_jkl3);
> > +
> > /* Helper for gr_set_halt and gr_set_wedge */
> > static int gr_set_halt_wedge(struct usb_ep *_ep, int halt, int wedge)
> > {
> > diff --git a/drivers/usb/gadget/udc/gr_udc.h b/drivers/usb/gadget/udc/gr_udc.h
> > index 70134239179e..14a18d5b5cf8 100644
> > --- a/drivers/usb/gadget/udc/gr_udc.h
> > +++ b/drivers/usb/gadget/udc/gr_udc.h
> > @@ -159,7 +159,6 @@ struct gr_ep {
> > };
> >
> > struct gr_request {
> > -     struct usb_request req;
> >       struct list_head queue;
> >
> >       /* Chain of dma descriptors */
> > @@ -171,6 +170,7 @@ struct gr_request {
> >       u16 oddlen; /* Size of odd length tail if buffer length is "odd" */
> >
> >       u8 setup; /* Setup packet */
> > +     struct usb_request req;
> > };
> >
> > enum gr_ep0state {
> > --
> > 2.25.1
>
> So, to follow the proposed solution in this thread, the following change
> is the "correct" one to make, right?
>
>
> 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;
>         }

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop
  2022-02-24 10:46           ` Cristiano Giuffrida
@ 2022-02-24 11:26             ` Greg Kroah-Hartman
  0 siblings, 0 replies; 70+ messages in thread
From: Greg Kroah-Hartman @ 2022-02-24 11:26 UTC (permalink / raw)
  To: Cristiano Giuffrida
  Cc: Jakob, Linus Torvalds, Linux Kernel Mailing List, linux-arch,
	Thomas Gleixner, Arnd Bergman, Andy Shevchenko, Andrew Morton,
	Kees Cook, Mike Rapoport, Gustavo A. R. Silva,
	Brian Johannesmeyer, Bos, H.J.

On Thu, Feb 24, 2022 at 11:46:40AM +0100, Cristiano Giuffrida wrote:
> I think the "problem" with this solution is that it doesn't prevent
> `tmp` from being used outside the loop still (and people getting it
> wrong again)? It would be good to have 'struct gr_request *tmp;' being
> visible only inside the loop (i.e., declared in the macro).

That is a larger change, one that we can hopefully make when changing to
introduce the temp variable in the loop_for_each() macro as Linus
described elsewhere in the thread.

But for now, it should be pretty obvious to not touch tmp again after
the loop is done.  That should make it easier to check for stuff like
this as well in an automated fashion (i.e. the loop variable is only
touched inside the loop.)

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop
  2022-02-23 21:53                 ` Linus Torvalds
@ 2022-02-24 16:04                   ` Nathan Chancellor
  0 siblings, 0 replies; 70+ messages in thread
From: Nathan Chancellor @ 2022-02-24 16:04 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Arnd Bergmann, Jakob, Linux Kernel Mailing List, linux-arch,
	Greg Kroah-Hartman, Thomas Gleixner, Andy Shevchenko,
	Andrew Morton, Kees Cook, Mike Rapoport, Gustavo A. R. Silva,
	Brian Johannesmeyer, Cristiano Giuffrida, Bos, H.J.,
	llvm

On Wed, Feb 23, 2022 at 01:53:39PM -0800, Linus Torvalds wrote:
> On Wed, Feb 23, 2022 at 1:46 PM Arnd Bergmann <arnd@arndb.de> wrote:
> >
> > > Ok, so we should be able to basically convert '--std=gnu89' into
> > > '--std=gnu11 -Wno-shift-negative-value' with no expected change of
> > > behavior.
> >
> > Yes, I think that is correct.
> 
> Ok, somebody please remind me, and let's just try this early in the
> 5.18 merge window.
> 
> Because at least for me, doing
> 
> -                  -std=gnu89
> +                  -std=gnu11 -Wno-shift-negative-value
> 
> for KBUILD_CFLAGS works fine both in my gcc and clang builds. But
> that's obviously just one version of each.

I ran that diff through my set of clang builds on
v5.17-rc5-21-g23d04328444a and only found one issue:

https://github.com/ClangBuiltLinux/linux/issues/1603

I think that should be fixed on the clang side. Once it is, I think we
could just disable that warning in those translation units for older
versions of clang to keep the status quo.

Cheers,
Nathan

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop
  2022-02-24 10:33         ` Greg Kroah-Hartman
@ 2022-02-24 17:56           ` Linus Torvalds
  0 siblings, 0 replies; 70+ messages in thread
From: Linus Torvalds @ 2022-02-24 17:56 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Jakob, Linux Kernel Mailing List, linux-arch, Thomas Gleixner,
	Arnd Bergman, Andy Shevchenko, Andrew Morton, Kees Cook,
	Mike Rapoport, Gustavo A. R. Silva, Brian Johannesmeyer,
	Cristiano Giuffrida, Bos, H.J.

On Thu, Feb 24, 2022 at 2:33 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> So, to follow the proposed solution in this thread, the following change
> is the "correct" one to make, right?

That would indeed be my preference.

If we ever are able to change

        list_for_each_entry(tmp, &ep->queue, queue)...

to simply declare 'tmp' as the right type itself, then the e need to do

        struct gr_request *tmp;

outside the loop goes away, and this kind of "set a variable that is
declared in the outside context" becomes the only way to do things.

                    Linus

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop
  2022-02-23 20:54               ` Linus Torvalds
  2022-02-23 22:21                 ` David Laight
@ 2022-02-25 21:36                 ` Uecker, Martin
  2022-02-25 22:02                   ` Linus Torvalds
  1 sibling, 1 reply; 70+ messages in thread
From: Uecker, Martin @ 2022-02-25 21:36 UTC (permalink / raw)
  To: torvalds, linux-kernel

Am Mittwoch, den 23.02.2022, 20:54 +0000 schrieb Linus Torvalds:
> On Wed, Feb 23, 2022 at 12:43 PM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> > Of course, the C standard being the bunch of incompetents they are,
> > they in the process apparently made left-shifts undefined (rather than
> > implementation-defined). Christ, they keep on making the same mistakes
> > over and over. What was the definition of insanity again?

Implementation-defined only means that it needs to be
documented (and clang does not even do this), so
I am not sure what difference this would make.

> Hey, some more googling on my part seems to say that somebody saw the
> light, and it's likely getting fixed in newer C standard version.

I don't think it is changed. But C23 will require
integers to be repreeted using two's complement,
so there is a better chance to fix things
like this in the future. 

> So it was just a mistake, not actual malice. Maybe we can hope that
> the tide is turning against the "undefined" crowd that used to rule
> the roost in the C standards bodies. Maybe the fundamental security
> issues with undefined behavior finally convinced people how bad it
> was?

The right people to complain to are the
compiler vendors, because they decide what
UB does in their implementation.  In the
standard body the same people argue that
the standard has to codify existing
practice.  Even in cases where the standard
defines behavior, compilers sometimes simply
ignore this (e.g. pointer comparison or
pointer-to-integer round  trips). So the
power is really with the compiler writers.


Martin 



^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop
  2022-02-25 21:36                 ` Uecker, Martin
@ 2022-02-25 22:02                   ` Linus Torvalds
  2022-02-26  1:21                     ` Martin Uecker
  0 siblings, 1 reply; 70+ messages in thread
From: Linus Torvalds @ 2022-02-25 22:02 UTC (permalink / raw)
  To: Uecker, Martin; +Cc: linux-kernel

On Fri, Feb 25, 2022 at 1:36 PM Uecker, Martin
<Martin.Uecker@med.uni-goettingen.de> wrote:
>
> Implementation-defined only means that it needs to be
> documented (and clang does not even do this), so
> I am not sure what difference this would make.

No, implementation-defined means something *much* more than "undefined".

Yes, it means that the behavior has to be documented.

But that's not actually the big deal from a user standpoint (although
it might be a big annoyance from a compiler writer's standpoint -
compiler writers would probably much prefer "let me do the obvious
sane thing" over "let me do the obvious sane thing and then have to
write documentation about how obviously sane it is").

From a user standpoint, the big thing is that "implementation-defined"
means that the behavior has to have *SOME* well-defined behavior.
Documentation is irrelevant. But RELIABILITY is relevant.

See?

That's a big big deal. The documentation is almost incidental - the
important part is that the code acts the same on the same
architecture, regardless of compiler options, and regardless of the
phase of the moon. When you upgrade your compiler to a new version,
the end result doesn't suddenly change.

In contrast, "undefined" means that the same C code can do two totally
different things with different optimization options, or based on just
any random thing - like some random register allocation issue.

So "undefined" behavior means that changes to code that isn't even
near the code in question can change what the code generation for that
code is. And the compiler may not even report it.

That is a complete disaster. It's a disaster from a security
standpoint, it's a disaster from a maintenance standpoint, it's just
*bad*.

And the C standards committee has traditionally used to think it was a
good thing, because they listened to compiler writers that claimed
that it allowed them to do wild optimizations and improve code
generation.

Example: the completely broken type-based aliasing rules that changed
semantics of C for the worse.

Reality: it doesn't actually improve code generation all that much.
Instead it just results in a lot of problems, and any sane C code base
that cares about security and stability will just turn that off.

Same goes for integer overflow etc.

The only really valid use-case for "undefined" is when you're doing
things like accessing past your own allocations. The compiler can't do
much about things like that.

But the C standards body has actually actively screwed the pooch over
the years, and turned perfectly traditional code into "undefined" code
for no good reason. "-1 << 1" is just one particularly simplistic
example.

> > Hey, some more googling on my part seems to say that somebody saw the
> > light, and it's likely getting fixed in newer C standard version.
>
> I don't think it is changed. But C23 will require
> integers to be repreeted using two's complement,
> so there is a better chance to fix things
> like this in the future.

If integers are guaranteed to be two's complement, then the only
possible explanation for "left shift is undefined" goes away. So
presumably the thing goes hand-in-hand with just making (the de-facto)
negative shifting well-defined.

(Btw, shifting *by* a negative number - or by amounts bigger than the
width of the type - is different. There are at least real reasons to
complain about that, although I think "implementation defined" would
still be hugely preferred over "undefined")

> The right people to complain to are the
> compiler vendors, because they decide what
> UB does in their implementation.

No. The bug was in the spec to begin with. The people to complain
about are the incompetents that literally changed the C standard to be
less well-specified.

As far as I know, no actual compiler has ever made integer left-shift
done anything bad. You'd literally have to do extra work for it, so
there's no reason to do so.

But because the standards committee messed up, the compiler writers
*did* do the extra work to warn about the fact (for the trivial
"negative constant" case - never mind that technically it's undefined
for non-constants: that would have been too much work).

End result: the warning is stupid. The warning only exists for
artificially stupid reasons.

And those reasons are literally "broken C standards committee behavior".

                      Linus

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop
  2022-02-25 22:02                   ` Linus Torvalds
@ 2022-02-26  1:21                     ` Martin Uecker
  2022-02-27 18:12                       ` Miguel Ojeda
  0 siblings, 1 reply; 70+ messages in thread
From: Martin Uecker @ 2022-02-26  1:21 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: linux-kernel

Am Freitag, den 25.02.2022, 14:02 -0800 schrieb Linus Torvalds:
> On Fri, Feb 25, 2022 at 1:36 PM Uecker, Martin
> <Martin.Uecker@med.uni-goettingen.de> wrote:
> > Implementation-defined only means that it needs to be
> > documented (and clang does not even do this), so
> > I am not sure what difference this would make.
> 
> No, implementation-defined means something *much* more than "undefined".
> 
> Yes, it means that the behavior has to be documented.
> 
> But that's not actually the big deal from a user standpoint (although
> it might be a big annoyance from a compiler writer's standpoint -
> compiler writers would probably much prefer "let me do the obvious
> sane thing" over "let me do the obvious sane thing and then have to
> write documentation about how obviously sane it is").
> 
> From a user standpoint, the big thing is that "implementation-defined"
> means that the behavior has to have *SOME* well-defined behavior.
> Documentation is irrelevant. But RELIABILITY is relevant.
> 
> See?

In principle, a compiler writer could document
implementation-defined behavior to do something
crazy and promise that something the standard
leaves undefined has useful and consistent behavior.

But I think you are right that this still makes
a huge difference in practice because the two
notions encourage different things.

...

> In contrast, "undefined" means that the same C code can do two totally
> different things with different optimization options, or based on just
> any random thing - like some random register allocation issue.
> 
> So "undefined" behavior means that changes to code that isn't even
> near the code in question can change what the code generation for that
> code is. And the compiler may not even report it.

Yes, it means the compiler is free to do this.

But it does not mean a compiler *has* to do this.

Compiler writers represented in the committee want
to have the UB because some customers seem to like
those optimizations. At the same time, a programmer
who complains about some undesired effect of such
an UB-based optimization is typically told that the
program has UB according to the C standard so
nothing can be done (bug report closed).

You see what is happening here?

Roughly the same group of people / companies that
write the compilers also control what goes into the
standard. They then like to point to the standard
and reject all responsibility for *their*
implementation choices.  By complaining about
the standard, one detracts from the fact that
the compiler writers make these choices and
also largely control what goes in the standard.


> That is a complete disaster. It's a disaster from a security
> standpoint, it's a disaster from a maintenance standpoint, it's just
> *bad*.

It is sometimes pure necessity. An unbounded write
has unbounded consequences and there is no realistic
way to give it defined behavior (in the context
of C).  But for many other cases of UB, I completely
agree with you.

> And the C standards committee has traditionally used to think it was a
> good thing, because they listened to compiler writers that claimed
> that it allowed them to do wild optimizations and improve code
> generation.

There are many compiler people in the standards committee,
so there is no surprise here.

> Example: the completely broken type-based aliasing rules that changed
> semantics of C for the worse.

I do not really know the history of this, but
I think these rules existed even before compilers
really exploited them for optimization.

> Reality: it doesn't actually improve code generation all that much.
> Instead it just results in a lot of problems, and any sane C code base
> that cares about security and stability will just turn that off.
> 
> Same goes for integer overflow etc.

For signed overflow, I am not entirely sure what the
right choice is.  Wrapping for signed overflow also seems
dangerous. I use UBsan to find such issues in my code, and
this would not really work if signed overflow was defined
to wrap.

...
> > > Hey, some more googling on my part seems to say that somebody saw the
> > > light, and it's likely getting fixed in newer C standard version.
> > 
> > I don't think it is changed. But C23 will require
> > integers to be repreeted using two's complement,
> > so there is a better chance to fix things
> > like this in the future.
> 
> If integers are guaranteed to be two's complement, then the only
> possible explanation for "left shift is undefined" goes away. So
> presumably the thing goes hand-in-hand with just making (the de-facto)
> negative shifting well-defined.

I think there is a fairy good chance to get this
changed.

> (Btw, shifting *by* a negative number - or by amounts bigger than the
> width of the type - is different. There are at least real reasons to
> complain about that, although I think "implementation defined" would
> still be hugely preferred over "undefined")
> 
> > The right people to complain to are the
> > compiler vendors, because they decide what
> > UB does in their implementation.
> 
> No. The bug was in the spec to begin with. The people to complain
> about are the incompetents that literally changed the C standard to be
> less well-specified.
>
> As far as I know, no actual compiler has ever made integer left-shift
> done anything bad. You'd literally have to do extra work for it, so
> there's no reason to do so.

I agree that the standard should be changed. But 
this example also shows that UB in the standard
does not inevitably causes the compilers to do
something stupid. It is still their choice.

> But because the standards committee messed up, the compiler writers
> *did* do the extra work to warn about the fact (for the trivial
> "negative constant" case - never mind that technically it's undefined
> for non-constants: that would have been too much work).

Compilers do *not* warn about a lot of undefined behavior 
(even where it would be easily possible and make perfect
sense). So why here?


Martin



^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop
  2022-02-23 19:23         ` Linus Torvalds
  2022-02-23 19:43           ` Linus Torvalds
  2022-02-23 20:24           ` Arnd Bergmann
@ 2022-02-26 12:42           ` Segher Boessenkool
  2022-02-26 22:14             ` Arnd Bergmann
  2 siblings, 1 reply; 70+ messages in thread
From: Segher Boessenkool @ 2022-02-26 12:42 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Jakob, Arnd Bergmann, Linux Kernel Mailing List, linux-arch,
	Greg Kroah-Hartman, Thomas Gleixner, Andy Shevchenko,
	Andrew Morton, Kees Cook, Mike Rapoport, Gustavo A. R. Silva,
	Brian Johannesmeyer, Cristiano Giuffrida, Bos, H.J.

On Wed, Feb 23, 2022 at 11:23:39AM -0800, Linus Torvalds wrote:
> On Wed, Feb 23, 2022 at 10:47 AM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > Arnd - remind me, please.. Was there some other problem than just gcc-4.9?
> 
> Hmm. Interesting. I decided to just try it and see for the compiler I
> have, and changing the gnu89 to gnu99 I get new warnings
> (-Werror=shift-negative-value).
> 
> Very annoying.  Especially since negative values are in many contexts
> actually *safer* than positive ones when used as a mask, because as
> long as the top bit is set in 'int', if the end result is then
> expanded to some wider type, the top bit stays set.
> 
> Example:
> 
>   unsigned long mask(unsigned long x)
>   { return x & (~0 << 5); }
> 
>   unsigned long badmask(unsigned long x)
>   { return x & (~0u << 5); }
> 
> One does it properly, the other is buggy.

You won't get this confusion if you write -1 instead.  Not that that
helps your problem here.

The GCC documentation says (at
<https://gcc.gnu.org/onlinedocs/gcc/Integers-implementation.html>):

  - The results of some bitwise operations on signed integers (C90 6.3,
    C99 and C11 6.5).

    Bitwise operators act on the representation of the value including
    both the sign and value bits, where the sign bit is considered
    immediately above the highest-value value bit. Signed ‘>>’ acts on
    negative numbers by sign extension.

    As an extension to the C language, GCC does not use the latitude
    given in C99 and C11 only to treat certain aspects of signed ‘<<’ as
    undefined. However, -fsanitize=shift (and -fsanitize=undefined) will
    diagnose such cases. They are also diagnosed where constant
    expressions are required.

But in fact they are diagnosed more often:
===
unsigned int n;
int f0(int x) { return x & (~0 << 5); }
int f1(int x) { return x & (-1 << 5); }
int f2(int x) { return x & (~0 << n); }
int f3(int x) { return x & (-1 << n); }
===
gets a warning on every line:
===
shifty.c: In function 'f0':
shifty.c:2:32: warning: left shift of negative value [-Wshift-negative-value]
    2 | int f0(int x) { return x & (~0 << 5); }
      |                                ^~
shifty.c: In function 'f1':
shifty.c:3:32: warning: left shift of negative value [-Wshift-negative-value]
    3 | int f1(int x) { return x & (-1 << 5); }
      |                                ^~
shifty.c: In function 'f2':
shifty.c:4:32: warning: left shift of negative value [-Wshift-negative-value]
    4 | int f2(int x) { return x & (~0 << n); }
      |                                ^~
shifty.c: In function 'f3':
shifty.c:5:32: warning: left shift of negative value [-Wshift-negative-value]
    5 | int f3(int x) { return x & (-1 << n); }
      |                                ^~
===
(with -O2 -Wall -W, nothing more).  No constant expression is required
in any of those cases, and in f2 and f3 the shift is not a constant
expression even.

> So this Werror=shift-negative-value warning seems to be actively
> detrimental, and I'm not seeing the reason for it. Can somebody
> explain the thinking for that stupid warning?

-Werror=*anything* is detrimental, *always*.  Warnings are warnings
and errors are errors.  Warnings have false positives: if not, they
should be errors instead!

> That said, we seem to only have two cases of it in the kernel, at
> least by a x86-64 allmodconfig build. So we could examine the types
> there, or we could just add '-Wno-shift-negative-value".

Yes.

The only reason the warning exists is because it is undefined behaviour
(not implementation-defined or anything).  The reason it is that in the
standard is that it is hard to implement and even describe for machines
that are not two's complement.  However relevant that is today :-)


Segher

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop
  2022-02-26 12:42           ` Segher Boessenkool
@ 2022-02-26 22:14             ` Arnd Bergmann
  2022-02-26 23:03               ` Linus Torvalds
  2022-02-27  1:09               ` Segher Boessenkool
  0 siblings, 2 replies; 70+ messages in thread
From: Arnd Bergmann @ 2022-02-26 22:14 UTC (permalink / raw)
  To: Segher Boessenkool
  Cc: Linus Torvalds, Jakob, Arnd Bergmann, Linux Kernel Mailing List,
	linux-arch, Greg Kroah-Hartman, Thomas Gleixner, Andy Shevchenko,
	Andrew Morton, Kees Cook, Mike Rapoport, Gustavo A. R. Silva,
	Brian Johannesmeyer, Cristiano Giuffrida, Bos, H.J.

On Sat, Feb 26, 2022 at 1:42 PM Segher Boessenkool
<segher@kernel.crashing.org> wrote:
> On Wed, Feb 23, 2022 at 11:23:39AM -0800, Linus Torvalds wrote:

> > That said, we seem to only have two cases of it in the kernel, at
> > least by a x86-64 allmodconfig build. So we could examine the types
> > there, or we could just add '-Wno-shift-negative-value".
>
> Yes.
>
> The only reason the warning exists is because it is undefined behaviour
> (not implementation-defined or anything).  The reason it is that in the
> standard is that it is hard to implement and even describe for machines
> that are not two's complement.  However relevant that is today :-)

Could gcc follow the clang behavior then and skip the warning and
sanitizer for this case when -fno-strict-overflow or -fwrapv are used?

         Arnd

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop
  2022-02-26 22:14             ` Arnd Bergmann
@ 2022-02-26 23:03               ` Linus Torvalds
  2022-02-27  1:19                 ` Segher Boessenkool
  2022-02-27  1:09               ` Segher Boessenkool
  1 sibling, 1 reply; 70+ messages in thread
From: Linus Torvalds @ 2022-02-26 23:03 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Segher Boessenkool, Jakob, Linux Kernel Mailing List, linux-arch,
	Greg Kroah-Hartman, Thomas Gleixner, Andy Shevchenko,
	Andrew Morton, Kees Cook, Mike Rapoport, Gustavo A. R. Silva,
	Brian Johannesmeyer, Cristiano Giuffrida, Bos, H.J.

On Sat, Feb 26, 2022 at 2:14 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> Could gcc follow the clang behavior then and skip the warning and
> sanitizer for this case when -fno-strict-overflow or -fwrapv are used?

Well, for the kernel, that horse has already left the barn, and we'd
have to use -Wno-shift-negative-value anyway.

But yes, from a sanity standpoint, it would be good to shut that
warning up automatically if compiling for a 2's complement machine (ie
"all of them") with -fwrapv.

Considering that gcc doesn't support any non-2's-complement machines
anyway afaik, and that the C standards people are also fixing the
standard, and gcc has never done anything odd in this area in the
first place, I think the warning is probably best removed entirely.
But we'll have to do it manually for the existing situation.

          Linus

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop
  2022-02-26 22:14             ` Arnd Bergmann
  2022-02-26 23:03               ` Linus Torvalds
@ 2022-02-27  1:09               ` Segher Boessenkool
  2022-02-27  7:10                 ` David Laight
  2022-02-27 21:28                 ` Arnd Bergmann
  1 sibling, 2 replies; 70+ messages in thread
From: Segher Boessenkool @ 2022-02-27  1:09 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Linus Torvalds, Jakob, Linux Kernel Mailing List, linux-arch,
	Greg Kroah-Hartman, Thomas Gleixner, Andy Shevchenko,
	Andrew Morton, Kees Cook, Mike Rapoport, Gustavo A. R. Silva,
	Brian Johannesmeyer, Cristiano Giuffrida, Bos, H.J.

On Sat, Feb 26, 2022 at 11:14:15PM +0100, Arnd Bergmann wrote:
> On Sat, Feb 26, 2022 at 1:42 PM Segher Boessenkool
> <segher@kernel.crashing.org> wrote:
> > On Wed, Feb 23, 2022 at 11:23:39AM -0800, Linus Torvalds wrote:
> 
> > > That said, we seem to only have two cases of it in the kernel, at
> > > least by a x86-64 allmodconfig build. So we could examine the types
> > > there, or we could just add '-Wno-shift-negative-value".
> >
> > Yes.
> >
> > The only reason the warning exists is because it is undefined behaviour
> > (not implementation-defined or anything).  The reason it is that in the
> > standard is that it is hard to implement and even describe for machines
> > that are not two's complement.  However relevant that is today :-)
> 
> Could gcc follow the clang behavior then and skip the warning and
> sanitizer for this case when -fno-strict-overflow or -fwrapv are used?

As I said, we have this implementation choice documented as
  As an extension to the C language, GCC does not use the latitude
  given in C99 and C11 only to treat certain aspects of signed '<<'
  as undefined.  However, '-fsanitize=shift' (and
  '-fsanitize=undefined') will diagnose such cases.  They are also
  diagnosed where constant expressions are required.
but that is not at all what we implement currently, we warn much more
often.  Constant expressions are required only in a few places (#if
condition, bitfield length, (non-variable) array length, enumeration
declarations, _Alignas, _Static_assert, case labels, most initialisers);
not places you will see code like this problem normally.

So imo we should just never do this by default, not just if the nasty
-fwrapv or nastier -fno-strict-overflow is used, just like we suggest
in our own documentation.  The only valid reason -Wshift-negative-value
is in -Wextra is it warns for situations that always are undefined
behaviour (even if not in GCC).

Could you open a GCC PR for this?  The current situation is quite
suboptimal, and what we document as our implementation choice is much
more useful!


Segher

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop
  2022-02-26 23:03               ` Linus Torvalds
@ 2022-02-27  1:19                 ` Segher Boessenkool
  0 siblings, 0 replies; 70+ messages in thread
From: Segher Boessenkool @ 2022-02-27  1:19 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Arnd Bergmann, Jakob, Linux Kernel Mailing List, linux-arch,
	Greg Kroah-Hartman, Thomas Gleixner, Andy Shevchenko,
	Andrew Morton, Kees Cook, Mike Rapoport, Gustavo A. R. Silva,
	Brian Johannesmeyer, Cristiano Giuffrida, Bos, H.J.

On Sat, Feb 26, 2022 at 03:03:09PM -0800, Linus Torvalds wrote:
> On Sat, Feb 26, 2022 at 2:14 PM Arnd Bergmann <arnd@arndb.de> wrote:
> >
> > Could gcc follow the clang behavior then and skip the warning and
> > sanitizer for this case when -fno-strict-overflow or -fwrapv are used?
> 
> Well, for the kernel, that horse has already left the barn, and we'd
> have to use -Wno-shift-negative-value anyway.
> 
> But yes, from a sanity standpoint, it would be good to shut that
> warning up automatically if compiling for a 2's complement machine (ie
> "all of them") with -fwrapv.
> 
> Considering that gcc doesn't support any non-2's-complement machines
> anyway afaik,

   * 'Whether signed integer types are represented using sign and
     magnitude, two's complement, or one's complement, and whether the
     extraordinary value is a trap representation or an ordinary value
     (C99 and C11 6.2.6.2).'

     GCC supports only two's complement integer types, and all bit
     patterns are ordinary values.

> and that the C standards people are also fixing the
> standard, and gcc has never done anything odd in this area in the
> first place, I think the warning is probably best removed entirely.

Well, not removed, it correctly identifies (formally) undefined
behaviour after all; but I agree it should not be in -Wextra.

-Wall should include the warnings that have a very good balance for
usefulness, number of false postives, seriousness of problems found.
-Wextra is exactly the same conditions, just a slightly lower bar.

-Wall should be useful for everyone.  -Wall -W should be good for most
people.

> But we'll have to do it manually for the existing situation.

Yes, sorry about that :-/


Segher

^ permalink raw reply	[flat|nested] 70+ messages in thread

* RE: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop
  2022-02-27  1:09               ` Segher Boessenkool
@ 2022-02-27  7:10                 ` David Laight
  2022-02-27 11:32                   ` Segher Boessenkool
  2022-02-27 21:28                 ` Arnd Bergmann
  1 sibling, 1 reply; 70+ messages in thread
From: David Laight @ 2022-02-27  7:10 UTC (permalink / raw)
  To: 'Segher Boessenkool', Arnd Bergmann
  Cc: Linus Torvalds, Jakob, Linux Kernel Mailing List, linux-arch,
	Greg Kroah-Hartman, Thomas Gleixner, Andy Shevchenko,
	Andrew Morton, Kees Cook, Mike Rapoport, Gustavo A. R. Silva,
	Brian Johannesmeyer, Cristiano Giuffrida, Bos, H.J.

From: Segher Boessenkool
> Sent: 27 February 2022 01:10
> 
> On Sat, Feb 26, 2022 at 11:14:15PM +0100, Arnd Bergmann wrote:
> > On Sat, Feb 26, 2022 at 1:42 PM Segher Boessenkool
> > <segher@kernel.crashing.org> wrote:
> > > On Wed, Feb 23, 2022 at 11:23:39AM -0800, Linus Torvalds wrote:
> >
> > >
> > > The only reason the warning exists is because it is undefined behaviour
> > > (not implementation-defined or anything).  The reason it is that in the
> > > standard is that it is hard to implement and even describe for machines
> > > that are not two's complement.  However relevant that is today :-)

I thought only right shifts of negative values were 'undefined'.
And that was to allow cpu that only had logical shift right
(ie ones that didn't propagate the sign) to be conformant.
I wonder when the last cpu like that was?

Quite why the standards keeps using the term 'undefined behaviour'
beats me - there ought to be something for 'undefined value'.

	David

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


^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop
  2022-02-27  7:10                 ` David Laight
@ 2022-02-27 11:32                   ` Segher Boessenkool
  2022-02-27 18:09                     ` Miguel Ojeda
  0 siblings, 1 reply; 70+ messages in thread
From: Segher Boessenkool @ 2022-02-27 11:32 UTC (permalink / raw)
  To: David Laight
  Cc: Arnd Bergmann, Linus Torvalds, Jakob, Linux Kernel Mailing List,
	linux-arch, Greg Kroah-Hartman, Thomas Gleixner, Andy Shevchenko,
	Andrew Morton, Kees Cook, Mike Rapoport, Gustavo A. R. Silva,
	Brian Johannesmeyer, Cristiano Giuffrida, Bos, H.J.

On Sun, Feb 27, 2022 at 07:10:45AM +0000, David Laight wrote:
> From: Segher Boessenkool
> > Sent: 27 February 2022 01:10
> > On Sat, Feb 26, 2022 at 11:14:15PM +0100, Arnd Bergmann wrote:
> > > On Sat, Feb 26, 2022 at 1:42 PM Segher Boessenkool
> > > <segher@kernel.crashing.org> wrote:
> > > > The only reason the warning exists is because it is undefined behaviour
> > > > (not implementation-defined or anything).  The reason it is that in the
> > > > standard is that it is hard to implement and even describe for machines
> > > > that are not two's complement.  However relevant that is today :-)
> 
> I thought only right shifts of negative values were 'undefined'.

All shifts by a negative amount, or an amount greater or equal to the
width of the first operand (after the integer promotions).

Right shifts of a negative value.

Left shifts of a negative value where E1*2**E2 is not expressable in the
result type.

C90 (aka C89) had those right shifts merely implementation-defined
behaviour, and the left shifts perfectly well-defined.

> And that was to allow cpu that only had logical shift right
> (ie ones that didn't propagate the sign) to be conformant.

The C99 rationale says
  The C89 Committee affirmed the freedom in implementation granted by
  K&R in not requiring the signed right shift operation to sign extend,
  since such a requirement might slow down fast code and since the
  usefulness of sign extended shifts is marginal. (Shifting a negative
  two’s-complement integer arithmetically right one place is not the
  same as dividing by two!)

> I wonder when the last cpu like that was?

There still are one-off cores without such an instruction.  If you have
right shifts at all (if you have shifts at all!) it quickly becomes
apparent what a hassle it is not to have an SRA/ASR/SAR instruction, and
it is easy to implement, so :-)

The last widely spread ones' complement machine was the 6600 I guess,
which disappeared somewhere in the 80's.  Sign-magnitude machines are
still made: all FP is like that, and some (simple, embedded, etc.)
machines have no separate integer register set.  C is available for most
such fringe CPUs :-)

> Quite why the standards keeps using the term 'undefined behaviour'
> beats me - there ought to be something for 'undefined value'.

It is pretty much impossible to not have *some* undefined behaviour.
How will you define dividing by zero so that its behaviour is reasonable
for every program, for example?  Invoking an error handler at runtime
has most of the same unwanted effects, except is is never silent.  You
can get that via UBSAN for example.  Defining some arbitrary value as
the "correct" answer when that is not at all obvious *does* give silent,
unexpected results.

C does have "unspecified value"s and "unspecified behaviour".  It
requires the implementation to document the choice made here.  At least
some effort was made not to have undefined behaviour everywhere.

Perhaps C does have the concept you are after in 6.3.2/2, where it talks
about using uninitialised objects?  Of course, using an uninitialised
object is undefined behaviour :-P

C has much more undefined behaviours than most other languages.  On one
hand there was no clear, formal definition of the language (and
testsuites for it etc.) before it became popular.  On the other hand, it
was implemented on widely different architectures, back in the days when
there was a lot more variety in implementation choices than there is
now.  When the language was standardised (and all the way to this day)
the sentiment was to not unnecessarily break existing implementations.


Segher

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop
  2022-02-27 11:32                   ` Segher Boessenkool
@ 2022-02-27 18:09                     ` Miguel Ojeda
  2022-02-27 20:17                       ` Segher Boessenkool
  0 siblings, 1 reply; 70+ messages in thread
From: Miguel Ojeda @ 2022-02-27 18:09 UTC (permalink / raw)
  To: Segher Boessenkool
  Cc: David Laight, Arnd Bergmann, Linus Torvalds, Jakob,
	Linux Kernel Mailing List, linux-arch, Greg Kroah-Hartman,
	Thomas Gleixner, Andy Shevchenko, Andrew Morton, Kees Cook,
	Mike Rapoport, Gustavo A. R. Silva, Brian Johannesmeyer,
	Cristiano Giuffrida, Bos, H.J.

On Sun, Feb 27, 2022 at 1:09 PM Segher Boessenkool
<segher@kernel.crashing.org> wrote:
>
> How will you define dividing by zero so that its behaviour is reasonable
> for every program, for example?

The solution is to let the developer specify what they need to happen.
That choice should include the unsafe possibility (i.e. unchecked),
because sometimes that is precisely what we need.

> Invoking an error handler at runtime
> has most of the same unwanted effects, except is is never silent.  You

It may not be what it is needed in some cases (thus the necessity to
be able to choose), but at least one can predict what happens and
different compilers, versions, flags, inputs, etc. would agree.

Cheers,
Miguel

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop
  2022-02-26  1:21                     ` Martin Uecker
@ 2022-02-27 18:12                       ` Miguel Ojeda
  2022-02-28  7:08                         ` Martin Uecker
  0 siblings, 1 reply; 70+ messages in thread
From: Miguel Ojeda @ 2022-02-27 18:12 UTC (permalink / raw)
  To: Martin Uecker; +Cc: Linus Torvalds, linux-kernel

On Sat, Feb 26, 2022 at 3:43 AM Martin Uecker <uecker@tugraz.at> wrote:
>
> Roughly the same group of people / companies that
> write the compilers also control what goes into the
> standard. They then like to point to the standard

Indeed, at least to a substantial degree.

> For signed overflow, I am not entirely sure what the
> right choice is.  Wrapping for signed overflow also seems
> dangerous. I use UBsan to find such issues in my code, and
> this would not really work if signed overflow was defined
> to wrap.

UBsan and similar tooling may still be used to find whatever behavior
one wants, whether defined or not. UBSan already has non-UB checks.

Cheers,
Miguel

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop
  2022-02-27 18:09                     ` Miguel Ojeda
@ 2022-02-27 20:17                       ` Segher Boessenkool
  2022-02-27 21:04                         ` Linus Torvalds
  2022-02-27 22:43                         ` Miguel Ojeda
  0 siblings, 2 replies; 70+ messages in thread
From: Segher Boessenkool @ 2022-02-27 20:17 UTC (permalink / raw)
  To: Miguel Ojeda
  Cc: David Laight, Arnd Bergmann, Linus Torvalds, Jakob,
	Linux Kernel Mailing List, linux-arch, Greg Kroah-Hartman,
	Thomas Gleixner, Andy Shevchenko, Andrew Morton, Kees Cook,
	Mike Rapoport, Gustavo A. R. Silva, Brian Johannesmeyer,
	Cristiano Giuffrida, Bos, H.J.

On Sun, Feb 27, 2022 at 07:09:03PM +0100, Miguel Ojeda wrote:
> On Sun, Feb 27, 2022 at 1:09 PM Segher Boessenkool
> <segher@kernel.crashing.org> wrote:
> >
> > How will you define dividing by zero so that its behaviour is reasonable
> > for every program, for example?
> 
> The solution is to let the developer specify what they need to happen.
> That choice should include the unsafe possibility (i.e. unchecked),
> because sometimes that is precisely what we need.

Requiring to annotate every place that has UB (or *can* have UB!) by the
user is even less friendly than having so much UB is already :-(

I don't see how you will fit this into the C syntax, btw?

> > Invoking an error handler at runtime
> > has most of the same unwanted effects, except is is never silent.  You
> 
> It may not be what it is needed in some cases (thus the necessity to
> be able to choose), but at least one can predict what happens and
> different compilers, versions, flags, inputs, etc. would agree.

You need a VM like Java's to get even *close* to that.  This is not the
C target: it is slower than wanted/expected, it is hosted instead of
embedded, and it comes with a whole host of issues of its own.  One of
the strengths of C is its tiny runtime, a few kB is a lot already!

I completely agree that if you design a new "systems" language, you want
to have much less undefined behaviour than C has.  But it is self-
delusion to think you can eradicate all (or even most).

And there are much bigger problems in any case!  If you think that if
programmers could no longer write programs that invoke undefined
behaviour they will write much better programs, programs with fewer
serious functionality or security problems, even just a factor of two
better, well...


Segher

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop
  2022-02-27 20:17                       ` Segher Boessenkool
@ 2022-02-27 21:04                         ` Linus Torvalds
  2022-02-28  6:15                           ` David Laight
  2022-02-27 22:43                         ` Miguel Ojeda
  1 sibling, 1 reply; 70+ messages in thread
From: Linus Torvalds @ 2022-02-27 21:04 UTC (permalink / raw)
  To: Segher Boessenkool
  Cc: Miguel Ojeda, David Laight, Arnd Bergmann, Jakob,
	Linux Kernel Mailing List, linux-arch, Greg Kroah-Hartman,
	Thomas Gleixner, Andy Shevchenko, Andrew Morton, Kees Cook,
	Mike Rapoport, Gustavo A. R. Silva, Brian Johannesmeyer,
	Cristiano Giuffrida, Bos, H.J.

On Sun, Feb 27, 2022 at 12:22 PM Segher Boessenkool
<segher@kernel.crashing.org> wrote:
>
> Requiring to annotate every place that has UB (or *can* have UB!) by the
> user is even less friendly than having so much UB is already :-(

Yeah, I don't think that's the solution. In fact, I don't think that's
even practically the _issue_.

Honestly, a lot of "undefined behavior" in C is quite often of the
kind "the programmer knows what he wants".

Things like word size or byte order issues etc are classic "undefined
behavior" in the sense that the C compiler really doesn't understand
them. The C compiler won't silently fix any silly behavior you get
from writing files in native byte order, and them not working on other
platforms.

Same goes for things like memory allocators - they often need to do
things that the standard doesn't really cover, and shouldn't even
*try* to cover. It's very much a core example of where people do odd
pointer arithmetic and change the type of pointers.

The problem with the C model of "undefined behavior" is not that the
behavior ends up being architecture-specific and depending on various
in-memory (or in-register) representation of the data. No, those
things are often very much intentional (even if in the case of byte
order, the "intention" may be that the programmer simply does not
care, and "knows" that all the world is little endian).

If the C compiler just generates reliable code that can sanely be
debugged - including very much using tools that look for "hey, this
behavior can be surprising", ie all those "look for bad patterns at
run-time", then that would be 100% FINE.

But the problem with the C notion of undefined behavior is NOT that
"results can depend on memory layout and other architecture issues
that the compiler doesn't understand".

No, the problem is that the C standards people - and compiler people -
have then declared that "because this can be surprising, and the
compiler doesn't understand what is going on, now the compiler can do
something *else* entirely".

THAT is the problem.

The classic case - and my personal "beat a dead horse" - is the
completely broken type-based aliasing rules. The standards people
literally said "the compiler doesn't understand this, it can expose
byte order dependencies that shouldn't be explained, SO THE COMPILER
CAN NOW DO SOMETHING COMPLETELY INSANE INSTEAD".

And compiler people? They rushed out to do completely broken garbage -
at least some of them did.

You can literally find compiler people who see code like this (very
traditional, traditionally very valid and common, although):

   // Return the least significant 16 bits of 'a' on LE machines
  #define low_16_bits(x) (*(unsigned short *)&(x))

and say "oh, because that's undefined, I can now decide to not do what
the programmer told me to do AT ALL".

Note that the above wasn't actually even badly defined originally. It
was well-defined, it was used, and it was done by programmers that
knew what they were doing.

And then the C standards people decided that "because our job isn't to
describe all the architectural issues you can hit, we'll call it
undefined, and in the process let compiler people intentionally break
it".

THAT is a problem.

Undefined results are are often intentional in system software - or
they can be debugged using smart tools (including possibly very
expensive run-time code generation) that actively look for them.

But compilers that randomly do crazy things because the standard was
bad? That's just broken.

If compilers treated "undefined" as the same as
"implementation-defined, but not explicitly documented", then that
would be fine. But the C standards people - and a lot of compiler
people - really don't seem to understand the problems they caused.

And, btw, caused for no actual good reason. The HPC people who wanted
Fortran-style aliasing could easily have had that with an extension.
Yes, "restrict" is kind of a crappy one, but it could have been
improved upon. Instead, people said "let's just break the language".

Same exact thing goes for signed integer overflow.

               Linus

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop
  2022-02-27  1:09               ` Segher Boessenkool
  2022-02-27  7:10                 ` David Laight
@ 2022-02-27 21:28                 ` Arnd Bergmann
  2022-02-27 22:43                   ` Segher Boessenkool
  1 sibling, 1 reply; 70+ messages in thread
From: Arnd Bergmann @ 2022-02-27 21:28 UTC (permalink / raw)
  To: Segher Boessenkool
  Cc: Arnd Bergmann, Linus Torvalds, Jakob, Linux Kernel Mailing List,
	linux-arch, Greg Kroah-Hartman, Thomas Gleixner, Andy Shevchenko,
	Andrew Morton, Kees Cook, Mike Rapoport, Gustavo A. R. Silva,
	Brian Johannesmeyer, Cristiano Giuffrida, Bos, H.J.

On Sun, Feb 27, 2022 at 2:09 AM Segher Boessenkool
<segher@kernel.crashing.org> wrote:
>
> So imo we should just never do this by default, not just if the nasty
> -fwrapv or nastier -fno-strict-overflow is used, just like we suggest
> in our own documentation.  The only valid reason -Wshift-negative-value
> is in -Wextra is it warns for situations that always are undefined
> behaviour (even if not in GCC).

Ok, I just realized that this is specific to the i915 driver because
that, unlike
most of the kernel builds with -Wextra by default. -Wextra is enabled when
users ask for a 'make W=1' build in linux, and i915 is one of just three
drivers that enable an equivalent set of warnings, the other ones
being greybus and btrfs.

This means to work around the extra warnings, we also just need to disable
it in the W=1 part of scripts/Makefile.extrawarn, as well as the three drivers
that copy those options, but not the default warnings that don't include them.

> Could you open a GCC PR for this?  The current situation is quite
> suboptimal, and what we document as our implementation choice is much
> more useful!

I hope I managed to capture the issue in
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104711

         Arnd

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop
  2022-02-27 20:17                       ` Segher Boessenkool
  2022-02-27 21:04                         ` Linus Torvalds
@ 2022-02-27 22:43                         ` Miguel Ojeda
  1 sibling, 0 replies; 70+ messages in thread
From: Miguel Ojeda @ 2022-02-27 22:43 UTC (permalink / raw)
  To: Segher Boessenkool
  Cc: David Laight, Arnd Bergmann, Linus Torvalds, Jakob,
	Linux Kernel Mailing List, linux-arch, Greg Kroah-Hartman,
	Thomas Gleixner, Andy Shevchenko, Andrew Morton, Kees Cook,
	Mike Rapoport, Gustavo A. R. Silva, Brian Johannesmeyer,
	Cristiano Giuffrida, Bos, H.J.

On Sun, Feb 27, 2022 at 9:19 PM Segher Boessenkool
<segher@kernel.crashing.org> wrote:
>
> Requiring to annotate every place that has UB (or *can* have UB!) by the
> user is even less friendly than having so much UB is already :-(

Sure, but I did not suggest to annotate every place -- at least not in C.

What I said is that in cases like your division by zero example, there
is no one-fits-all solution. For instance, for integer arithmetic,
leaving the choice (e.g. wrapping, saturating, unchecked...) to the
user is a better approach.

> You need a VM like Java's to get even *close* to that.  This is not the
> C target: it is slower than wanted/expected, it is hosted instead of
> embedded, and it comes with a whole host of issues of its own.  One of
> the strengths of C is its tiny runtime, a few kB is a lot already!
>
> I completely agree that if you design a new "systems" language, you want
> to have much less undefined behaviour than C has.  But it is self-
> delusion to think you can eradicate all (or even most).

Nobody is suggesting to remove "all UB" in that sense or to use
VM-supported languages.

However, you can """eradicate all UB""" in a sense: you can offer to
write most code in a subset that does not allow any potential-UB
operations. This can even be "all" code, depending on how you count
(e.g. all application code).

Obviously, you still need the unchecked operations to implement the
safe APIs. This is why I mentioned them.

> And there are much bigger problems in any case!  If you think that if
> programmers could no longer write programs that invoke undefined
> behaviour they will write much better programs, programs with fewer
> serious functionality or security problems, even just a factor of two
> better, well...

Actually, according to several reports from Google, Microsoft, etc.,
it _is_ the biggest problem (~70%) when talking about security bugs.

So it is a good bet that it will translate into "better" programs, at
least on that axis, unless it is showed that removing UB somehow
increases the rate of other errors.

As for functionality problems, there are several ways to improve upon
C too, though it is harder to show data on that.

Cheers,
Miguel

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop
  2022-02-27 21:28                 ` Arnd Bergmann
@ 2022-02-27 22:43                   ` Segher Boessenkool
  0 siblings, 0 replies; 70+ messages in thread
From: Segher Boessenkool @ 2022-02-27 22:43 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Linus Torvalds, Jakob, Linux Kernel Mailing List, linux-arch,
	Greg Kroah-Hartman, Thomas Gleixner, Andy Shevchenko,
	Andrew Morton, Kees Cook, Mike Rapoport, Gustavo A. R. Silva,
	Brian Johannesmeyer, Cristiano Giuffrida, Bos, H.J.

On Sun, Feb 27, 2022 at 10:28:41PM +0100, Arnd Bergmann wrote:
> On Sun, Feb 27, 2022 at 2:09 AM Segher Boessenkool
> <segher@kernel.crashing.org> wrote:
> >
> > So imo we should just never do this by default, not just if the nasty
> > -fwrapv or nastier -fno-strict-overflow is used, just like we suggest
> > in our own documentation.  The only valid reason -Wshift-negative-value
> > is in -Wextra is it warns for situations that always are undefined
> > behaviour (even if not in GCC).
> 
> Ok, I just realized that this is specific to the i915 driver because
> that, unlike
> most of the kernel builds with -Wextra by default. -Wextra is enabled when
> users ask for a 'make W=1' build in linux, and i915 is one of just three
> drivers that enable an equivalent set of warnings, the other ones
> being greybus and btrfs.
> 
> This means to work around the extra warnings, we also just need to disable
> it in the W=1 part of scripts/Makefile.extrawarn, as well as the three drivers
> that copy those options, but not the default warnings that don't include them.

Ah good, all of the workaround in one simple place, neat.

> > Could you open a GCC PR for this?  The current situation is quite
> > suboptimal, and what we document as our implementation choice is much
> > more useful!
> 
> I hope I managed to capture the issue in
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104711

That looks fine.  Thank you!

(I attached the testcase to the bug itself, we prefer it that way, maybe
godbolt will go away some day, who knows.)


Segher

^ permalink raw reply	[flat|nested] 70+ messages in thread

* RE: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop
  2022-02-27 21:04                         ` Linus Torvalds
@ 2022-02-28  6:15                           ` David Laight
  0 siblings, 0 replies; 70+ messages in thread
From: David Laight @ 2022-02-28  6:15 UTC (permalink / raw)
  To: 'Linus Torvalds', Segher Boessenkool
  Cc: Miguel Ojeda, Arnd Bergmann, Jakob, Linux Kernel Mailing List,
	linux-arch, Greg Kroah-Hartman, Thomas Gleixner, Andy Shevchenko,
	Andrew Morton, Kees Cook, Mike Rapoport, Gustavo A. R. Silva,
	Brian Johannesmeyer, Cristiano Giuffrida, Bos, H.J.

From: Linus Torvalds
> Sent: 27 February 2022 21:05
...
> And then the C standards people decided that "because our job isn't to
> describe all the architectural issues you can hit, we'll call it
> undefined, and in the process let compiler people intentionally break
> it".
> 
> THAT is a problem.

I'm waiting for them to decide that memset(ptr, 0, len) of
any structure that contains a pointer is UB (because a NULL
pointer need not be the all zero bit pattern) so decide
to discard the call completely (or some such).

Non-zero NULL pointers is the only reason arithmetic on NULL
pointers isn't valid.

Or maybe that character range tests are UB because '0' to '9'
don't have to be adjacent - they are even adjacent in EBCDIC.

Some of the 'strict aliasing' bits are actually useful since
they let the compiler reorder reads and writes.
But the definition is brain-dead.
Sometimes it would be nice to have byte writes reordered,
but even using int:8 doesn't work.

I have never worked out what 'restrict' actually does,
in any places I've tried it did nothing.
Although I may have been hoping it would still help when
the function got inlined.

	David

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

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop
  2022-02-27 18:12                       ` Miguel Ojeda
@ 2022-02-28  7:08                         ` Martin Uecker
  2022-02-28 13:49                           ` Miguel Ojeda
  0 siblings, 1 reply; 70+ messages in thread
From: Martin Uecker @ 2022-02-28  7:08 UTC (permalink / raw)
  To: Miguel Ojeda; +Cc: Linus Torvalds, linux-kernel

Am Sonntag, den 27.02.2022, 19:12 +0100 schrieb Miguel Ojeda:
> On Sat, Feb 26, 2022 at 3:43 AM Martin Uecker <uecker@tugraz.at> wrote:
> > Roughly the same group of people / companies that
> > write the compilers also control what goes into the
> > standard. They then like to point to the standard
> 
> Indeed, at least to a substantial degree.
> 
> > For signed overflow, I am not entirely sure what the
> > right choice is.  Wrapping for signed overflow also seems
> > dangerous. I use UBsan to find such issues in my code, and
> > this would not really work if signed overflow was defined
> > to wrap.
> 
> UBsan and similar tooling may still be used to find whatever behavior
> one wants, whether defined or not. UBSan already has non-UB checks.

Technically, this is true but not really in practice. If signed
overflow would be defined to wrap, then code would start to
rely on it and detecting it becomes useless because there are
too many false positives.  In your own small controlled code 
base it could work though.

Martin


^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop
  2022-02-28  7:08                         ` Martin Uecker
@ 2022-02-28 13:49                           ` Miguel Ojeda
  2022-03-01 20:26                             ` Linus Torvalds
  0 siblings, 1 reply; 70+ messages in thread
From: Miguel Ojeda @ 2022-02-28 13:49 UTC (permalink / raw)
  To: Martin Uecker; +Cc: Linus Torvalds, linux-kernel

On Mon, Feb 28, 2022 at 8:08 AM Martin Uecker <uecker@tugraz.at> wrote:
>
> Technically, this is true but not really in practice. If signed
> overflow would be defined to wrap, then code would start to
> rely on it and detecting it becomes useless because there are
> too many false positives.  In your own small controlled code
> base it could work though.

Either code is written with signed overflow in mind, or not. That is
what actually matters for detection, not whether it is UB in the
standard or not.

If a project starts relying on overflow wrapping, then they are taking
the same stance as projects that already rely on `-fwrapv`. That is
their choice, same as today.

But making it non-UB in the standard does not force a project to
consider it "not an error", which is what actually matters for being
able to use UBSan effectively or not.

In fact, if you are just concerned about detectability or `-fwrapv`
being the wrong default, there is still a way out without keeping it
UB: we could consider it an error (thus people is not encouraged to
rely on it), yet not UB. That is what Rust does, and why I suggested
the past exploring the move of some existing UB in C into an
"erroneous behavior, yet defined" area.

And, for the cherry on top, if users had a way to write exactly what
they mean (per operation or per type), then we can flag exactly the
cases that are not intentional, and users can still use unchecked
operations for performance sensitive code, etc.

Cheers,
Miguel

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop
  2022-02-28 13:49                           ` Miguel Ojeda
@ 2022-03-01 20:26                             ` Linus Torvalds
  2022-03-02  7:27                               ` Martin Uecker
  0 siblings, 1 reply; 70+ messages in thread
From: Linus Torvalds @ 2022-03-01 20:26 UTC (permalink / raw)
  To: Miguel Ojeda; +Cc: Martin Uecker, linux-kernel

On Mon, Feb 28, 2022 at 5:50 AM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> But making it non-UB in the standard does not force a project to
> consider it "not an error", which is what actually matters for being
> able to use UBSan effectively or not.

Absolutely.

I think people should treat UBsan and friends a bit like "runtime lint".

"lint" traditionally doesn't necessarily check for just *incorrect* C.

It checks for things that can be confusing to humans, even if they are
100% completely conforming standard C.

Classic example: indentation. Having the wrong indentation is not in
any shape of form "undefined behavior" from a C standpoint, but it
sure is something that makes sense checking for anyway.

I think "integer overflow" should be considered the exact same thing.
It should *not* be treated as "undefined behavior", and it should not
give the compiler the option to generate code that doesn't match what
the programmer wrote.

But having a checking tool that says "This looks wrong - you just had
an integer overflow"? THAT makes 100% sense.

The C standard rules "undefined behavior" really is a problem in the standard.

                     Linus

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop
  2022-03-01 20:26                             ` Linus Torvalds
@ 2022-03-02  7:27                               ` Martin Uecker
  0 siblings, 0 replies; 70+ messages in thread
From: Martin Uecker @ 2022-03-02  7:27 UTC (permalink / raw)
  To: Linus Torvalds, Miguel Ojeda; +Cc: linux-kernel

Am Dienstag, den 01.03.2022, 12:26 -0800 schrieb Linus Torvalds:
> On Mon, Feb 28, 2022 at 5:50 AM Miguel Ojeda
> <miguel.ojeda.sandonis@gmail.com> wrote:
> > But making it non-UB in the standard does not force a project to
> > consider it "not an error", which is what actually matters for being
> > able to use UBSan effectively or not.
> 
> Absolutely.
>
> I think people should treat UBsan and friends a bit like "runtime lint".
> 
> "lint" traditionally doesn't necessarily check for just *incorrect* C.
> 
> It checks for things that can be confusing to humans, even if they are
> 100% completely conforming standard C.
>
> Classic example: indentation. Having the wrong indentation is not in
> any shape of form "undefined behavior" from a C standpoint, but it
> sure is something that makes sense checking for anyway.

You can automatically re-indent code form
other sources without breaking it. Assume you
have code that relis on signed integer wrapping,
but you want to use UBSan to screen for possible
signed arithmetic errors  and/or have it trap
in production to protect against exploits. You
would then have to carefully analyze each
individual case of signed arithmetic whether
it makes sense, and then somehow add an
annotation that it is actually ok (or rewrite
it which introduces new risks). This does not
seem comparable to indentation at all.

On the other hand, if you have a self-contained
code base and like wrapping signed integer, you
can now use a compiler flag and also get what
you want.

So I am still not yet convinced that the
standard was wrong making it undefined. 

Whether it is wise for compilers to use it
aggressively for optimization is a different
question...


Martin


^ permalink raw reply	[flat|nested] 70+ messages in thread

end of thread, other threads:[~2022-03-02  7:28 UTC | newest]

Thread overview: 70+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-17 18:48 [RFC PATCH 00/13] Proposal for speculative safe list iterator Jakob Koschel
2022-02-17 18:48 ` [RFC PATCH 01/13] list: introduce speculative safe list_for_each_entry() Jakob Koschel
2022-02-17 19:29   ` Greg Kroah-Hartman
2022-02-18 16:29     ` Jann Horn
2022-02-18 16:29   ` Jann Horn
2022-02-23 14:32     ` Jakob
2022-02-19 19:44   ` Jann Horn
2022-02-17 18:48 ` [RFC PATCH 02/13] scripts: coccinelle: adapt to find list_for_each_entry nospec issues Jakob Koschel
2022-02-17 18:48 ` [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop Jakob Koschel
2022-02-17 19:28   ` Linus Torvalds
2022-02-23 14:13     ` Jakob
2022-02-23 14:16       ` Jakob
2022-02-24 10:33         ` Greg Kroah-Hartman
2022-02-24 17:56           ` Linus Torvalds
     [not found]         ` <6d191223d93249a98511177d4af08420@pexch012b.vu.local>
2022-02-24 10:46           ` Cristiano Giuffrida
2022-02-24 11:26             ` Greg Kroah-Hartman
2022-02-23 18:47       ` Linus Torvalds
2022-02-23 19:23         ` Linus Torvalds
2022-02-23 19:43           ` Linus Torvalds
2022-02-23 20:24           ` Arnd Bergmann
2022-02-23 20:43             ` Linus Torvalds
2022-02-23 20:48               ` Arnd Bergmann
2022-02-23 21:53                 ` Linus Torvalds
2022-02-24 16:04                   ` Nathan Chancellor
2022-02-23 20:54               ` Linus Torvalds
2022-02-23 22:21                 ` David Laight
2022-02-25 21:36                 ` Uecker, Martin
2022-02-25 22:02                   ` Linus Torvalds
2022-02-26  1:21                     ` Martin Uecker
2022-02-27 18:12                       ` Miguel Ojeda
2022-02-28  7:08                         ` Martin Uecker
2022-02-28 13:49                           ` Miguel Ojeda
2022-03-01 20:26                             ` Linus Torvalds
2022-03-02  7:27                               ` Martin Uecker
2022-02-26 12:42           ` Segher Boessenkool
2022-02-26 22:14             ` Arnd Bergmann
2022-02-26 23:03               ` Linus Torvalds
2022-02-27  1:19                 ` Segher Boessenkool
2022-02-27  1:09               ` Segher Boessenkool
2022-02-27  7:10                 ` David Laight
2022-02-27 11:32                   ` Segher Boessenkool
2022-02-27 18:09                     ` Miguel Ojeda
2022-02-27 20:17                       ` Segher Boessenkool
2022-02-27 21:04                         ` Linus Torvalds
2022-02-28  6:15                           ` David Laight
2022-02-27 22:43                         ` Miguel Ojeda
2022-02-27 21:28                 ` Arnd Bergmann
2022-02-27 22:43                   ` Segher Boessenkool
2022-02-17 18:48 ` [RFC PATCH 04/13] vfio/mdev: " Jakob Koschel
2022-02-18 15:12   ` Jason Gunthorpe
2022-02-23 14:18     ` Jakob
2022-02-23 19:06       ` Linus Torvalds
2022-02-23 19:12         ` Jason Gunthorpe
2022-02-23 19:31           ` Linus Torvalds
2022-02-23 20:15             ` Jakob
2022-02-23 20:22               ` Linus Torvalds
2022-02-23 22:08                 ` Jakob
2022-02-23 20:19             ` Rasmus Villemoes
2022-02-23 20:34               ` Linus Torvalds
2022-02-17 18:48 ` [RFC PATCH 05/13] drivers/perf: " Jakob Koschel
2022-02-17 18:48 ` [RFC PATCH 06/13] ARM: mmp: " Jakob Koschel
2022-02-17 18:48 ` [RFC PATCH 07/13] udp_tunnel: " Jakob Koschel
2022-02-23 20:00   ` Christophe JAILLET
2022-02-24  6:20     ` Dan Carpenter
2022-02-17 18:48 ` [RFC PATCH 08/13] net: dsa: future proof usage of " Jakob Koschel
2022-02-17 18:48 ` [RFC PATCH 09/13] drbd: " Jakob Koschel
2022-02-17 18:48 ` [RFC PATCH 10/13] powerpc/spufs: " Jakob Koschel
2022-02-17 18:48 ` [RFC PATCH 11/13] ath6kl: remove use " Jakob Koschel
2022-02-17 18:48 ` [RFC PATCH 12/13] staging: greybus: audio: Remove usage " Jakob Koschel
2022-02-17 18:48 ` [RFC PATCH 13/13] scsi: mpt3sas: comment about invalid usage of the list iterator Jakob Koschel

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).