dw-axi-dmac-platform: Avoid trampling with zero length buffer #6118
Conversation
|
This is the forum bug report: Let me test this at home. In the office the TV signal is very low, so I wasn't able to run for long, |
|
Testing at home is positive. @pelwell do you agree with my logic? |
| @@ -918,6 +918,9 @@ dw_axi_dma_chan_prep_slave_sg(struct dma_chan *dchan, struct scatterlist *sgl, | |||
| num_segments = DIV_ROUND_UP(sg_dma_len(sg), axi_block_len); | |||
| segment_len = DIV_ROUND_UP(sg_dma_len(sg), num_segments); | |||
There was a problem hiding this comment.
If num_segments can be zero on the next line, why didn't we get a division-by-zero error here?
There was a problem hiding this comment.
Looking at a disassembly of the output, the compiler seems to have moved the (num_segments == 0) test (which should by (!num_segments) BTW) between the two DIV_ROUND_UP calls, hence the lack of an exception.
There was a problem hiding this comment.
There was no divide by zero exception before this PR where the num_segments == 0 protection wasn't present, nor with logging added which printed the result of the divide by zero (which was 0).
For interest, @pelwell did find:
ARM Architecture Reference Manual, ARMv8 [ARM DDI 0487B.b]
http://infocenter.arm.com/help/topic/com.arm.doc.ddi0487b.b/DDI0487B_b_armv8_arm.pdf
1) aarch64 instruction set: section C3.4.7 and C6.2.279 (UDIV)
"A division by zero results in a zero being written to
the destination register, without any indication that
the division by zero occurred."
2) aarch32 instruction set: section F1.4.8 and F5.1.263 (UDIV)
"For the SDIV and UDIV instructions, division by zero
always returns a zero result."
There was a problem hiding this comment.
The !num_segments test should go before the segment_len calculation, if only for appearances, but otherwise I agree with this commit and the reasoning behind it.
This code: for_each_sg(sgl, sg, sg_len, i) num_sgs += DIV_ROUND_UP(sg_dma_len(sg), axi_block_len); determines how many hw_desc are allocated. If sg_dma_len(sg)=0 we don't allocate for this sgl. However in the next loop, we will increment loop for this case, and loop gets higher than num_sgs and we trample memory. Signed-off-by: Dom Cobley <popcornmix@gmail.com>
|
Moved the test up a line to avoid the potential divide by zero. |
See: raspberrypi/linux#6113 kernel: DRM: rp1: rp1-dsi: Fix escape clock divider and timeouts See: raspberrypi/linux#6120 kernel: vc4/hdmi: Ignore hotplug interrupt with force_hotplug See: raspberrypi/linux#6123 kernel: drivers: media: cfe: Add remap entries for mono formats See: raspberrypi/linux#6122 kernel: dw-axi-dmac-platform: Avoid trampling with zero length buffer See: raspberrypi/linux#6118 kernel: Add a strict_gpiod option See: raspberrypi/linux#6117 kernel: Unicam FS/FE timing GPIO See: raspberrypi/linux#6088
See: raspberrypi/linux#6113 kernel: DRM: rp1: rp1-dsi: Fix escape clock divider and timeouts See: raspberrypi/linux#6120 kernel: vc4/hdmi: Ignore hotplug interrupt with force_hotplug See: raspberrypi/linux#6123 kernel: drivers: media: cfe: Add remap entries for mono formats See: raspberrypi/linux#6122 kernel: dw-axi-dmac-platform: Avoid trampling with zero length buffer See: raspberrypi/linux#6118 kernel: Add a strict_gpiod option See: raspberrypi/linux#6117 kernel: Unicam FS/FE timing GPIO See: raspberrypi/linux#6088
This code:
determines how many hw_desc are allocated.
If
sg_dma_len(sg)=0we don't allocate for this sgl.However in the next loop, we will increment loop
for this case, and loop gets higher than num_sgs
and we trample memory.