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: <20201104082738.1054792-1-hch@lst.de> <20201104082738.1054792-2-hch@lst.de>
 <20201110213253.GV3576660@ZenIV.linux.org.uk> <20201110213511.GW3576660@ZenIV.linux.org.uk>
 <20201110232028.GX3576660@ZenIV.linux.org.uk>
In-Reply-To: <20201110232028.GX3576660@ZenIV.linux.org.uk>
From:   Linus Torvalds <torvalds@linux-foundation.org>
Date:   Wed, 11 Nov 2020 09:54:12 -0800
Message-ID: <CAHk-=whTqr4Lp0NYR6k3yc2EbiF0RR17=TJPa4JBQATMR__XqA@mail.gmail.com>
Subject: Re: [PATCH 1/6] seq_file: add seq_read_iter
To:     Al Viro <viro@zeniv.linux.org.uk>
Cc:     Christoph Hellwig <hch@lst.de>,
        Greg KH <gregkh@linuxfoundation.org>,
        Alexey Dobriyan <adobriyan@gmail.com>,
        linux-fsdevel <linux-fsdevel@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Nov 10, 2020 at 3:20 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> Any objections to the following?

Well, I don't _object_, but I find it ugly.

And I think both the old and the "fixed" code is wrong when an EFAULT
happens in the middle.

Yes, we can just return EFAULT. But for read() and write() we really
try to do the proper partial returns in other places, why not here?

IOW, why isn't the proper fix just something like this:

    diff --git a/fs/seq_file.c b/fs/seq_file.c
    index 3b20e21604e7..ecc6909b71f5 100644
    --- a/fs/seq_file.c
    +++ b/fs/seq_file.c
    @@ -209,7 +209,8 @@ ssize_t seq_read_iter(struct kiocb *iocb,
struct iov_iter *iter)
        /* if not empty - flush it first */
        if (m->count) {
                n = min(m->count, size);
    -           if (copy_to_iter(m->buf + m->from, n, iter) != n)
    +           n = copy_to_iter(m->buf + m->from, n, iter);
    +           if (!n)
                        goto Efault;
                m->count -= n;
                m->from += n;

which should get the "efault in the middle" case roughly right (ie the
usual "exact byte alignment and page crosser" caveats apply).

               Linus