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;
From:   Chuck Lever III <chuck.lever@oracle.com>
To:     Neil Brown <neilb@suse.de>
CC:     Jeff Layton <jlayton@kernel.org>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Subject: Re: nfsd: another possible delegation race
Thread-Topic: nfsd: another possible delegation race
Thread-Index: AQHY17BC08CkcsMynkeV/ZMR9ZjUjq3+R1KA
Date:   Tue, 4 Oct 2022 14:12:36 +0000
Message-ID: <A20D72B2-097F-4240-A894-0759B53C02F4@oracle.com>
References: <166486048770.14457.133971372966856907@noble.neil.brown.name>
In-Reply-To: <166486048770.14457.133971372966856907@noble.neil.brown.name>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?z1TZwnpyoPzn5mhVYEo7w7EiGzNMG+mz0541oyCs+u/AWeX3p68ilQnrK3UT?=
 =?us-ascii?Q?SI2W8ApysuAXGH4rZxKB6i0GMNxsDAkKwvNbfQEChMYmwAUfMh45845hEdqs?=
 =?us-ascii?Q?76twSf+nV7CzJS8L+0I1TtruM0ZhSbD2CZX/ds6nxdzmXO2BYhJm7cOb2zZ2?=
 =?us-ascii?Q?mr7mlp8u4Ed+WPpD5VdubwzkzvFJ30DxOWwN3R5Eo2ar4Z/PnZGC/l200zza?=
 =?us-ascii?Q?IjgiPpDo1RzqqR8+B1bm770TIbrrAnt2uyPSkir0Y9FqQUWX5c/hAN0vc68U?=
 =?us-ascii?Q?Z92wIWp8lY4sEcpp6RI6FfTQaCnhooCBk4QsCQlIVzsRxMsKsHsgsAvzwVvc?=
 =?us-ascii?Q?9tNg+H3Edn1hjAfE0Ry9E66+mZC6lJbQmUyYxd6RmMXpzvEhlh3W50WoNVML?=
 =?us-ascii?Q?cr/qCJMmVtyHRDsImaEuyUHunsOdwt0dfMBnQRHRF27BWxF2cxT0A5yybOYh?=
 =?us-ascii?Q?Lol1+fHbWeG+eimC0dw0xVZMOmxF5qE8hp8hrXOSl9Ms+n18K+3Ksi4U9KF4?=
 =?us-ascii?Q?w2z+9gi9iesDtLUKNf4j9E71GOOwCNr0GVIHC6a1dDe2hPNCa2ovxtbyYDNK?=
 =?us-ascii?Q?zaqrLZu8qRiJ9XHkUH2dPZlNpEe9Xcunn/wqv6pya+s8cHLU/gyquvpSmGbl?=
 =?us-ascii?Q?HhV1Bv8nuLKYmh+NIrO2GSkXbXxF1Z2PEbQWdffxUceI1zY+qsFZfF+oHT7d?=
 =?us-ascii?Q?95BWTgB8dozVpolmDv9dvtVbJw/geANEF6KTHvOUqQ9G3jIyd/18GMPa/AOq?=
 =?us-ascii?Q?CCIBcIq/3982GBQrDXyCU3xznqxAZfdvZZJLiQuBiZSpS3yW8t/U7K8h13Tf?=
 =?us-ascii?Q?F9tP2qm67aGLqAjz4u7WyA88Mg/hUM1E1M3hxz4EHj+Hd2QJokKanamN4tEA?=
 =?us-ascii?Q?3926Ui7Kvw++OFNq25hndpF7CvIfasm/za+ySBunRw9hr+JFB4Szv6XjHanI?=
 =?us-ascii?Q?zqtTjZSko1sPk7fYKr3w2xxyvhZiDg/u8apqBchrBZyTpblTeeyHs0D+KVo+?=
 =?us-ascii?Q?xc//urcuNRXX5wRRQbEKSwKOicbVHtB7U5kA3BONi4pl+Z2vPkWSdW7XVXrB?=
 =?us-ascii?Q?6kDNcTWdLmAQRkWUvdGlhGqCLx/H0veUP6ZvTA+YZhERdKfk+MVeoiEHYiXF?=
 =?us-ascii?Q?w9FYMYL7dMBHSs9b/nVnJz2UV9Qk3AsCIEg6kqgI+iH2NnLYm8Pah7QXiCGe?=
 =?us-ascii?Q?/NdelylbPOj0uzfnsbdaBV8swD8j7FwqOey0O/J2p0QfkFEFgPXy0DmPulnM?=
 =?us-ascii?Q?fm9d+TAo/J3tCafmKKqDPMfOnd7mwxYY6NCDoePmAarO6xlIKODIaoiH+1ie?=
 =?us-ascii?Q?eO1Txiyl41f2RoWG9tDVWTVj1t67bc7dVJu0qAY6Uj84aPrItklyvUoQ/8Dr?=
 =?us-ascii?Q?6UjQv2skPTuqwtAYbGD+eMQ5vY4uba0iCaYm87Nu47YReVwq3YQGOJ4ovPFw?=
 =?us-ascii?Q?ybzOerMg/EsgWqM7jfx8SWYfx+3avyfD46vkIZZFhtd8uYf4qLagAy47G4WW?=
 =?us-ascii?Q?60GWeW6bNOewzrgdRcdNq35WMmrXFm1x0phkWgPdOJDJZ/gmfGuix/u36AfC?=
 =?us-ascii?Q?TkTbpeUIhAS2HFX276pqrMpxydwNLaIWTSxR/hkKky5Z75gEm7xCTc2WUJFs?=
 =?us-ascii?Q?qQ=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <59241C9B7CDFE747B7934CAB71ACB94F@namprd10.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 678042db-16c6-4f82-5d8a-08daa6127d35
