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]

Message-ID: <CAK8P3a0owDwLYJ1nk6HciV=15wecC6QHPY-QiseRmeRnahcXXQ@mail.gmail.com>
Date:   Mon, 2 May 2022 17:04:51 +0200
From:   Arnd Bergmann <arnd@...db.de>
To:     "H. Nikolaus Schaller" <hns@...delico.com>
Cc:     Arnd Bergmann <arnd@...db.de>, 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

On Mon, May 2, 2022 at 4:47 PM H. Nikolaus Schaller <hns@...delico.com> wrote:
> > 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:
> >
> > 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.

Ah right, I missed that one.

> > 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...

No, I wasn't even thinking of combining code, just whether there
is value in keeping the similar bits in sync to ensure we have the
same set of bugs on both ;-)

I think the best fix for both drivers would be to keep the DMA
members (partition and buffer_32) in the respective device
structures, but mark each one as aligned.

> > 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.

Ok. In general I like ensure we keep things working for big-endian
kernels, which are meant to work on any ARMv6+ or newer machine.
Most of the time it's just a couple of drivers (like this one) that get
in the way of actually doing it, but then again very few people ever
care about big-endian ARMv6/v7.

If we don't have a reason to believe this one is actually wrong, I think
fixing the endian issue silently is fine, as is ignoring the potential
other endian issues in the same driver that nobody complained about
in the past decade ;-)

       Arnd