On 2016-07-27 09:26, J. Bruce Fields wrote: > On Wed, Jul 27, 2016 at 08:57:26AM +0900, Seiichi Ikarashi wrote: >> Hi Bruce, >> >> On 2016-07-27 05:19, J. Bruce Fields wrote: >>> Thanks for the report. >>> >>> On Tue, Jul 26, 2016 at 11:38:11AM +0900, Seiichi Ikarashi wrote: >>>> If over-"RPCSVC_MAXPAGES" pages are sent from file system through pipe_buffer, >>>> nfsd_splice_actor() corrupts struct svc_rqst and results in kernel panic. It >>>> actually occurred with a parallel distributed file system. It needs boundary >>>> checking. >>> >>> This check might be useful as defensive programming, but the bug was >>> elsewhere. >> >> Yah, I think the main factor exists in file system and/or VFS splice sides. >> But I also think that NFS should defend itself against overlimit pages >> because the limit is decided by NFS/SUNRPC, not by file system and others. >> >>> >>> In theory this should be prevented by the "maxcount" calculations in >>> nfsd4_encode_read(). >> >> The "maxcount" looks just limiting the read length from the file system. >> Is my understanding correct? > > Right. > >> >> And the number of pages provided from the file system is up to the file system. >> The file system can split the read data into an arbitrary number of pages. > > Oh, so if we ask the filesystem for 3 bytes it might potentially return > those in 3 separate pages? Is that at all legal? I think the code actually permits such a split though I am not sure that one-single-byte-per-page situation really happens. :-) For example, on 4kB-page architecture, 1kB-data-per-page placement will result in 4 times larger number of pages than expected. I do not know whether it's legal or not. I thought NFS needs to defend itself against such large numbers, or, can somewhere in VFS splice merge such pages into a minimum set of pages? I cannot find such code in VFS. Opinions or suggestions? Seiichi. > >>> What version of the kernel did you see this happen on? What was the >>> client, and what was it doing? Any other hints on reproducing? >> >> It was a NFSv3 read access from a client of maybe-Linux-based system >> against the server of RHEL6 system exporting a file system that I cannot >> access its source code. Sorry for lack of info. > > OK, understood. > > --b. > >> >> >> Seiichi. >> >> >>> >>> --b. >>> >>>> >>>> v2: Fix semicolon-missing bug. >>>> >>>> Signed-off-by: Seiichi Ikarashi <s.ikarashi@xxxxxxxxxxxxxx> >>>> >>>> --- >>>> fs/nfsd/vfs.c | 8 ++++++++ >>>> 1 file changed, 8 insertions(+) >>>> >>>> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c >>>> index 6fbd81e..43393f3 100644 >>>> --- a/fs/nfsd/vfs.c >>>> +++ b/fs/nfsd/vfs.c >>>> @@ -811,12 +811,20 @@ nfsd_splice_actor(struct pipe_inode_info *pipe, struct pipe_buffer *buf, >>>> size = sd->len; >>>> >>>> if (rqstp->rq_res.page_len == 0) { >>>> + if (rqstp->rq_next_page > &rqstp->rq_pages[RPCSVC_MAXPAGES-1]) { >>>> + WARN_ON(1); >>>> + return -ENOMEM; >>>> + } >>>> get_page(page); >>>> put_page(*rqstp->rq_next_page); >>>> *(rqstp->rq_next_page++) = page; >>>> rqstp->rq_res.page_base = buf->offset; >>>> rqstp->rq_res.page_len = size; >>>> } else if (page != pp[-1]) { >>>> + if (rqstp->rq_next_page > &rqstp->rq_pages[RPCSVC_MAXPAGES-1]) { >>>> + WARN_ON(1); >>>> + return -ENOMEM; >>>> + } >>>> get_page(page); >>>> if (*rqstp->rq_next_page) >>>> put_page(*rqstp->rq_next_page); >>>> -- >>>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in >>>> the body of a message to majordomo@xxxxxxxxxxxxxxx >>>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>> . >>> > . > -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html