for_each macros correctness

PATCH In review Jose Alonso 5 participants 6 messages Jan 26, 2014 – Jan 28, 2014

Subsystems (9)

SCSI Sound Block layer ASoC +5 more subsystems

lore

Share & tools

lore

Jose Alonso

author

2014-01-26 11:01 UTC #0 lore raw

PATCH 1/1 for_each macros correctness

I observed that there are for_each macros that do an extra memory access beyond the defined area.
Normally this does not cause problems.
But, this can cause exceptions. For example: if the area is allocated at the end of a page and the next page is not accessible.

For correctness, I suggest changing the arguments of the 'for loop' like others 'for_each' do in the kernel.

files involved:
drivers/s390/cio/qdio.h
drivers/scsi/isci/host.h
drivers/sh/clk/core.c
include/linux/blk-mq.h
include/linux/shdma-base.h
sound/soc/sh/rcar/adg.c

Signed-off-by Jose Alonso <[email protected]>

---

drivers/s390/cio/qdio.h +6 -8

index 8acaae1..a563e4c 100644
@@ -359,14 +359,12 @@ static inline int multicast_outbound(struct qdio_q *q)
 #define need_siga_sync_out_after_pci(q)	\
 	(unlikely(q->irq_ptr->siga_flag.sync_out_after_pci))
 
-#define for_each_input_queue(irq_ptr, q, i)	\
-	for (i = 0, q = irq_ptr->input_qs[0];	\
-		i < irq_ptr->nr_input_qs;	\
-		q = irq_ptr->input_qs[++i])
-#define for_each_output_queue(irq_ptr, q, i)	\
-	for (i = 0, q = irq_ptr->output_qs[0];	\
-		i < irq_ptr->nr_output_qs;	\
-		q = irq_ptr->output_qs[++i])
+#define for_each_input_queue(irq_ptr, q, i)		\
+	for (i = 0; i < irq_ptr->nr_input_qs &&		\
+		({ q = irq_ptr->input_qs[i]; 1; }); i++)
+#define for_each_output_queue(irq_ptr, q, i)		\
+	for (i = 0; i < irq_ptr->nr_output_qs &&	\
+		({ q = irq_ptr->output_qs[i]; 1; }); i++)
 
 #define prev_buf(bufnr)	\
 	((bufnr + QDIO_MAX_BUFFERS_MASK) & QDIO_MAX_BUFFERS_MASK)

drivers/scsi/isci/host.h +2 -3

index 4911310..99c5a69 100644
@@ -311,9 +311,8 @@ static inline struct Scsi_Host *to_shost(struct isci_host *ihost)
 }
 
 #define for_each_isci_host(id, ihost, pdev) \