X-MS-Exchange-CrossTenant-originalarrivaltime: 04 Oct 2022 14:12:36.6305
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: w2f0LGndEOQcPnlYZlxQu5AaGBLtiKpD82da0CBVJg1NfU88djxxvxxKSuxa3DCHuB6cfBKiuZCcfJh4DxkYiw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS7PR10MB4925
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.895,Hydra:6.0.528,FMLib:17.11.122.1
 definitions=2022-10-04_06,2022-09-29_03,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 mlxlogscore=999
 suspectscore=0 phishscore=0 bulkscore=0 malwarescore=0 mlxscore=0
 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2209130000 definitions=main-2210040092
X-Proofpoint-GUID: MmgVUVGu16hyv0yy4EH1Z7-gpQlH0ZqC
X-Proofpoint-ORIG-GUID: MmgVUVGu16hyv0yy4EH1Z7-gpQlH0ZqC
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org


> On Oct 4, 2022, at 1:14 AM, NeilBrown <neilb@suse.de> wrote:
>=20
>=20
> Hi,
> I have a customer who experienced a crash in nfsd which appears to be
> related to delegation return.  I cannot completely rule-out
>  Commit 548ec0805c39 ("nfsd: fix use-after-free due to delegation race")
> as the kernel being used didn't have that commit, but the symptoms are
> quite different, and while exploring I found, I think, a different
> race.  This new race doesn't obviously address all the symptoms, but
> maybe it does...
>=20
> The symptoms were:
>  1/   WARN_ON(!unhash_delegation_locked(dp));
>    in nfs4_laundromat complained (delegation wasn't hashed!)
>  2/   refcount_t: saturated; leaking memory
>    This came from the refcount_inc in revoke_delegation() called from
>    nfs4_laundromat(), a few lines below the above warning
>  3/ BUG: kernel NULL pointer dereference, address: 0000000000000028
>     This is from the destroy_unhashed_deleg() call at the end of
>     that same revoke_delegation() call, which calls
>     nfs4_unlock_deleg_lease() and passes fp->fi_deleg_file, which is
>     NULL (!!!), to vfs_setlease().
>  These three happened in a 200usec window.
>=20
> What I imagine might be happening is that the nfsd_break_deleg_cb()
> callback is called after destroy_delegation() has unhashed the deleg,
> but before destroy_unhashed_delegation() gets called.
>=20
> If nfsd_break_deleg_cb() is called before the unhash - and particularly
> if nfsd_break_one_deleg()->nfsd4_run_cb() is called before, then the
> unhash will disconnect the delegation from the recall list, and no
> harm can be done.
> Once vfs_setlease(F_UNLCK) is called, the callback can no longer be
> called, so again no harm is possible.
>=20
> Between these two is a race window.  The delegation can be put on the
> recall list, but the deleg will be unhashed and put_deleg_file() will
> have set fi_deleg_file to NULL - resulting in first WARNING and the
> BUG.

That seems plausible. I've been accepting defensive patches like
what you proposed below, so I can queue that up for v6.2 as soon as
you post an official version.

It would help to know the kernel version where you encountered
these symptoms, and to have a rough description of the workload;
I assume you do not have a reliable reproducer. I'm wondering if
there should be a bug report too (bugzilla.linux-nfs.org)?


> I cannot see how the refcount_t warning can be generated ...  so maybe
> I've missed something.

stid refcounting does not seem reliable in the current code base.
It's possible that the overflow is a separate issue that simply
appeared at the same time or due to the same conditions that
triggered the BUG.


> My proposed solution is to test delegation_hashed() while holding
> fp->fi_lock in nfsd_break_deleg_cb().  If the delegation is locked, we
> can safely schedule the recall.  If it isn't, then the lease is about
> to be unlocked and there is no need to schedule anything.
>=20
> I don't know this code at all well, so I thought it safest to ask for
> comments before posting a proper patch.
> I'm particularly curious to know if anyone has ideas about the refcount
> overflow.  Corruption is unlikely as the deleg looked mostly OK and the
> memory has been freed, but not reallocated (though it is possible it
> was freed, reallocated, and freed again).
> This wasn't a refcount_inc on a zero refcount - that gives a different
> error.  I don't know what the refcount value was, it has already been
> changed to the 'saturated' value - 0xc0000000.
>=20
> Thanks,
> NeilBrown
>=20
>=20
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index c5d199d7e6b4..e02d638df6be 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -4822,8 +4822,10 @@ nfsd_break_deleg_cb(struct file_lock *fl)
> 	fl->fl_break_time =3D 0;
>=20
> 	spin_lock(&fp->fi_lock);
> -	fp->fi_had_conflict =3D true;
> -	nfsd_break_one_deleg(dp);
> +	if (delegation_hashed(dp)) {
> +		fp->fi_had_conflict =3D true;
> +		nfsd_break_one_deleg(dp);
> +	}
> 	spin_unlock(&fp->fi_lock);
> 	return ret;
> }
>=20
>=20

--
Chuck Lever