Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <0fbcbdc37e7e3f070b491848a74be348843074c2.camel@kernel.org>
Subject: Re: [PATCH 2/2] nfsd: clean up potential nfsd_file refcount leaks
 in COPY codepath
From:   Jeff Layton <jlayton@kernel.org>
To:     Chuck Lever III <chuck.lever@oracle.com>,
        Olga Kornievskaia <aglo@umich.edu>
Cc:     Linux NFS Mailing List <linux-nfs@vger.kernel.org>,
        Dai Ngo <dai.ngo@oracle.com>
Date:   Wed, 18 Jan 2023 12:06:24 -0500
In-Reply-To: <12C5F3B3-6DB1-4483-8160-A691EB464464@oracle.com>
References: <20230117193831.75201-1-jlayton@kernel.org>
         <20230117193831.75201-3-jlayton@kernel.org>
         <CAN-5tyHA6JgqnEorEqz1b3CLdbXWhT6hNZKXzgfZy3Fr_TdW7Q@mail.gmail.com>
         <1fc9af5a2c2a79c5befa4510c714f97e26b13ed5.camel@kernel.org>
         <CAN-5tyHKS9o3KDV3zUmzjiOjSxyC1rNe77Tc8c7RHmoXE6s_RQ@mail.gmail.com>
         <12C5F3B3-6DB1-4483-8160-A691EB464464@oracle.com>
Content-Type: text/plain; charset="ISO-8859-15"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.46.3 (3.46.3-1.fc37) 
MIME-Version: 1.0
Precedence: bulk

On Wed, 2023-01-18 at 16:39 +0000, Chuck Lever III wrote:
>=20
> > On Jan 18, 2023, at 11:29 AM, Olga Kornievskaia <aglo@umich.edu> wrote:
> >=20
> > On Wed, Jan 18, 2023 at 10:27 AM Jeff Layton <jlayton@kernel.org> wrote=
:
> > >=20
> > > On Wed, 2023-01-18 at 09:42 -0500, Olga Kornievskaia wrote:
> > > > On Tue, Jan 17, 2023 at 2:38 PM Jeff Layton <jlayton@kernel.org> wr=
ote:
> > > > >=20
> > > > > There are two different flavors of the nfsd4_copy struct. One is
> > > > > embedded in the compound and is used directly in synchronous copi=
es. The
> > > > > other is dynamically allocated, refcounted and tracked in the cli=
ent
> > > > > struture. For the embedded one, the cleanup just involves releasi=
ng any
> > > > > nfsd_files held on its behalf. For the async one, the cleanup is =
a bit
> > > > > more involved, and we need to dequeue it from lists, unhash it, e=
tc.
> > > > >=20
> > > > > There is at least one potential refcount leak in this code now. I=
f the
> > > > > kthread_create call fails, then both the src and dst nfsd_files i=
n the
> > > > > original nfsd4_copy object are leaked.
> > > >=20
> > > > I don't believe that's true. If kthread_create thread fails we call
> > > > cleanup_async_copy() that does a put on the file descriptors.
> > > >=20
> > >=20
> > > You mean this?
> > >=20
> > > out_err:
> > >        if (async_copy)
> > >                cleanup_async_copy(async_copy);
> > >=20
> > > That puts the references that were taken in dup_copy_fields, but the
> > > original (embedded) nfsd4_copy also holds references and those are no=
t
> > > being put in this codepath.
> >=20
> > Can you please point out where do we take a reference on the original c=
opy?
> >=20
> > > > > The cleanup in this codepath is also sort of weird. In the async =
copy
> > > > > case, we'll have up to four nfsd_file references (src and dst for=
 both
> > > > > flavors of copy structure).
> > > >=20
> > > > That's not true. There is a careful distinction between intra -- wh=
ich
> > > > had 2 valid file pointers and does a get on both as they both point=
 to
> > > > something that's opened on this server--- but inter -- only does a =
get
> > > > on the dst file descriptor, the src doesn't exit. And yes I realize
> > > > the code checks for nfs_src being null which it should be but it ma=
kes
> > > > the code less clear and at some point somebody might want to decide=
 to
> > > > really do a put on it.
> > > >=20
> > >=20
> > > This is part of the problem here. We have a nfsd4_copy structure, and
> > > depending on what has been done to it, you need to call different
> > > methods to clean it up. That seems like a real antipattern to me.
> >=20
> > But they call different methods because different things need to be
> > done there and it makes it clear what needs to be for what type of
> > copy.
>=20
> In cases like this, it makes sense to consider using types to
> ensure the code can't do the wrong thing. So you might want to
> have a struct nfs4_copy_A for the inter code to use, and a struct
> nfs4_copy_B for the intra code to use. Sharing the same struct
> for both use cases is probably what's confusing to human readers.
>=20
> I've never been a stickler for removing every last ounce of code
> duplication. Here, it might help to have a little duplication
> just to make it easier to reason about the reference counting in
> the two use cases.
>=20
> That's my view from the mountain top, worth every penny you paid
> for it.
>=20

+1

The nfsd4_copy structure has a lot of fields in it that only matter for
the async copy case. ISTM that nfsd4_copy (the function) should
dynamically allocate a struct nfsd4_async_copy that contains a
nfsd4_copy and whatever other fields are needed.

Then, we could trim down struct nfsd4_copy to just the info needed.

For instance, the nf_src and nf_dst fields really don't need to be in
nfsd4_copy. For the synchronous copy case, we can just keep those
pointers on the stack, and for the async case they would be inside the
larger structure.

That would allow us to trim down the footprint of the compound union
too.

--=20
Jeff Layton <jlayton@kernel.org>