Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210704150025.GC21572@xsang-OptiPlex-9020> <20210705125756.GA25141@lst.de>
 <CAHk-=wj_Gfqkdp+K3iCiqMjAZQK_BrRWDs2eOS_BAw=bB=CdRw@mail.gmail.com> <20210706143647.GA28289@lst.de>
In-Reply-To: <20210706143647.GA28289@lst.de>
From:   Linus Torvalds <torvalds@linux-foundation.org>
Date:   Tue, 6 Jul 2021 12:08:42 -0700
Message-ID: <CAHk-=wgPyx7tHFNaO2N6bsaB_E6gL+t1uDAmrD91jJw+hiTvrQ@mail.gmail.com>
Subject: Re: [ide] b7fb14d3ac: EIP:ioread32_rep
To:     Christoph Hellwig <hch@lst.de>
Cc:     kernel test robot <oliver.sang@intel.com>,
        Jens Axboe <axboe@kernel.dk>,
        LKML <linux-kernel@vger.kernel.org>, lkp@lists.01.org,
        kernel test robot <lkp@intel.com>,
        "H. Peter Anvin" <hpa@zytor.com>, Borislav Petkov <bp@alien8.de>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Jul 6, 2021 at 7:36 AM Christoph Hellwig <hch@lst.de> wrote:
>
> Yeah, there's usually a huge offset into the page.  The otherwise
> similar ATAPI code actually has checks to chunk it up and not cross
> page boundaries, and copying that over fixes the problem.

Ok.

Your patch made me go "I think it should loop until it has transferred
the full 512 bytes", but maybe the caller loops properly?

Or should it just discard the end?

HOWEVER.

Even if the caller does loop/discard properly, your patch still worries me:

> +       /* don't cross page boundaries */
> +       count = min(count, (unsigned int)PAGE_SIZE - offset);

Is the offset guaranteed to be 4-byte aligned?

Because I'm looking at ata_sff_data_xfer32(), and I think it
fundamentally would fail the "retry after partial 4-byte transfer".

Let's imagine that "offset" is 511 bytes off the end of the page, and
so you'd first do a 511-byte transfer, and then a 1-byte transfer.

That's not how ata_sff_data_xfer32() works. It would actually first do
a 508-byte transfer (using that "rep insl" to do 4 bytes at a time),
and then it would do a 4-byte transfer into a temporary buffer, and
copy the first three bytes to fill out the 511 bytes in the first
page.

If you then loop back to do the last byte, it would do another 4-byte
transfer into a temporary buffer, and copy the remaining byte - ending
up with 512 bytes result as asked for.

Except they wouldn't be the *RIGHT* 512 bytes. It would have done 516
bytes worth of "inl", and from those 516 bytes it would have filled
the last 4 bytes with basically random garbage (ok, the first three
bytes would be ok, but the last byte would not be).

So I think that ap->ops->sff_data_xfer fundamentally cannot handle a
page crosser correctly - at least not if it's not 4-byte aligned.

How does IO to a non-sector-aligned buffer eevr happen? Because I
think that's broken, and your patch is only hiding further bugs.

                    Linus