On Fri, Nov 02, 2018 at 12:35:26PM -0400, Olga Kornievskaia wrote: > On Fri, Nov 2, 2018 at 11:46 AM J. Bruce Fields <bfields@xxxxxxxxxxxx> wrote: > > > > On Fri, Oct 19, 2018 at 11:28:59AM -0400, Olga Kornievskaia wrote: > > > @@ -4273,6 +4337,9 @@ static __be32 nfsd4_encode_readv(struct nfsd4_compoundres *resp, > > > p = xdr_reserve_space(&resp->xdr, 4 + 4); > > > *p++ = xdr_one; /* cr_consecutive */ > > > *p++ = cpu_to_be32(copy->cp_synchronous); > > > + > > > + /* allocated in nfsd4_decode_copy */ > > > + kfree(copy->cp_src); > > > > This can result in a leak--for example, if we decode the compound > > succesfully, but processing fails before we could to this op, then we'll > > never call this encoder, so we'll allocate without freeing. > > > > I think simplest would be to replace this: > > > > > diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h > > > index feeb6d4..b4d1140 100644 > > > --- a/fs/nfsd/xdr4.h > > > +++ b/fs/nfsd/xdr4.h > > > @@ -521,6 +521,7 @@ struct nfsd4_copy { > > > u64 cp_src_pos; > > > u64 cp_dst_pos; > > > u64 cp_count; > > > + struct nl4_server *cp_src; > > > > by just a > > > > struct nl4_server cp_src; > > > > since it sounds like you really only need one of them, not a whole array > > (at least for now). > > So this is problematic as the presence of this memory is what is used > to distinguish "inter" from "intra". It would be easy enough to add a new bit for that. > Can things really fail between the xdr and calling of the operation? Yes. Consider a PUTFH+SAVEFH+COPY compound. We decode the whole thing before starting any processing. Then we call nfsd4_putfh() and fh_verify fails (maybe the filehandle is stale or something). Then neither nfsd4_copy() nor nfsd4_encode_copy() will be called. If you absolutely have to do this, you can look at SAVEMEM. --b. > What gets freed in the encoder is the "copy" of the what was decoded > in the decoder. But really freeing in the encoder is the wrong place. > Encoder doesn't need to free. I already free the "copy" of the > copy->cp_src in the cleanup_async_copy(). However, what is missing is > freeing the original copy->cp_src which needs to be freed in the > dup_copy_fields(). > > To clarify: > copy->cp_src gets allocated in the decoder > during the process of the copy: > 1. it gets copied to the kthread and the original copy->cp_src needs > to be freed. Or during any error it will be freed. > 2. cleanup_async_copy frees the copy of the copy->cp_src. > (need to remove the kfree from the encoder). > > > > > --b. > > > > > > > > /* both */ > > > bool cp_synchronous; > > > -- > > > 1.8.3.1