| Message ID | 20220310121327.63C6FC340E8@smtp.kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | [GIT,PULL] SPI fixes for v5.17-rc7 | expand |
On Thu, Mar 10, 2022 at 4:13 AM Mark Brown <broonie@kernel.org> wrote: > > One fix for type conversion issues when working out maximum > scatter/gather segment sizes which caused problems for some systems > which where the limits overflow due to the type conversion. Side note: 'ctrl->max_dma_len' is a 'size_t', so even 'unsigned int' isn't necessarily a sufficient type. Not that I see anything setting it to values that would cause problems, but it looks conceptually like a similar problem as that 'max_seg_size' thing was. Linus
On Thu, Mar 10, 2022 at 04:25:34AM -0800, Linus Torvalds wrote: > On Thu, Mar 10, 2022 at 4:13 AM Mark Brown <broonie@kernel.org> wrote: > > One fix for type conversion issues when working out maximum > > scatter/gather segment sizes which caused problems for some systems > > which where the limits overflow due to the type conversion. > Side note: 'ctrl->max_dma_len' is a 'size_t', so even 'unsigned int' > isn't necessarily a sufficient type. Hrm, yes - thanks for spotting that.
Hi Mark, On Thu, Mar 10, 2022 at 3:46 PM Mark Brown <broonie@kernel.org> wrote: > On Thu, Mar 10, 2022 at 04:25:34AM -0800, Linus Torvalds wrote: > > On Thu, Mar 10, 2022 at 4:13 AM Mark Brown <broonie@kernel.org> wrote: > > > One fix for type conversion issues when working out maximum > > > scatter/gather segment sizes which caused problems for some systems > > > which where the limits overflow due to the type conversion. > > > Side note: 'ctrl->max_dma_len' is a 'size_t', so even 'unsigned int' > > isn't necessarily a sufficient type. > > Hrm, yes - thanks for spotting that. I had noticed while reviewing the patch, but changing to size_t wouldn't help much, as other related code paths treat the value as unsigned int anyway. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Tue, Mar 15, 2022 at 2:08 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > I had noticed while reviewing the patch, but changing to size_t wouldn't > help much, as other related code paths treat the value as unsigned int > anyway. .. but it really would. Note that the paths *after* this code don't matter. Because the result is guaranteed to fit in 'unsigned int' anyway. Put another way: min_t(unsigned int,x,y) is buggy if one of x/y is 'size_t'. Why? Because if that one gets truncated, you're doing 'min()' with a value that may be artificially much too small (that was exactly the problem commit 1a4e53d2fc4f: "spi: Fix invalid sgs value")fixed). But the situation is _not_ true in the reverse. Look: min(size_t,x,y) is guaranteed to fit in 'unsigned int' as long as _one_ of x,y fits in 'unsigned int' - even if the other doesn't. Because then 'min()' will just pick the one that already had the right size. To make it really concrete, compare min_t(unsigned int, 5, 0x100000001); min_t(size_t, 5, 0x100000001); on a 64-bit machine (ie size_t is 64-bits, and unsigned int is 32-bit). One returns 1. The other returns 5. Both fit the result in 'unsigned int', but one of them is wrong. Linus
Hi Linus, On Tue, Mar 15, 2022 at 5:48 PM Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Tue, Mar 15, 2022 at 2:08 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > I had noticed while reviewing the patch, but changing to size_t wouldn't > > help much, as other related code paths treat the value as unsigned int > > anyway. > > .. but it really would. > > Note that the paths *after* this code don't matter. Because the result > is guaranteed to fit in 'unsigned int' anyway. > > Put another way: > > min_t(unsigned int,x,y) > > is buggy if one of x/y is 'size_t'. Why? Because if that one gets > truncated, you're doing 'min()' with a value that may be artificially > much too small (that was exactly the problem commit 1a4e53d2fc4f: > "spi: Fix invalid sgs value")fixed). > > But the situation is _not_ true in the reverse. Look: > > min(size_t,x,y) > > is guaranteed to fit in 'unsigned int' as long as _one_ of x,y fits in > 'unsigned int' - even if the other doesn't. Because then 'min()' will > just pick the one that already had the right size. > > To make it really concrete, compare > > min_t(unsigned int, 5, 0x100000001); > min_t(size_t, 5, 0x100000001); > > on a 64-bit machine (ie size_t is 64-bits, and unsigned int is 32-bit). > > One returns 1. The other returns 5. Both fit the result in 'unsigned > int', but one of them is wrong. You're absolutely right. So the code should be changed to: if (vmalloced_buf || kmap_buf) { - desc_len = min_t(unsigned int, max_seg_size, PAGE_SIZE); + desc_len = min_t(unsigned long, max_seg_size, PAGE_SIZE); sgs = DIV_ROUND_UP(len + offset_in_page(buf), desc_len); } else if (virt_addr_valid(buf)) { - desc_len = min_t(unsigned int, max_seg_size, ctlr->max_dma_len); + desc_len = min_t(size_t, max_seg_size, ctlr->max_dma_len); sgs = DIV_ROUND_UP(len, desc_len); } else { return -EINVAL; } Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Tue, Mar 15, 2022 at 07:56:13PM +0100, Geert Uytterhoeven wrote: > You're absolutely right. So the code should be changed to: > > if (vmalloced_buf || kmap_buf) { > - desc_len = min_t(unsigned int, max_seg_size, PAGE_SIZE); > + desc_len = min_t(unsigned long, max_seg_size, PAGE_SIZE); > sgs = DIV_ROUND_UP(len + offset_in_page(buf), desc_len); > } else if (virt_addr_valid(buf)) { > - desc_len = min_t(unsigned int, max_seg_size, ctlr->max_dma_len); > + desc_len = min_t(size_t, max_seg_size, ctlr->max_dma_len); > sgs = DIV_ROUND_UP(len, desc_len); > } else { > return -EINVAL; > } Can you send a patch please?
Hi Mark, > Subject: Re: [GIT PULL] SPI fixes for v5.17-rc7 > > On Tue, Mar 15, 2022 at 07:56:13PM +0100, Geert Uytterhoeven wrote: > > > You're absolutely right. So the code should be changed to: > > > > if (vmalloced_buf || kmap_buf) { > > - desc_len = min_t(unsigned int, max_seg_size, > PAGE_SIZE); > > + desc_len = min_t(unsigned long, max_seg_size, > PAGE_SIZE); > > sgs = DIV_ROUND_UP(len + offset_in_page(buf), desc_len); > > } else if (virt_addr_valid(buf)) { > > - desc_len = min_t(unsigned int, max_seg_size, ctlr- > >max_dma_len); > > + desc_len = min_t(size_t, max_seg_size, ctlr- > >max_dma_len); > > sgs = DIV_ROUND_UP(len, desc_len); > > } else { > > return -EINVAL; > > } > > Can you send a patch please? I have send the patch[1] [1] https://patchwork.kernel.org/project/linux-renesas-soc/patch/20220316175317.465-1-biju.das.jz@bp.renesas.com/ Cheers, Biju
The following changes since commit 80808768e41324d2e23de89972b5406c1020e6e4: spi: rockchip: terminate dma transmission when slave abort (2022-02-17 15:33:18 +0000) are available in the Git repository at: https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git tags/spi-fix-v5.17-rc7 for you to fetch changes up to 1a4e53d2fc4f68aa654ad96d13ad042e1a8e8a7d: spi: Fix invalid sgs value (2022-03-08 12:27:33 +0000) ---------------------------------------------------------------- spi: Fix for v5.17 One fix for type conversion issues when working out maximum scatter/gather segment sizes which caused problems for some systems which where the limits overflow due to the type conversion. ---------------------------------------------------------------- Biju Das (1): spi: Fix invalid sgs value drivers/spi/spi.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)