On Thu, Apr 20 2017, J. Bruce Fields wrote: > On Thu, Apr 20, 2017 at 12:19:35PM -0400, J. Bruce Fields wrote: >> On Tue, Apr 18, 2017 at 01:13:51PM -0400, J. Bruce Fields wrote: >> > On Tue, Apr 18, 2017 at 10:25:20AM +1000, NeilBrown wrote: >> > > I can't say that I like this patch at all. >> > > >> > > The problem is that: >> > > >> > > pages = size / PAGE_SIZE + 1; /* extra page as we hold both request and reply. >> > > * We assume one is at most one page >> > > */ >> > > >> > > this assumption is never verified. >> > > To my mind, the "obvious" way to verify this assumption is that an >> > > attempt to generate a multi-page reply should fail if there was a >> > > multi-page request. >> > >> > A third option, by the way, which Ari Kauppi argued for, is adding a >> > null check each time we increment rq_next_page, since we seem to arrange >> > for the page array to always be NULL-terminated. >> > >> > > Failing if there was a little bit of extra noise at the end of the >> > > request seems harsher than necessary, and could result in a regression. >> > >> > You're worrying there might be a weird old client out there somewhere? >> > I guess it seems like a small enough risk to me. I'm more worried the >> > extra garbage might violate assumptions elsewhere in the code. >> > >> > But, this looks good too: >> >> But, I'm not too happy about putting that NFSv2/v3-specific check in >> common rpc code. Also, I think this check comes too late for some of >> the damage. Too late? It is earlier than anything else. >> >> I may go with some variation on Ari's idea, let me give it a try.... > > In the read case, I think Ari's approach wouldn't catch the error until > nfsd_direct_splice_actor(), which doesn't actually look capable of > handling errors. Maybe that should be fixed. Or maybe read just needs > some more checks. Ugh. By the time you get to nfsd_read(), the 'struct kvec' should be set up and valid. So we need checks is e.g. nfs3svc_decode_readargs(), but not deeper. NeilBrown
Attachment:
signature.asc
Description: PGP signature
