Hello NeilBrown,
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:
fs/nfsd/nfs4state.c:1674 release_openowner()
warn: sleeping in atomic context
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 = oo->oo_owner.so_client;
1661 struct list_head reaplist;
1662
1663 INIT_LIST_HEAD(&reaplist);
1664
1665 spin_lock(&clp->cl_lock);
1666 unhash_openowner_locked(oo);
1667 while (!list_empty(&oo->oo_owner.so_stateids)) {
1668 stp = list_first_entry(&oo->oo_owner.so_stateids,
1669 struct nfs4_ol_stateid, st_perstateowner);
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.
1675 release_last_closed_stateid(oo);
1676 nfs4_put_stateowner(&oo->oo_owner);
1677 }
The caller is find_or_alloc_open_stateowner()
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 = cstate->clp;
4867 struct nfs4_openowner *oo, *new = NULL;
4868
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?
4871 oo = find_openstateowner_str(strhashval, open, clp);
4872 if (oo && !(oo->oo_flags & NFS4_OO_CONFIRMED)) {
4873 /* Replace unconfirmed owners without checking for replay. */
4874 release_openowner(oo);
^^^^^^^^^^^^^^^^^^^^^^
Here
4875 oo = 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);
regards,
dan carpenter
On Fri, 08 Mar 2024, Dan Carpenter wrote:
> Hello NeilBrown,
>
> 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:
>
> fs/nfsd/nfs4state.c:1674 release_openowner()
> warn: sleeping in atomic context
>
> 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 = oo->oo_owner.so_client;
> 1661 struct list_head reaplist;
> 1662
> 1663 INIT_LIST_HEAD(&reaplist);
> 1664
> 1665 spin_lock(&clp->cl_lock);
> 1666 unhash_openowner_locked(oo);
> 1667 while (!list_empty(&oo->oo_owner.so_stateids)) {
> 1668 stp = list_first_entry(&oo->oo_owner.so_stateids,
> 1669 struct nfs4_ol_stateid, st_perstateowner);
> 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!
>
> 1675 release_last_closed_stateid(oo);
> 1676 nfs4_put_stateowner(&oo->oo_owner);
> 1677 }
>
> The caller is find_or_alloc_open_stateowner()
>
> 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 = cstate->clp;
> 4867 struct nfs4_openowner *oo, *new = NULL;
> 4868
> 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
>
>
> 4871 oo = find_openstateowner_str(strhashval, open, clp);
> 4872 if (oo && !(oo->oo_flags & NFS4_OO_CONFIRMED)) {
> 4873 /* Replace unconfirmed owners without checking for replay. */
> 4874 release_openowner(oo);
> ^^^^^^^^^^^^^^^^^^^^^^
> Here
>
> 4875 oo = 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);
>
>
>
> regards,
> dan carpenter
>
>