| Message ID | 20230317105608.19393-2-jlayton@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] nfsd: don't replace page in rq_pages if it's a continuation of last page | expand |
> On Mar 17, 2023, at 6:56 AM, Jeff Layton <jlayton@kernel.org> wrote: > > There's no good way to handle this gracefully, but if rq_next_page ends > up pointing outside the array, we can at least crash the box before it > scribbles over too much else. > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > --- > net/sunrpc/svc.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c > index fea7ce8fba14..864e62945647 100644 > --- a/net/sunrpc/svc.c > +++ b/net/sunrpc/svc.c > @@ -845,6 +845,16 @@ EXPORT_SYMBOL_GPL(svc_set_num_threads); > */ > void svc_rqst_replace_page(struct svc_rqst *rqstp, struct page *page) > { > + struct page **begin, **end; > + > + /* > + * Bounds check: make sure rq_next_page points into the rq_respages > + * part of the array. > + */ > + begin = rqstp->rq_pages; > + end = &rqstp->rq_pages[RPCSVC_MAXPAGES]; > + BUG_ON(rqstp->rq_next_page < begin || rqstp->rq_next_page > end); Linus has stated clearly that he does not want BUG_ON assertions if the system is not actually in danger... and this is clearly the result of a software bug, so a crash will occur anyway. Can you make this a pr_warn_once() ? > + > if (*rqstp->rq_next_page) { > if (!pagevec_space(&rqstp->rq_pvec)) > __pagevec_release(&rqstp->rq_pvec); > -- > 2.39.2 > -- Chuck Lever
On Fri, 2023-03-17 at 13:44 +0000, Chuck Lever III wrote: > > > On Mar 17, 2023, at 6:56 AM, Jeff Layton <jlayton@kernel.org> wrote: > > > > There's no good way to handle this gracefully, but if rq_next_page ends > > up pointing outside the array, we can at least crash the box before it > > scribbles over too much else. > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > --- > > net/sunrpc/svc.c | 10 ++++++++++ > > 1 file changed, 10 insertions(+) > > > > diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c > > index fea7ce8fba14..864e62945647 100644 > > --- a/net/sunrpc/svc.c > > +++ b/net/sunrpc/svc.c > > @@ -845,6 +845,16 @@ EXPORT_SYMBOL_GPL(svc_set_num_threads); > > */ > > void svc_rqst_replace_page(struct svc_rqst *rqstp, struct page *page) > > { > > + struct page **begin, **end; > > + > > + /* > > + * Bounds check: make sure rq_next_page points into the rq_respages > > + * part of the array. > > + */ > > + begin = rqstp->rq_pages; > > + end = &rqstp->rq_pages[RPCSVC_MAXPAGES]; > > + BUG_ON(rqstp->rq_next_page < begin || rqstp->rq_next_page > end); > > Linus has stated clearly that he does not want BUG_ON assertions > if the system is not actually in danger... and this is clearly > the result of a software bug, so a crash will occur anyway. > It'll crash, but only after we scribble over some memory. Actually, it looks like the splice actor can return an error. We could return -EIO here or something without doing anything if we hit this case and then let that bubble back up to the read? > Can you make this a pr_warn_once() ? > > > > + > > if (*rqstp->rq_next_page) { > > if (!pagevec_space(&rqstp->rq_pvec)) > > __pagevec_release(&rqstp->rq_pvec); > > -- > > 2.39.2 > > > > -- > Chuck Lever > >
> On Mar 17, 2023, at 9:52 AM, Jeff Layton <jlayton@kernel.org> wrote: > > On Fri, 2023-03-17 at 13:44 +0000, Chuck Lever III wrote: >> >>> On Mar 17, 2023, at 6:56 AM, Jeff Layton <jlayton@kernel.org> wrote: >>> >>> There's no good way to handle this gracefully, but if rq_next_page ends >>> up pointing outside the array, we can at least crash the box before it >>> scribbles over too much else. >>> >>> Signed-off-by: Jeff Layton <jlayton@kernel.org> >>> --- >>> net/sunrpc/svc.c | 10 ++++++++++ >>> 1 file changed, 10 insertions(+) >>> >>> diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c >>> index fea7ce8fba14..864e62945647 100644 >>> --- a/net/sunrpc/svc.c >>> +++ b/net/sunrpc/svc.c >>> @@ -845,6 +845,16 @@ EXPORT_SYMBOL_GPL(svc_set_num_threads); >>> */ >>> void svc_rqst_replace_page(struct svc_rqst *rqstp, struct page *page) >>> { >>> + struct page **begin, **end; >>> + >>> + /* >>> + * Bounds check: make sure rq_next_page points into the rq_respages >>> + * part of the array. >>> + */ >>> + begin = rqstp->rq_pages; >>> + end = &rqstp->rq_pages[RPCSVC_MAXPAGES]; >>> + BUG_ON(rqstp->rq_next_page < begin || rqstp->rq_next_page > end); >> >> Linus has stated clearly that he does not want BUG_ON assertions >> if the system is not actually in danger... and this is clearly >> the result of a software bug, so a crash will occur anyway. >> > > It'll crash, but only after we scribble over some memory. > > Actually, it looks like the splice actor can return an error. We could > return -EIO here or something without doing anything if we hit this case > and then let that bubble back up to the read? Yes, if it's possible to fail just the READ operation, that would be best. Maybe a emitting a trace event would be better than a pr_warn. >> Can you make this a pr_warn_once() ? >> >> >>> + >>> if (*rqstp->rq_next_page) { >>> if (!pagevec_space(&rqstp->rq_pvec)) >>> __pagevec_release(&rqstp->rq_pvec); >>> -- >>> 2.39.2 >>> >> >> -- >> Chuck Lever >> >> > > -- > Jeff Layton <jlayton@kernel.org> -- Chuck Lever
diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c index fea7ce8fba14..864e62945647 100644 --- a/net/sunrpc/svc.c +++ b/net/sunrpc/svc.c @@ -845,6 +845,16 @@ EXPORT_SYMBOL_GPL(svc_set_num_threads); */ void svc_rqst_replace_page(struct svc_rqst *rqstp, struct page *page) { + struct page **begin, **end; + + /* + * Bounds check: make sure rq_next_page points into the rq_respages + * part of the array. + */ + begin = rqstp->rq_pages; + end = &rqstp->rq_pages[RPCSVC_MAXPAGES]; + BUG_ON(rqstp->rq_next_page < begin || rqstp->rq_next_page > end); + if (*rqstp->rq_next_page) { if (!pagevec_space(&rqstp->rq_pvec)) __pagevec_release(&rqstp->rq_pvec);
There's no good way to handle this gracefully, but if rq_next_page ends up pointing outside the array, we can at least crash the box before it scribbles over too much else. Signed-off-by: Jeff Layton <jlayton@kernel.org> --- net/sunrpc/svc.c | 10 ++++++++++ 1 file changed, 10 insertions(+)