Received-SPF: pass (google.com: domain of linux-nfs+bounces-2249-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk
MIME-Version: 1.0
From: "NeilBrown" <neilb@suse.de>
To: "Dan Carpenter" <dan.carpenter@linaro.org>
Cc: linux-nfs@vger.kernel.org
Subject: Re: [bug report] nfsd: perform all find_openstateowner_str calls in
 the one place.
In-reply-to: <44e5a5f5-f8df-45bd-be1f-7d67cfb011de@moroto.mountain>
References: <44e5a5f5-f8df-45bd-be1f-7d67cfb011de@moroto.mountain>
Date: Mon, 11 Mar 2024 09:11:13 +1100
Message-id: <171010867304.13576.11261693671416322907@noble.neil.brown.name>

On Fri, 08 Mar 2024, Dan Carpenter wrote:
> Hello NeilBrown,
>=20
> Commit 11c3d0a15bbc ("nfsd: perform all find_openstateowner_str calls
> in the one place.") from Mar 5, 2024 (linux-next), leads to the
> following Smatch static checker warning:
>=20
> 	fs/nfsd/nfs4state.c:1674 release_openowner()
> 	warn: sleeping in atomic context
>=20
> fs/nfsd/nfs4state.c
>     1657 static void release_openowner(struct nfs4_openowner *oo)
>     1658 {
>     1659         struct nfs4_ol_stateid *stp;
>     1660         struct nfs4_client *clp =3D oo->oo_owner.so_client;
>     1661         struct list_head reaplist;
>     1662=20
>     1663         INIT_LIST_HEAD(&reaplist);
>     1664=20
>     1665         spin_lock(&clp->cl_lock);
>     1666         unhash_openowner_locked(oo);
>     1667         while (!list_empty(&oo->oo_owner.so_stateids)) {
>     1668                 stp =3D list_first_entry(&oo->oo_owner.so_stateids,
>     1669                                 struct nfs4_ol_stateid, st_perstat=
eowner);
>     1670                 if (unhash_open_stateid(stp, &reaplist))
>     1671                         put_ol_stateid_locked(stp, &reaplist);
>     1672         }
>     1673         spin_unlock(&clp->cl_lock);
> --> 1674         free_ol_stateid_reaplist(&reaplist);
>                  ^^^^^^^^^^^^^^^^^^^^^^^^
> This is a might sleep function.

Thanks Dan - very helpful as always!


>=20
>     1675         release_last_closed_stateid(oo);
>     1676         nfs4_put_stateowner(&oo->oo_owner);
>     1677 }
>=20
> The caller is find_or_alloc_open_stateowner()
>=20
> fs/nfsd/nfs4state.c
>   4863  find_or_alloc_open_stateowner(unsigned int strhashval, struct nfsd4=
_open *open,
>   4864                                struct nfsd4_compound_state *cstate)
>   4865  {
>   4866          struct nfs4_client *clp =3D cstate->clp;
>   4867          struct nfs4_openowner *oo, *new =3D NULL;
>   4868 =20
>   4869          while (1) {
>   4870                  spin_lock(&clp->cl_lock);
>                        ^^^^^^^^^^^^^^^^^^^^^^^^^
> Huh...  This looks like the same lock that we take in
> release_openowner().  Why do I not see a static checker warning for
> double lock?

Good question.  Maybe the relationship between oo and clp is too subtle
for the checker to follow?
It would certainly have deadlocked if ever NFS4_OO_CONFIRMED was not
set.

Fortunately this is easily fixed.

Thanks again,
NeilBrown


>=20
>=20
>   4871                  oo =3D find_openstateowner_str(strhashval, open, cl=
p);
>   4872                  if (oo && !(oo->oo_flags & NFS4_OO_CONFIRMED)) {
>   4873                          /* Replace unconfirmed owners without check=
ing for replay. */
>   4874                          release_openowner(oo);
>                                 ^^^^^^^^^^^^^^^^^^^^^^
> Here
>=20
>   4875                          oo =3D NULL;
>   4876                  }
>   4877                  if (oo) {
>   4878                          spin_unlock(&clp->cl_lock);
>   4879                          if (new)
>   4880                                  nfs4_free_stateowner(&new->oo_owner=
);
>   4881                          return oo;
>   4882                  }
>   4883                  if (new) {
>   4884                          hash_openowner(new, clp, strhashval);
>   4885                          spin_unlock(&clp->cl_lock);
>   4886                          return new;
>   4887                  }
>   4888                  spin_unlock(&clp->cl_lock);
>=20
>=20
>=20
> regards,
> dan carpenter
>=20
>=20