DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 0CBEC2081B
MIME-Version: 1.0
References: <20181019152905.32418-1-olga.kornievskaia@gmail.com>
 <20181019152905.32418-8-olga.kornievskaia@gmail.com> <20181102154623.GA20367@fieldses.org>
 <CAN-5tyGkz=hHJzQRwBZQgAuGTWq9KLdDreoBoyvWVYQPRLTM0w@mail.gmail.com> <20181102164924.GB20367@fieldses.org>
In-Reply-To: <20181102164924.GB20367@fieldses.org>
From:   Olga Kornievskaia <olga.kornievskaia@gmail.com>
Date:   Fri, 2 Nov 2018 13:04:54 -0400
Message-ID: <CAN-5tyELP7O-F4CTsQMzia0pL4iQpACGyMBqpEn3K7wFBzE17A@mail.gmail.com>
Subject: Re: [PATCH v1 07/13] NFSD add ca_source_server<> to COPY
To:     "J. Bruce Fields" <bfields@fieldses.org>
Cc:     "J. Bruce Fields" <bfields@redhat.com>,
        linux-nfs <linux-nfs@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-nfs-owner@vger.kernel.org
Precedence: bulk

On Fri, Nov 2, 2018 at 12:49 PM J. Bruce Fields <bfields@fieldses.org> 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@fieldses.org> 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