On Fri, Nov 2, 2018 at 12:49 PM J. Bruce Fields <bfields@xxxxxxxxxxxx> wrote:
>
> 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.
Using a new bit is good enough for me.
>
> --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
