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".
Can things really fail between the xdr and calling of the operation?
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
