linux-kernel - Re: [GIT PULL] sound updates for 6.6-rc1

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] [day] [month] [year] [list]

Message-ID: <878r9rsnzk.wl-tiwai@suse.de>
Date:   Thu, 31 Aug 2023 08:51:59 +0200
From:   Takashi Iwai <tiwai@...e.de>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Takashi Iwai <tiwai@...e.de>, Mark Brown <broonie@...nel.org>,
        Liam Girdwood <lgirdwood@...il.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [GIT PULL] sound updates for 6.6-rc1

On Wed, 30 Aug 2023 23:17:36 +0200,
Linus Torvalds wrote:
> 
> On Wed, 30 Aug 2023 at 04:37, Takashi Iwai <tiwai@...e.de> wrote:
> >
> > - Unified PCM copy ops with iov_iter
> 
> So I know I suggested this, but I think some of it is seriously buggy.
> 
> In particular, look at dmaengine_copy().
> 
> It was *really* completely and unacceptbly broken at one point, when it did that
> 
>         void *ptr = (void __force *)iter_iov_addr(buf);
> 
> which is complete garbage in so many ways. That was removed by commit
> 9bebd65443c1 ("ASoC: dmaengine: Use iov_iter for process callback,
> too"), and the end result looks superficially much better.
> 
> But the key word there is "superficially". The end result is still
> completely broken as far as I can see.
> 
> Why? Because the code does
> 
>         if (is_playback)
>                 if (copy_from_iter(dma_ptr, bytes, buf) != bytes)
>                         return -EFAULT;
> 
>         if (process) {
>                 int ret = process(substream, channel, hwoff, buf, bytes);
>                 if (ret < 0)
>         ...
> 
> and notice how the "is_playback" has already *used* the iter in 'buf',
> and has advanced the iterator.
> 
> So that operation is completely nonsensical.
> 
> Now, the commit message says "(although both atmel and stm drivers
> don't use the given buffer address at all for now)", which may be the
> only thing that saves the code from being broken.
> 
> Or rather, it's completely broken, but it is not broken in actual
> *noticeable* ways.
> 
> Please just remove that useless iter argument. You simply cannot do
> what that code tries to do.

OK, makes sense.

> There are alternatives, which involve either "dup_iter()" or
> "iov_iter_save_state() / iov_iter_restore()". So using an iter twice
> can be made to work, but not the way you do it.
> 
> Can I also please ask you to not use a name like "buf" for an
> iterator. It's not a buffer. You must not think of it as a buffer.
> Thinking of it as a buffer is what made the above nonsensical code
> happen.
> 
> It's literally an _iterator_. There's a buffer somewhere behind it,
> but that thing itself does *not* act as a buffer. It acts as a
> descriptor of where in the buyffer you are, which is exactly why you
> can't then re-use it twice as if it was some "buffer".
> 
> So please - when you change a buffer interface to use an iterator,
> change the variable name. Don't make mistakes like the above.

OK, will cover in the follow up patch.


thanks,

Takashi