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
