netdev - Re: [PATCH] wl1251: dynamically allocate memory used for DMA

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 2 May 2022 16:47:22 +0200
From:   "H. Nikolaus Schaller" <hns@...delico.com>
To:     Arnd Bergmann <arnd@...db.de>
Cc:     Tony Lindgren <tony@...mide.com>, Kalle Valo <kvalo@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        linux-wireless <linux-wireless@...r.kernel.org>,
        Networking <netdev@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Discussions about the Letux Kernel 
        <letux-kernel@...nphoenux.org>, kernel@...a-handheld.com,
        linux-omap <linux-omap@...r.kernel.org>,
        Luca Coelho <luca@...lho.fi>
Subject: Re: [PATCH] wl1251: dynamically allocate memory used for DMA

Hi Arnd,

> Am 02.05.2022 um 16:06 schrieb Arnd Bergmann <arnd@...db.de>:
> 
> On Mon, May 2, 2022 at 2:38 PM H. Nikolaus Schaller <hns@...delico.com> wrote:
>> With introduction of vmap'ed stacks, stack parameters can no
>> longer be used for DMA and now leads to kernel panic.
>> 
>> It happens at several places for the wl1251 (e.g. when
>> accessed through SDIO) making it unuseable on e.g. the
>> OpenPandora.
>> 
>> We solve this by allocating temporary buffers or use wl1251_read32().
> 
> This looks all correct to me. I had another look at the related wlcore
> driver now,
> and see that the same problem existed there but was fixed back in 2012
> in a different way, see 690142e98826 ("wl12xx: fix DMA-API-related warnings").

Interesting!

> 
> The approach in the wlcore driver appears to be simpler because it
> avoids dynamic memory allocation and the associated error handling.

It looks as if it just avoids kmalloc/free sequences in event handling
by allocating a big enough buffer once.

wl1271_cmd_wait_for_event_or_timeout() allocates it like we do now.

> However, it probably makes another problem worse that also exists
> here:
> 
> static inline u32 wl1251_read32(struct wl1251 *wl, int addr)
> {
>       u32 response;
>       wl->if_ops->read(wl, addr, &wl->buffer_32, sizeof(wl->buffer_32));
>       return le32_to_cpu(wl->buffer_32);
> }
> 
> I think the 'buffer_32' member of 'struct wl1251' needs an explicit
> '__cacheline_aligned' attribute to avoid potentially clobbering
> some of the structure during a DMA write.
> 
> I don't know if anyone cares enough about the two drivers to
> have an opinion. I've added Luca to Cc, but he hasn't maintained
> the driver since 2013 and probably doesn't.

Well, there seems to be quite some common code but indeed devices
using these older chips are getting rare so it is probably not worth
combining code. And testing needs someone who owns boards
with both chips...

> 
> It's probably ok to just apply your patch for the moment to fix
> the regression we saw on the machines that we know use this.
> 
> One more detail:
> 
>> diff --git a/drivers/net/wireless/ti/wl1251/event.c b/drivers/net/wireless/ti/wl1251/event.c
>> index e6d426edab56b..e945aafd88ee5 100644
>> --- a/drivers/net/wireless/ti/wl1251/event.c
>> +++ b/drivers/net/wireless/ti/wl1251/event.c
>> @@ -169,11 +169,9 @@ int wl1251_event_wait(struct wl1251 *wl, u32 mask, int timeout_ms)
>>                msleep(1);
>> 
>>                /* read from both event fields */
>> -               wl1251_mem_read(wl, wl->mbox_ptr[0], &events_vector,
>> -                               sizeof(events_vector));
>> +               events_vector = wl1251_mem_read32(wl, wl->mbox_ptr[0]);
>>                event = events_vector & mask;
>> -               wl1251_mem_read(wl, wl->mbox_ptr[1], &events_vector,
>> -                               sizeof(events_vector));
>> +               events_vector = wl1251_mem_read32(wl, wl->mbox_ptr[1]);
>>                event |= events_vector & mask;
> 
> This appears to change endianness of the data, on big-endian kernels.
> Is that intentional?

Hm. I didn't think about it. I just noticed that wl1251_mem_read32 uses the
internal buffer so we don't have to allocate its own buffer any more.

> 
> My first guess would be that the driver never worked correctly on big-endian
> machines, and that the change is indeed correct, but on the other hand
> the conversion was added in commit ac9e2d9afa90 ("wl1251: convert
> 32-bit values to le32 before writing to the chip") in a way that suggests it
> was meant to work on both.

wl1251_event_wait() seems to work with the masks provided by code.
So I guess the conversion to le32 is harmless on the OpenPandora.
Most likely it should be done on big endian devices. I.e. we might have
done the right thing.

Let's see if someone compains or knows more. Otherwise we should
fix it just for the Pandora and N900 (both omap3 based) as the only
upstream users of this chip.

BR and thanks,
Nikolaus