On Wed, Sep 07, 2022 at 08:52:46AM -0400, Benjamin Coddington wrote:
> On 7 Sep 2022, at 0:58, Chuck Lever III wrote:
>
> > > On Sep 6, 2022, at 3:12 PM, Olga Kornievskaia <aglo@xxxxxxxxx> wrote:
> > >
> > > On Tue, Sep 6, 2022 at 2:28 PM Benjamin Coddington
> > > <bcodding@xxxxxxxxxx> wrote:
> > > >
> > > > On 1 Sep 2022, at 21:27, Olga Kornievskaia wrote:
> > > >
> > > > > Thanks Chuck. I first, based on a hunch, narrowed down that it's
> > > > > coming from Al Viro's merge commit. Then I git bisected his
> > > > > 32patches
> > > > > to the following commit f0f6b614f83dbae99d283b7b12ab5dd2e04df979
> > > >
> > > > No crash for me after reverting
> > > > f0f6b614f83dbae99d283b7b12ab5dd2e04df979.
> > >
> > > I second that. No crash after a revert here.
> >
> > I bisected the new xfstests failures to the same commit:
> >
> > f0f6b614f83dbae99d283b7b12ab5dd2e04df979 is the first bad commit
> > commit f0f6b614f83dbae99d283b7b12ab5dd2e04df979
> > Author: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
> > Date: Thu Jun 23 17:21:37 2022 -0400
> >
> > copy_page_to_iter(): don't split high-order page in case of
> > ITER_PIPE
> >
> > ... just shove it into one pipe_buffer.
> >
> > Signed-off-by: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
> >
> > lib/iov_iter.c | 21 ++++++---------------
> > 1 file changed, 6 insertions(+), 15 deletions(-)
> >
>
> I've been reliably reproducing this on generic/551 on xfs. In the case
> where it crashes, rqstp->rq_res.page_base is positive multiple of PAGE_SIZE
> after getting set in nfsd_splice_actor(), and that with page_len overruns
> the 256 pages we have.
>
> With f0f6b614f83d reverted, rqstp->rq_res.page_base is always zero.
>
> After 47b7fcae419dc and f0f6b614f83d, buf->offset in nfsd_splice_actor can
> be a positive multiple of PAGE_SIZE, whereas before it was always just the
> offset into the page.
>
> Something like this might fix it up:
>
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index 9f486b788ed0..d62963f36f03 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -849,7 +849,7 @@ nfsd_splice_actor(struct pipe_inode_info *pipe, struct
> pipe_buffer *buf,
>
> svc_rqst_replace_page(rqstp, buf->page);
> if (rqstp->rq_res.page_len == 0)
> - rqstp->rq_res.page_base = buf->offset;
> + rqstp->rq_res.page_base = buf->offset % PAGE_SIZE;
> rqstp->rq_res.page_len += sd->len;
> return sd->len;
> }
>
> .. but we should check with Al about whether this needs to be fixed over in
> copy_page_to_iter_pipe(), since I don't think pipe_buffer offset should be
> greater than PAGE_SIZE.
>
> Al, any thoughts?
pipe_buffer offsets in general can be greater than PAGE_SIZE. What's more,
buffer *size* can be greater than PAGE_SIZE - it really can contain more
than PAGE_SIZE worth of data. In that case the page is a compound one, of
course.
Regression is the combination of "splice from regular file to pipe hadn't
produced that earlier, now it does" and "nfsd never needed to handle that".
I don't believe that fix is correct. *IF* you can't deal with compound
page in sunrpc, you need a loop going by subpages in nfsd_splice_actor(),
similar to one that used to be in copy_page_to_iter(). Could you try
the following:
nfsd_splice_actor(): handle compound pages
pipe_buffer might refer to a compound page (and contain more than a PAGE_SIZE
worth of data). Theoretically it had been possible since way back, but
nfsd_splice_actor() hadn't run into that until copy_page_to_iter() change.
Fortunately, the only thing that changes for compound pages is that we
need to stuff each relevant subpage in and convert the offset into offset
in the first subpage.
Hopefully-fixes: f0f6b614f83d "copy_page_to_iter(): don't split high-order page in case of ITER_PIPE"
Signed-off-by: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
---
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 9f486b788ed0..b16aed158ba6 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -846,10 +846,14 @@ nfsd_splice_actor(struct pipe_inode_info *pipe, struct pipe_buffer *buf,
struct splice_desc *sd)
{
struct svc_rqst *rqstp = sd->u.data;
-
- svc_rqst_replace_page(rqstp, buf->page);
- if (rqstp->rq_res.page_len == 0)
- rqstp->rq_res.page_base = buf->offset;
+ struct page *page = buf->page; // may be a compound one
+ unsigned offset = buf->offset;
+
+ page += offset / PAGE_SIZE;
+ for (int i = sd->len; i > 0; i -= PAGE_SIZE)
+ svc_rqst_replace_page(rqstp, page++);
+ if (rqstp->rq_res.page_len == 0) // first call
+ rqstp->rq_res.page_base = offset % PAGE_SIZE;
rqstp->rq_res.page_len += sd->len;
return sd->len;
}