-	for (id = 0, ihost = to_pci_info(pdev)->hosts[id]; \
-	     id < ARRAY_SIZE(to_pci_info(pdev)->hosts) && ihost; \
-	     ihost = to_pci_info(pdev)->hosts[++id])
+	for (id = 0; id < ARRAY_SIZE(to_pci_info(pdev)->hosts) && \
+	     (ihost = to_pci_info(pdev)->hosts[id]); id++)
 
 static inline void wait_for_start(struct isci_host *ihost)
 {

drivers/sh/clk/core.c +2 -2

index 7472785..6dd3cbb 100644
@@ -82,8 +82,8 @@ struct clk_rate_round_data {
 };
 
 #define for_each_frequency(pos, r, freq)			\
-	for (pos = r->min, freq = r->func(pos, r);		\
-	     pos <= r->max; pos++, freq = r->func(pos, r))	\
+	for (pos = r->min; pos <= r->max &&			\
+	     ({ freq = r->func(pos, r); 1; }); pos++)		\
 		if (unlikely(freq == 0))			\
 			;					\
 		else

include/linux/blk-mq.h +6 -6

index ab0e9b2..a4b40cf 100644
@@ -159,16 +159,16 @@ static inline struct request *blk_mq_tag_to_rq(struct blk_mq_hw_ctx *hctx,
 }
 
 #define queue_for_each_hw_ctx(q, hctx, i)				\
-	for ((i) = 0, hctx = (q)->queue_hw_ctx[0];			\
-	     (i) < (q)->nr_hw_queues; (i)++, hctx = (q)->queue_hw_ctx[i])
+	for ((i) = 0; (i) < (q)->nr_hw_queues &&			\
+	     ({ hctx = (q)->queue_hw_ctx[i]; 1; }); (i)++)
 
 #define queue_for_each_ctx(q, ctx, i)					\
-	for ((i) = 0, ctx = per_cpu_ptr((q)->queue_ctx, 0);		\
-	     (i) < (q)->nr_queues; (i)++, ctx = per_cpu_ptr(q->queue_ctx, (i)))
+	for ((i) = 0; (i) < (q)->nr_queues &&				\
+	     ({ ctx = per_cpu_ptr((q)->queue_ctx, (i)); 1; }); (i)++)
 
 #define hctx_for_each_ctx(hctx, ctx, i)					\
-	for ((i) = 0, ctx = (hctx)->ctxs[0];				\
-	     (i) < (hctx)->nr_ctx; (i)++, ctx = (hctx)->ctxs[(i)])
+	for ((i) = 0; (i) < (hctx)->nr_ctx &&				\
+	     ({ ctx = (hctx)->ctxs[(i)]; 1; }); (i)++)
 
 #define blk_ctx_sum(q, sum)						\
 ({									\

include/linux/shdma-base.h +2 -2

index f92c0a4..35285f7 100644
@@ -111,8 +111,8 @@ struct shdma_dev {
 	size_t desc_size;
 };
 
-#define shdma_for_each_chan(c, d, i) for (i = 0, c = (d)->schan[0]; \
-				i < (d)->dma_dev.chancnt; c = (d)->schan[++i])
+#define shdma_for_each_chan(c, d, i) for (i = 0; i < (d)->dma_dev.chancnt && \
+				({ c = (d)->schan[i]; 1; }); i++)
 
 int shdma_request_irq(struct shdma_chan *, int,
 			   unsigned long, const char *);

sound/soc/sh/rcar/adg.c +2 -3

index a53235c..ef19c15 100644
@@ -25,9 +25,8 @@ struct rsnd_adg {
 };
 
 #define for_each_rsnd_clk(pos, adg, i)		\
-	for (i = 0, (pos) = adg->clk[i];	\
-	     i < CLKMAX;			\
-	     i++, (pos) = adg->clk[i])
+	for (i = 0; i < CLKMAX &&		\
+	     ({ (pos) = adg->clk[i]; 1; }); i++)
 #define rsnd_priv_to_adg(priv) ((struct rsnd_adg *)(priv)->adg)
 
 static int rsnd_adg_set_convert_clk_gen1(struct rsnd_priv *priv,

Fubo Chen

2014-01-26 13:39 UTC #1 lore raw

Quoted 8 lines · Jose Alonso

I observed that there are for_each macros that do an extra memory access beyond the defined area.
Normally this does not cause problems.
But, this can cause exceptions. For example: if the area is allocated at the end of a page and the next page is not accessible.
For correctness, I suggest changing the arguments of the 'for loop' like others 'for_each' do in the kernel.

Does this patch fix a kernel crash when using gcc 4.8 like the patch in http://lkml.org/lkml/2014/1/21/146 ?

Thanks,

Fubo.

Jose Alonso

author

2014-01-26 18:05 UTC #2 lore raw

Quoted 7 lines · Fubo Chen

Quoted 6 lines · Jose Alonso
I observed that there are for_each macros that do an extra memory access beyond the defined area.
Normally this does not cause problems.
But, this can cause exceptions. For example: if the area is allocated at the end of a page and the next page is not accessible.

Quoted 5 lines

Quoted 2 lines
For correctness, I suggest changing the arguments of the 'for loop' like others 'for_each' do in the kernel.
Does this patch fix a kernel crash when using gcc 4.8 like the patch in http://lkml.org/lkml/2014/1/21/146 ?

The patch for 'for_each_isci_host' is equivalent.

I followed the link above, and as I understand: The access to to_pci_info(pdev)->hosts[2] do not caused exception, but the fact of hosts[2] is "logically accessed" "confused" the gcc 4.8 compiler. (The gcc compiler do very aggressive loop optimization and can eliminate loops executing each pass linearly).

Dorau, Lukasz

2014-01-27 09:26 UTC #3 lore raw

In reply to Jose Alonso

Quoted 6 lines · Jose Alonso

I observed that there are for_each macros that do an extra memory access beyond the defined area.
Normally this does not cause problems.
But, this can cause exceptions. For example: if the area is allocated at the end of a page and the next page is not accessible.

Quoted 6 lines

For correctness, I suggest changing the arguments of the 'for loop' like others 'for_each' do in the kernel.
files involved:
   drivers/s390/cio/qdio.h
   drivers/scsi/isci/host.h

Hi Jose,

The macro in "drivers/scsi/isci/host.h" has already been corrected:
http://marc.info/?l=linux-kernel&m=139030404415970&w=2
So you can remove the part regarding "drivers/scsi/isci/host.h" from your patch.

Lukasz

��{.n�+��+%��ݶ��w��{.n�+��{��G��{ay�ʇڙ�,j��f��h��z_�(�階�ݢj"��m��G��?��&��~��iO��z��v�^�m��?�I�

Jens Axboe

maintainer

2014-01-27 20:02 UTC #4 lore raw

In reply to Jose Alonso

Quoted 10 lines · Jose Alonso

I observed that there are for_each macros that do an extra memory access beyond the defined area.
Normally this does not cause problems.
But, this can cause exceptions. For example: if the area is allocated at the end of a page and the next page is not accessible.
For correctness, I suggest changing the arguments of the 'for loop' like others 'for_each' do in the kernel.

Quoted 7 lines

files involved:

   drivers/s390/cio/qdio.h
   drivers/scsi/isci/host.h
   drivers/sh/clk/core.c
   include/linux/blk-mq.h
   include/linux/shdma-base.h
   sound/soc/sh/rcar/adg.c

Thanks, I'll dig out the blk-mq bit.

Signature · 1 line

Jens Axboe

Heiko Carstens

2014-01-28 12:02 UTC #5 lore raw

In reply to Jose Alonso

Quoted 33 lines · Jose Alonso

I observed that there are for_each macros that do an extra memory access beyond the defined area.
Normally this does not cause problems.
But, this can cause exceptions. For example: if the area is allocated at the end of a page and the next page is not accessible.
For correctness, I suggest changing the arguments of the 'for loop' like others 'for_each' do in the kernel.
files involved:
   drivers/s390/cio/qdio.h
   drivers/scsi/isci/host.h
   drivers/sh/clk/core.c
   include/linux/blk-mq.h
   include/linux/shdma-base.h
   sound/soc/sh/rcar/adg.c
Signed-off-by Jose Alonso <[email protected]>
---
drivers/s390/cio/qdio.h +0 -4
index 8acaae1..a563e4c 100644
@@ -359,14 +359,12 @@ static inline int multicast_outbound(struct qdio_q *q)
 #define need_siga_sync_out_after_pci(q)	\
 	(unlikely(q->irq_ptr->siga_flag.sync_out_after_pci))
 
-#define for_each_input_queue(irq_ptr, q, i)	\
-	for (i = 0, q = irq_ptr->input_qs[0];	\
-		i < irq_ptr->nr_input_qs;	\
-		q = irq_ptr->input_qs[++i])

drivers/s390/cio/qdio.h static inline int multicast_outbound(struct qdio_q *q) · line 367

Quoted 10 lines

drivers/s390/cio/qdio.h +6 -4

-#define for_each_output_queue(irq_ptr, q, i)	\
-	for (i = 0, q = irq_ptr->output_qs[0];	\
-		i < irq_ptr->nr_output_qs;	\
-		q = irq_ptr->output_qs[++i])
+#define for_each_input_queue(irq_ptr, q, i)		\
+	for (i = 0; i < irq_ptr->nr_input_qs &&		\
+		({ q = irq_ptr->input_qs[i]; 1; }); i++)
+#define for_each_output_queue(irq_ptr, q, i)		\
+	for (i = 0; i < irq_ptr->nr_output_qs &&	\
+		({ q = irq_ptr->output_qs[i]; 1; }); i++)

Hi Jose,

I generated a single commit of this part of your patch and applied it to the s390 git tree.

Thanks!