On Tue, Apr 18 2017, J. Bruce Fields wrote: > On Wed, Apr 19, 2017 at 10:17:09AM +1000, NeilBrown wrote: >> On Tue, Apr 18 2017, 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. >> >> Not a bad idea. That is what nfsaclsvc_encode_getaclres() and >> nfs3svc_encode_getaclres do. >> Hmm... your change to xdr_argsize_check will break >> nfsaclsvc_decode_setaclargs(), won't it? It performs the check before >> the final nfsacl_decode(). > > Ugh, I forget that I don't run any tests for NFSv3 ACLs. Well, that > would be easy enough to fix.... > >> >> I haven't tested this at all and haven't even convinced myself that >> >> it covers every case (though I cannot immediately think of any likely >> >> corners). >> >> >> >> Does it address your test case? >> > >> > I'll check, it probably does. >> > >> > We'd need to limit the test to v2/v3. >> >> Why? Does v4 allocate extra pages? Or is it more careful about using >> them? >> v4 does need something different, as pc_xdrressize is always zero.. > > NFSv4 compounds just don't have that limitation. You can read and write > in the same compound if you want. (Why you'd want to, I've no idea.) I realise NFSv4 compounds don't have that limitation. I wondered what code in the NFSv4 server ensures that we don't try to use more memory than was allocated. I notice lots of calls to xdr_reserve_space() in nfs4xdr.c. Many of them trigger nfserr_resource when xdr_reserve_space() returns NULL. But not all. nfsd4_encode_readv() just pops up a warning. Once. Then will (eventually) de-reference the NULL pointer and crash. So presumably it really cannot happen (should be a BUG_ON anyway)? So why can this not happen? I see that nfsd4_encode_read() limits the size of the read to xdr->buf->buflen - xdr->buf->len and nfsd4_encode_readdir() does a similar thing when computing bytes_left. So, it is more careful about using the allocated pages than v2/3 is. Thanks, NeilBrown > > (In fact, I think at least in the version >=4.1 case we should probably > only be placing limits on argument and reply sizes individually, so our > current implementation (which also places limits on the sum of the two) > is probably wrong. This doesn't keep me up at night.) > > --b.
Attachment:
signature.asc
Description: PGP signature
