On Fri, 2023-03-17 at 20:55 +0000, Chuck Lever III wrote:

quoted

On Mar 17, 2023, at 2:59 PM, Jeff Layton [off-list ref] wrote:

On Fri, 2023-03-17 at 18:08 +0000, Chuck Lever III wrote:

quoted

quoted

On Mar 17, 2023, at 2:04 PM, Jeff Layton [off-list ref] wrote:

On Fri, 2023-03-17 at 17:51 +0000, Chuck Lever III wrote:

quoted

quoted

On Mar 17, 2023, at 1:13 PM, Jeff Layton [off-list ref] wrote:

If rq_next_page ends up pointing outside the array, then we can
corrupt
memory when we go to change its value. Ensure that it hasn't strayed
outside the array, and have svc_rqst_replace_page return -EIO
without
changing anything if it has.

Fix up nfsd_splice_actor (the only caller) to handle this case by
either
returning an error or a short splice when this happens.

IMO it's not worth the extra complexity to return a short splice.
This is a "should never happen" scenario in a hot I/O path. Let's
keep this code as simple as possible, and use unlikely() for the
error cases in both nfsd_splice_actor and svc_rqst_replace_page().

Are there any issues with just returning an error even though we have
successfully spliced some of the data into the buffer? I guess the
caller will just see an EIO or whatever instead of the short read in
that case?

NFSv4 READ is probably going to truncate the XDR buffer. I'm not
sure NFSv3 is so clever, so you should test it.

Honestly, I don't have the cycles to do that sort of fault injection
testing for this.

nfsd_splice_actor() has never returned an error, so IMO it is
necessary to confirm that when svc_rqst_replace_page() returns
an error, it doesn't create further problems. I don't see how
we can avoid some kind of simple fault injection while developing
the fix.

Tell you what, I can take it from here if you'd like.

Sure! All yours!

quoted

If you think handling it as a short read is overblown,
then tell me what you would like see here.

It's not the short reads that bugs me, it's the additional
code in a hot path that is worrisome.

I wouldn't think tracking some extra stuff on the stack would show up
much in profiles, but it's your call.

-- 
Jeff Layton [off-list ref]