[GIT PULL] dma-mapping fix for 5.10

[GIT PULL] dma-mapping fix for 5.10

[Christoph Hellwig]

The following changes since commit ed8780e3f2ecc82645342d070c6b4e530532e680:

  Merge tag 'x86-urgent-2020-10-27' of git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip (2020-10-27 14:39:29 -0700)

are available in the Git repository at:

  git://git.infradead.org/users/hch/dma-mapping.git tags/dma-mapping-5.10-2

for you to fetch changes up to 48ab6d5d1f096d6fac5b59f94af0aa394115a001:

  dma-mapping: fix 32-bit overflow with CONFIG_ARM_LPAE=n (2020-10-29 16:59:34 +0100)

----------------------------------------------------------------
dma-mapping fix for 5.10:

 - fix an integer overflow on 32-bit platforms in the new DMA range code
   (Geert Uytterhoeven)

----------------------------------------------------------------
Geert Uytterhoeven (1):
      dma-mapping: fix 32-bit overflow with CONFIG_ARM_LPAE=n

 drivers/of/device.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Re: [GIT PULL] dma-mapping fix for 5.10

[Linus Torvalds]

On Sat, Oct 31, 2020 at 2:40 AM Christoph Hellwig <hch@infradead.org> wrote:
>
> dma-mapping fix for 5.10:
>
>  - fix an integer overflow on 32-bit platforms in the new DMA range code
>    (Geert Uytterhoeven)

