dma_zalloc_coherent broken with 57bf5a8963f80fb3828c46c3e3a5b2dd790e09a7

dma_zalloc_coherent broken with 57bf5a8963f80fb3828c46c3e3a5b2dd790e09a7

[Sebastian Ott]

Hi,

commit 57bf5a8963f80fb3828c46c3e3a5b2dd790e09a7 broke usage of dma
allocations specifying __GFP_ZERO by silently removing that flag.

Why should "the memory returned [] always be zeroed."?

Regards,
Sebastian

Re: dma_zalloc_coherent broken with 57bf5a8963f80fb3828c46c3e3a5b2dd790e09a7

[Geert Uytterhoeven]

Hi Sebastian,

On Tue, Mar 27, 2018 at 10:12 AM, Sebastian Ott
<sebott@linux.vnet.ibm.com> wrote:
> commit 57bf5a8963f80fb3828c46c3e3a5b2dd790e09a7 broke usage of dma
> allocations specifying __GFP_ZERO by silently removing that flag.

How did it break? The flag is now always added.

> Why should "the memory returned [] always be zeroed."?

To avoid leaking information.

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: dma_zalloc_coherent broken with 57bf5a8963f80fb3828c46c3e3a5b2dd790e09a7

[Sebastian Ott]

Hello Geert,

On Tue, 27 Mar 2018, Geert Uytterhoeven wrote:
> On Tue, Mar 27, 2018 at 10:12 AM, Sebastian Ott
> <sebott@linux.vnet.ibm.com> wrote:
> > commit 57bf5a8963f80fb3828c46c3e3a5b2dd790e09a7 broke usage of dma
> > allocations specifying __GFP_ZERO by silently removing that flag.
> 
> How did it break? The flag is now always added.

+       flag &= ~(__GFP_DMA | __GFP_DMA32 | __GFP_HIGHMEM | __GFP_ZERO);

I read that as a removal. But maybe I need more coffee..

Sebastian

Re: dma_zalloc_coherent broken with 57bf5a8963f80fb3828c46c3e3a5b2dd790e09a7

[Geert Uytterhoeven]

Hi Sebastian,

On Tue, Mar 27, 2018 at 10:41 AM, Sebastian Ott
<sebott@linux.vnet.ibm.com> wrote:
> On Tue, 27 Mar 2018, Geert Uytterhoeven wrote:
>> On Tue, Mar 27, 2018 at 10:12 AM, Sebastian Ott
>> <sebott@linux.vnet.ibm.com> wrote:
>> > commit 57bf5a8963f80fb3828c46c3e3a5b2dd790e09a7 broke usage of dma
>> > allocations specifying __GFP_ZERO by silently removing that flag.
>>
>> How did it break? The flag is now always added.
>
> +       flag &= ~(__GFP_DMA | __GFP_DMA32 | __GFP_HIGHMEM | __GFP_ZERO);
>
> I read that as a removal. But maybe I need more coffee..

Sorry, you're right.

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: dma_zalloc_coherent broken with 57bf5a8963f80fb3828c46c3e3a5b2dd790e09a7

[Christoph Hellwig]

On Tue, Mar 27, 2018 at 10:12:54AM +0200, Sebastian Ott wrote:
> Hi,
> 
> commit 57bf5a8963f80fb3828c46c3e3a5b2dd790e09a7 broke usage of dma
> allocations specifying __GFP_ZERO by silently removing that flag.
> 
> Why should "the memory returned [] always be zeroed."?

Because that is what the current implementations do - we always zero
the memory returned from dma_map_ops.alloc().

Re: dma_zalloc_coherent broken with 57bf5a8963f80fb3828c46c3e3a5b2dd790e09a7

[Sebastian Ott]

On Tue, 27 Mar 2018, Christoph Hellwig wrote:

> On Tue, Mar 27, 2018 at 10:12:54AM +0200, Sebastian Ott wrote:
> > Hi,
> > 
> > commit 57bf5a8963f80fb3828c46c3e3a5b2dd790e09a7 broke usage of dma
> > allocations specifying __GFP_ZERO by silently removing that flag.
> > 
> > Why should "the memory returned [] always be zeroed."?
> 
> Because that is what the current implementations do - we always zero
> the memory returned from dma_map_ops.alloc().

What do you mean by current implementations? Arch specific code? At least
on s390 we don't do that. dma-mapping.h doesn't do it either.

Sebastian

Re: dma_zalloc_coherent broken with 57bf5a8963f80fb3828c46c3e3a5b2dd790e09a7

[Christoph Hellwig]

On Tue, Mar 27, 2018 at 05:24:10PM +0200, Sebastian Ott wrote:
> What do you mean by current implementations? Arch specific code? At least
> on s390 we don't do that. dma-mapping.h doesn't do it either.

The arch implementations should do it.  Seems like s390 and potentially
a few others don't.  I'm a little slow due to a cast on one arm, but I'll
do an audit and will fix up anything I'll find.

Re: dma_zalloc_coherent broken with 57bf5a8963f80fb3828c46c3e3a5b2dd790e09a7

[Sebastian Ott]

On Tue, 27 Mar 2018, Christoph Hellwig wrote:
> On Tue, Mar 27, 2018 at 05:24:10PM +0200, Sebastian Ott wrote:
> > What do you mean by current implementations? Arch specific code? At least
> > on s390 we don't do that. dma-mapping.h doesn't do it either.
> 
> The arch implementations should do it.  Seems like s390 and potentially
> a few others don't.  I'm a little slow due to a cast on one arm, but I'll
> do an audit and will fix up anything I'll find.

What was wrong with the old behavior (let the caller decide - the same
as with memory allocations)? We have interfaces like dma_zalloc_coherent,
which explicitely set __GFP_ZERO. We have callers of dma_alloc followed by
a manually memset..

Sebastian

Re: dma_zalloc_coherent broken with 57bf5a8963f80fb3828c46c3e3a5b2dd790e09a7

[Christoph Hellwig]

On Tue, Mar 27, 2018 at 07:04:51PM +0200, Sebastian Ott wrote:
> What was wrong with the old behavior (let the caller decide - the same
> as with memory allocations)?

The old behavior on most (all?) mainstream architectures is that we
always zero the return value.  On x86/i386 this goes back to at least
Linux 2.4.  So common drivers simply expect this to happen.