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

Message-ID: <CAHk-=wje+VkXjjfVTmK-uJdG_M5=ar14QxAwK+XDiq07k_pzBg@mail.gmail.com>
Date:   Wed, 30 Aug 2023 14:17:36 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Takashi Iwai <tiwai@...e.de>
Cc:     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 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.

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.

                 Linus