So this is just a stylistic nit, and has no impact on this pull (which
I've done). But looking at the patch, it triggers one of my "this is
wrong" patterns.

In particular, this:

        u64 dma_start = 0;
        ...
        for (dma_start = ~0ULL; r->size; r++) {

is actually completely bogus in theory, and it's a horribly horribly
bad pattern to have.

The thing that I hate about that parttern is "~0ULL", which is simply wrong.

The correct pattern for "all bits set" is ~0. NOTHING ELSE. No extra
letters at the end.

Why? Because using an unsigned type is wrong, and will not extend the
bits up to a potentially bigger size.

So adding that "ULL" is not just three extra characters to type, it
actually _detracts_ from the code and makes it more fragile and
potentially wrong.

It so happens, that yes, in the kernel, "ull" us 64-bit, and you get
the right results. So the code _works_. But it's wrong, and it now
requires that the types match exactly (ie it would not be broken if
somebody ever were to say "I want to use use 128-bit dma addresses and
u128").

Another example is using "~0ul", which would give different results on
a 32-bit kernel and a 64-bit kernel. Again: DON'T DO THAT.

I repeat: the right thing to do for "all bits set" is just a plain ~0
or -1. Either of those are fine (technically assumes a 2's complement
machine, but let's just be honest: that's a perfectly fine assumption,
and -1 might be preferred by some because it makes that sign extension
behavior of the integer constant more obvious).

Don't try to do anything clever or anything else, because it's going
to be strictly worse.

The old code that that patch removed was "technically correct", but
just pointless, and actually shows the problem:

        for (dma_start = ~(dma_addr_t)0; r->size; r++) {

the above is indeed a correct way to say "I want all bits set in a
dma_addr_t", but while correct, it is - once again - strictly inferior
to just using "~0".

Why? Because "~0" works regardless of type. IOW, exactly *because*
people used the wrong pattern for "all bits set", that patch was now
(a) bigger than necessary and (b) much more ilkely to cause bugs (ie I
could have imagined people changing just the type of the variable
without changing the initialization).

So in that tiny three-line patch there were actually several examples
of why "~0" is the right pattern to use for "all bits set". Because it
JustWorks(tm) in ways other patterns do not.

And if you have a compiler that complains about assigning -1 or ~0 to
an unsigned variable, get rid of that piece of garbage. You're almost
certainly either using some warning flag that you shouldn't be using,
or the compiler writer didn't know what they were doing.

            Linus

Re: [GIT PULL] dma-mapping fix for 5.10

[Christoph Hellwig]

On Sat, Oct 31, 2020 at 12:50:44PM -0700, Linus Torvalds wrote:
> So this is just a stylistic nit, and has no impact on this pull (which
> I've done). But looking at the patch, it triggers one of my "this is
> wrong" patterns.

Adding the author and maintainer of that code so that they can sort it
out.

> 
> In particular, this:
> 
>         u64 dma_start = 0;
>         ...
>         for (dma_start = ~0ULL; r->size; r++) {
> 
> is actually completely bogus in theory, and it's a horribly horribly
> bad pattern to have.
> 
> The thing that I hate about that parttern is "~0ULL", which is simply wrong.
> 
> The correct pattern for "all bits set" is ~0. NOTHING ELSE. No extra
> letters at the end.
> 
> Why? Because using an unsigned type is wrong, and will not extend the
> bits up to a potentially bigger size.
> 
> So adding that "ULL" is not just three extra characters to type, it
> actually _detracts_ from the code and makes it more fragile and
> potentially wrong.
> 
> It so happens, that yes, in the kernel, "ull" us 64-bit, and you get
> the right results. So the code _works_. But it's wrong, and it now
> requires that the types match exactly (ie it would not be broken if
> somebody ever were to say "I want to use use 128-bit dma addresses and
> u128").
> 
> Another example is using "~0ul", which would give different results on
> a 32-bit kernel and a 64-bit kernel. Again: DON'T DO THAT.
> 
> I repeat: the right thing to do for "all bits set" is just a plain ~0
> or -1. Either of those are fine (technically assumes a 2's complement
> machine, but let's just be honest: that's a perfectly fine assumption,
> and -1 might be preferred by some because it makes that sign extension
> behavior of the integer constant more obvious).
> 
> Don't try to do anything clever or anything else, because it's going
> to be strictly worse.
> 
> The old code that that patch removed was "technically correct", but
> just pointless, and actually shows the problem:
> 
>         for (dma_start = ~(dma_addr_t)0; r->size; r++) {
> 
> the above is indeed a correct way to say "I want all bits set in a
> dma_addr_t", but while correct, it is - once again - strictly inferior
> to just using "~0".
> 
> Why? Because "~0" works regardless of type. IOW, exactly *because*
> people used the wrong pattern for "all bits set", that patch was now
> (a) bigger than necessary and (b) much more ilkely to cause bugs (ie I
> could have imagined people changing just the type of the variable
> without changing the initialization).
> 
> So in that tiny three-line patch there were actually several examples
> of why "~0" is the right pattern to use for "all bits set". Because it
> JustWorks(tm) in ways other patterns do not.
> 
> And if you have a compiler that complains about assigning -1 or ~0 to
> an unsigned variable, get rid of that piece of garbage. You're almost
> certainly either using some warning flag that you shouldn't be using,
> or the compiler writer didn't know what they were doing.
> 
>             Linus
> _______________________________________________
> iommu mailing list
> iommu@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu
---end quoted text---

Re: [GIT PULL] dma-mapping fix for 5.10

[Geert Uytterhoeven]

Hi Linus,

On Sat, Oct 31, 2020 at 8:51 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Sat, Oct 31, 2020 at 2:40 AM Christoph Hellwig <hch@infradead.org> wrote:
> > dma-mapping fix for 5.10:
> >
> >  - fix an integer overflow on 32-bit platforms in the new DMA range code
> >    (Geert Uytterhoeven)
>
> So this is just a stylistic nit, and has no impact on this pull (which
> I've done). But looking at the patch, it triggers one of my "this is
> wrong" patterns.
>
> In particular, this:
>
>         u64 dma_start = 0;
>         ...
>         for (dma_start = ~0ULL; r->size; r++) {
>
> is actually completely bogus in theory, and it's a horribly horribly
> bad pattern to have.
>
> The thing that I hate about that parttern is "~0ULL", which is simply wrong.
>
> The correct pattern for "all bits set" is ~0. NOTHING ELSE. No extra
> letters at the end.
>
> Why? Because using an unsigned type is wrong, and will not extend the
> bits up to a potentially bigger size.
>
> So adding that "ULL" is not just three extra characters to type, it
> actually _detracts_ from the code and makes it more fragile and
> potentially wrong.
>
> It so happens, that yes, in the kernel, "ull" us 64-bit, and you get
> the right results. So the code _works_. But it's wrong, and it now
> requires that the types match exactly (ie it would not be broken if
> somebody ever were to say "I want to use use 128-bit dma addresses and
> u128").

Thanks, you're right, the "ULL" suffix is not needed, and could cause
future issues.

> Another example is using "~0ul", which would give different results on
> a 32-bit kernel and a 64-bit kernel. Again: DON'T DO THAT.

Definitely.

> I repeat: the right thing to do for "all bits set" is just a plain ~0
> or -1. Either of those are fine (technically assumes a 2's complement
> machine, but let's just be honest: that's a perfectly fine assumption,
> and -1 might be preferred by some because it makes that sign extension
> behavior of the integer constant more obvious).

"-1" definitely causes warnings, depending on your compiler (not with
the gcc 9.3.0 I'm currently using, though).

> Don't try to do anything clever or anything else, because it's going
> to be strictly worse.
>
> The old code that that patch removed was "technically correct", but
> just pointless, and actually shows the problem:
>
>         for (dma_start = ~(dma_addr_t)0; r->size; r++) {
>
> the above is indeed a correct way to say "I want all bits set in a
> dma_addr_t", but while correct, it is - once again - strictly inferior
> to just using "~0".

Obviously I was misled by the old code, and instead of changing
the cast, I replaced the cast ("casts are evil") by a suffix. Doh.

Any, I've just sent a patch. Thanks!

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

Re: [GIT PULL] dma-mapping fix for 5.10

[pr-tracker-bot]

The pull request you sent on Sat, 31 Oct 2020 10:38:23 +0100:

> git://git.infradead.org/users/hch/dma-mapping.git tags/dma-mapping-5.10-2

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/bb3540be73ca1e483aa977d859960895fe85372d

Thank you!

-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/prtracker.html