Linux NFS development
 help / color / mirror / Atom feed
* [PATCH RFT v2] nfsd: provide locking for v4_end_grace
@ 2025-07-01 10:43 NeilBrown
  2025-07-02  7:03 ` Li Lingfeng
  2025-07-02 13:56 ` Chuck Lever
  0 siblings, 2 replies; 8+ messages in thread
From: NeilBrown @ 2025-07-01 10:43 UTC (permalink / raw)
  To: Chuck Lever, Jeff Layton, Li Lingfeng
  Cc: linux-nfs, Olga Kornievskaia, Dai Ngo, Tom Talpey


Writing to v4_end_grace can race with server shutdown and result in
memory being accessed after it was freed - reclaim_str_hashtbl in
particularly.

We cannot hold nfsd_mutex across the nfsd4_end_grace() call as that is
held while client_tracking_ops->init() is called and that can wait for
an upcall to nfsdcltrack which can write to v4_end_grace, resulting in a
deadlock.

nfsd4_end_grace() is also called by the landromat work queue and this
doesn't require locking as server shutdown will stop the work and wait
for it before freeing anything that nfsd4_end_grace() might access.

However, we must be sure that writing to v4_end_grace doesn't restart
the work item before init has completed or after shutdown has already
waited for it.  For this we can use disable_delayed_work() after
INIT_DELAYED_WORK(), and disable_delayed_work_sync() instead of
cancel_delayed_work_sync().

So this patch adds a nfsd_net field "grace_end_forced", sets that when
v4_end_grace is written, and schedules the laundromat (providing it
hasn't been disabled).  This field bypasses other checks for whether the
grace period has finished.  The delayed work is disabled while
nfsd4_client_tracking_init() is running and before
nfsd4_client_tracking_exit() is call to shutdown client tracking.

This resolves a race which can result in use-after-free.

Note that disable_delayed_work_sync() was added in v6.10.  To backport
to an earlier kernel without that interface the exclusion could be
provided by a spinlock and a flag in nn. The flag would be set when
the delayed_work is enabled and cleared when it is disabled.  The
spinlock would be used to ensure nfsd4_force_end_grace() only queues the
work while the flag is set.

[[ 
 v2 - disable laundromat_work while _init is running as well as while
 _exit is running.  Don't depend on ->nfsd_serv, test
 ->client_tracking_ops instead.
]]

Reported-by: Li Lingfeng <lilingfeng3@huawei.com>
Cc: stable@vger.kernel.org
Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/nfsd/netns.h     |  1 +
 fs/nfsd/nfs4state.c | 27 ++++++++++++++++++++++++---
 fs/nfsd/nfsctl.c    |  6 +++---
 fs/nfsd/state.h     |  2 +-
 4 files changed, 29 insertions(+), 7 deletions(-)

diff --git a/fs/nfsd/netns.h b/fs/nfsd/netns.h
index 3e2d0fde80a7..d83c68872c4c 100644
--- a/fs/nfsd/netns.h
+++ b/fs/nfsd/netns.h
@@ -66,6 +66,7 @@ struct nfsd_net {
 
 	struct lock_manager nfsd4_manager;
 	bool grace_ended;
+	bool grace_end_forced;
 	time64_t boot_time;
 
 	struct dentry *nfsd_client_dir;
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index d5694987f86f..857606035f94 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -84,7 +84,7 @@ static u64 current_sessionid = 1;
 /* forward declarations */
 static bool check_for_locks(struct nfs4_file *fp, struct nfs4_lockowner *lowner);
 static void nfs4_free_ol_stateid(struct nfs4_stid *stid);
-void nfsd4_end_grace(struct nfsd_net *nn);
+static void nfsd4_end_grace(struct nfsd_net *nn);
 static void _free_cpntf_state_locked(struct nfsd_net *nn, struct nfs4_cpntf_state *cps);
 static void nfsd4_file_hash_remove(struct nfs4_file *fi);
 static void deleg_reaper(struct nfsd_net *nn);
@@ -6458,7 +6458,7 @@ nfsd4_renew(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	return nfs_ok;
 }
 
-void
+static void
 nfsd4_end_grace(struct nfsd_net *nn)
 {
 	/* do nothing if grace period already ended */
@@ -6491,6 +6491,20 @@ nfsd4_end_grace(struct nfsd_net *nn)
 	 */
 }
 
+bool
+nfsd4_force_end_grace(struct nfsd_net *nn)
+{
+	if (!nn->client_tracking_ops)
+		return false;
+	/* laundromat_work must be initialised now, though it might be disabled */
+	nn->grace_end_forced = true;
+	/* This is a no-op after nfs4_state_shutdown_net() has called
+	 * disable_delayed_work_sync()
+	 */
+	mod_delayed_work(laundry_wq, &nn->laundromat_work, 0);
+	return true;
+}
+
 /*
  * If we've waited a lease period but there are still clients trying to
  * reclaim, wait a little longer to give them a chance to finish.
@@ -6500,6 +6514,8 @@ static bool clients_still_reclaiming(struct nfsd_net *nn)
 	time64_t double_grace_period_end = nn->boot_time +
 					   2 * nn->nfsd4_lease;
 
+	if (nn->grace_end_forced)
+		return false;
 	if (nn->track_reclaim_completes &&
 			atomic_read(&nn->nr_reclaim_complete) ==
 			nn->reclaim_str_hashtbl_size)
@@ -8807,6 +8823,7 @@ static int nfs4_state_create_net(struct net *net)
 	nn->unconf_name_tree = RB_ROOT;
 	nn->boot_time = ktime_get_real_seconds();
 	nn->grace_ended = false;
+	nn->grace_end_forced = false;
 	nn->nfsd4_manager.block_opens = true;
 	INIT_LIST_HEAD(&nn->nfsd4_manager.list);
 	INIT_LIST_HEAD(&nn->client_lru);
@@ -8821,6 +8838,8 @@ static int nfs4_state_create_net(struct net *net)
 	INIT_LIST_HEAD(&nn->blocked_locks_lru);
 
 	INIT_DELAYED_WORK(&nn->laundromat_work, laundromat_main);
+	/* Make sure his cannot run until client tracking is initialised */
+	disable_delayed_work(&nn->laundromat_work);
 	INIT_WORK(&nn->nfsd_shrinker_work, nfsd4_state_shrinker_worker);
 	get_net(net);
 
@@ -8887,6 +8906,8 @@ nfs4_state_start_net(struct net *net)
 		return ret;
 	locks_start_grace(net, &nn->nfsd4_manager);
 	nfsd4_client_tracking_init(net);
+	/* safe for laundromat to run now */
+	enable_delayed_work(&nn->laundromat_work);
 	if (nn->track_reclaim_completes && nn->reclaim_str_hashtbl_size == 0)
 		goto skip_grace;
 	printk(KERN_INFO "NFSD: starting %lld-second grace period (net %x)\n",
@@ -8935,7 +8956,7 @@ nfs4_state_shutdown_net(struct net *net)
 
 	shrinker_free(nn->nfsd_client_shrinker);
 	cancel_work_sync(&nn->nfsd_shrinker_work);
-	cancel_delayed_work_sync(&nn->laundromat_work);
+	disable_delayed_work_sync(&nn->laundromat_work);
 	locks_end_grace(&nn->nfsd4_manager);
 
 	INIT_LIST_HEAD(&reaplist);
diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
index 3f3e9f6c4250..658f3f86a59f 100644
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -1082,10 +1082,10 @@ static ssize_t write_v4_end_grace(struct file *file, char *buf, size_t size)
 		case 'Y':
 		case 'y':
 		case '1':
-			if (!nn->nfsd_serv)
-				return -EBUSY;
 			trace_nfsd_end_grace(netns(file));
-			nfsd4_end_grace(nn);
+			if (!nfsd4_force_end_grace(nn))
+				return -EBUSY;
+
 			break;
 		default:
 			return -EINVAL;
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index 1995bca158b8..05eabc69de40 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -836,7 +836,7 @@ static inline void nfsd4_revoke_states(struct net *net, struct super_block *sb)
 #endif
 
 /* grace period management */
-void nfsd4_end_grace(struct nfsd_net *nn);
+bool nfsd4_force_end_grace(struct nfsd_net *nn);
 
 /* nfs4recover operations */
 extern int nfsd4_client_tracking_init(struct net *net);
-- 
2.49.0


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH RFT v2] nfsd: provide locking for v4_end_grace
  2025-07-01 10:43 [PATCH RFT v2] nfsd: provide locking for v4_end_grace NeilBrown
@ 2025-07-02  7:03 ` Li Lingfeng
  2025-07-02  9:38   ` NeilBrown
  2025-07-02 13:56 ` Chuck Lever
  1 sibling, 1 reply; 8+ messages in thread
From: Li Lingfeng @ 2025-07-02  7:03 UTC (permalink / raw)
  To: NeilBrown
  Cc: linux-nfs, Olga Kornievskaia, Dai Ngo, Tom Talpey, Chuck Lever,
	Jeff Layton, yangerkun, zhangyi (F), Hou Tao,
	chengzhihao1@huawei.com, yukuai (C)

Hi Neil,

Thank you for the patch. I tested it on mainline and can confirm it
resolves the issue I encountered without triggering any deadlocks.
The approach looks solid, and I'll have my colleagues review it as well.

However, during validation on our internal 5.10-based kernel (which
backported disable_delayed_work/disable_delayed_work_sync), I observed an
unexpected behavior: the laundromat_work still executed after calling
disable_delayed_work and before enable_delayed_work could be invoked.
I'll investigate why this occurred.

As you mentioned, disable_delayed_work_sync() was introduced in v6.10.
Do you have plans to provide a backport solution for earlier kernel?

Thanks,
Lingfeng.

在 2025/7/1 18:43, NeilBrown 写道:
> Writing to v4_end_grace can race with server shutdown and result in
> memory being accessed after it was freed - reclaim_str_hashtbl in
> particularly.
>
> We cannot hold nfsd_mutex across the nfsd4_end_grace() call as that is
> held while client_tracking_ops->init() is called and that can wait for
> an upcall to nfsdcltrack which can write to v4_end_grace, resulting in a
> deadlock.
>
> nfsd4_end_grace() is also called by the landromat work queue and this
> doesn't require locking as server shutdown will stop the work and wait
> for it before freeing anything that nfsd4_end_grace() might access.
>
> However, we must be sure that writing to v4_end_grace doesn't restart
> the work item before init has completed or after shutdown has already
> waited for it.  For this we can use disable_delayed_work() after
> INIT_DELAYED_WORK(), and disable_delayed_work_sync() instead of
> cancel_delayed_work_sync().
>
> So this patch adds a nfsd_net field "grace_end_forced", sets that when
> v4_end_grace is written, and schedules the laundromat (providing it
> hasn't been disabled).  This field bypasses other checks for whether the
> grace period has finished.  The delayed work is disabled while
> nfsd4_client_tracking_init() is running and before
> nfsd4_client_tracking_exit() is call to shutdown client tracking.
>
> This resolves a race which can result in use-after-free.
>
> Note that disable_delayed_work_sync() was added in v6.10.  To backport
> to an earlier kernel without that interface the exclusion could be
> provided by a spinlock and a flag in nn. The flag would be set when
> the delayed_work is enabled and cleared when it is disabled.  The
> spinlock would be used to ensure nfsd4_force_end_grace() only queues the
> work while the flag is set.
>
> [[
>   v2 - disable laundromat_work while _init is running as well as while
>   _exit is running.  Don't depend on ->nfsd_serv, test
>   ->client_tracking_ops instead.
> ]]
>
> Reported-by: Li Lingfeng <lilingfeng3@huawei.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>   fs/nfsd/netns.h     |  1 +
>   fs/nfsd/nfs4state.c | 27 ++++++++++++++++++++++++---
>   fs/nfsd/nfsctl.c    |  6 +++---
>   fs/nfsd/state.h     |  2 +-
>   4 files changed, 29 insertions(+), 7 deletions(-)
>
> diff --git a/fs/nfsd/netns.h b/fs/nfsd/netns.h
> index 3e2d0fde80a7..d83c68872c4c 100644
> --- a/fs/nfsd/netns.h
> +++ b/fs/nfsd/netns.h
> @@ -66,6 +66,7 @@ struct nfsd_net {
>   
>   	struct lock_manager nfsd4_manager;
>   	bool grace_ended;
> +	bool grace_end_forced;
>   	time64_t boot_time;
>   
>   	struct dentry *nfsd_client_dir;
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index d5694987f86f..857606035f94 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -84,7 +84,7 @@ static u64 current_sessionid = 1;
>   /* forward declarations */
>   static bool check_for_locks(struct nfs4_file *fp, struct nfs4_lockowner *lowner);
>   static void nfs4_free_ol_stateid(struct nfs4_stid *stid);
> -void nfsd4_end_grace(struct nfsd_net *nn);
> +static void nfsd4_end_grace(struct nfsd_net *nn);
>   static void _free_cpntf_state_locked(struct nfsd_net *nn, struct nfs4_cpntf_state *cps);
>   static void nfsd4_file_hash_remove(struct nfs4_file *fi);
>   static void deleg_reaper(struct nfsd_net *nn);
> @@ -6458,7 +6458,7 @@ nfsd4_renew(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>   	return nfs_ok;
>   }
>   
> -void
> +static void
>   nfsd4_end_grace(struct nfsd_net *nn)
>   {
>   	/* do nothing if grace period already ended */
> @@ -6491,6 +6491,20 @@ nfsd4_end_grace(struct nfsd_net *nn)
>   	 */
>   }
>   
> +bool
> +nfsd4_force_end_grace(struct nfsd_net *nn)
> +{
> +	if (!nn->client_tracking_ops)
> +		return false;
> +	/* laundromat_work must be initialised now, though it might be disabled */
> +	nn->grace_end_forced = true;
> +	/* This is a no-op after nfs4_state_shutdown_net() has called
> +	 * disable_delayed_work_sync()
> +	 */
> +	mod_delayed_work(laundry_wq, &nn->laundromat_work, 0);
> +	return true;
> +}
> +
>   /*
>    * If we've waited a lease period but there are still clients trying to
>    * reclaim, wait a little longer to give them a chance to finish.
> @@ -6500,6 +6514,8 @@ static bool clients_still_reclaiming(struct nfsd_net *nn)
>   	time64_t double_grace_period_end = nn->boot_time +
>   					   2 * nn->nfsd4_lease;
>   
> +	if (nn->grace_end_forced)
> +		return false;
>   	if (nn->track_reclaim_completes &&
>   			atomic_read(&nn->nr_reclaim_complete) ==
>   			nn->reclaim_str_hashtbl_size)
> @@ -8807,6 +8823,7 @@ static int nfs4_state_create_net(struct net *net)
>   	nn->unconf_name_tree = RB_ROOT;
>   	nn->boot_time = ktime_get_real_seconds();
>   	nn->grace_ended = false;
> +	nn->grace_end_forced = false;
>   	nn->nfsd4_manager.block_opens = true;
>   	INIT_LIST_HEAD(&nn->nfsd4_manager.list);
>   	INIT_LIST_HEAD(&nn->client_lru);
> @@ -8821,6 +8838,8 @@ static int nfs4_state_create_net(struct net *net)
>   	INIT_LIST_HEAD(&nn->blocked_locks_lru);
>   
>   	INIT_DELAYED_WORK(&nn->laundromat_work, laundromat_main);
> +	/* Make sure his cannot run until client tracking is initialised */
> +	disable_delayed_work(&nn->laundromat_work);
>   	INIT_WORK(&nn->nfsd_shrinker_work, nfsd4_state_shrinker_worker);
>   	get_net(net);
>   
> @@ -8887,6 +8906,8 @@ nfs4_state_start_net(struct net *net)
>   		return ret;
>   	locks_start_grace(net, &nn->nfsd4_manager);
>   	nfsd4_client_tracking_init(net);
> +	/* safe for laundromat to run now */
> +	enable_delayed_work(&nn->laundromat_work);
>   	if (nn->track_reclaim_completes && nn->reclaim_str_hashtbl_size == 0)
>   		goto skip_grace;
>   	printk(KERN_INFO "NFSD: starting %lld-second grace period (net %x)\n",
> @@ -8935,7 +8956,7 @@ nfs4_state_shutdown_net(struct net *net)
>   
>   	shrinker_free(nn->nfsd_client_shrinker);
>   	cancel_work_sync(&nn->nfsd_shrinker_work);
> -	cancel_delayed_work_sync(&nn->laundromat_work);
> +	disable_delayed_work_sync(&nn->laundromat_work);
>   	locks_end_grace(&nn->nfsd4_manager);
>   
>   	INIT_LIST_HEAD(&reaplist);
> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> index 3f3e9f6c4250..658f3f86a59f 100644
> --- a/fs/nfsd/nfsctl.c
> +++ b/fs/nfsd/nfsctl.c
> @@ -1082,10 +1082,10 @@ static ssize_t write_v4_end_grace(struct file *file, char *buf, size_t size)
>   		case 'Y':
>   		case 'y':
>   		case '1':
> -			if (!nn->nfsd_serv)
> -				return -EBUSY;
>   			trace_nfsd_end_grace(netns(file));
> -			nfsd4_end_grace(nn);
> +			if (!nfsd4_force_end_grace(nn))
> +				return -EBUSY;
> +
>   			break;
>   		default:
>   			return -EINVAL;
> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> index 1995bca158b8..05eabc69de40 100644
> --- a/fs/nfsd/state.h
> +++ b/fs/nfsd/state.h
> @@ -836,7 +836,7 @@ static inline void nfsd4_revoke_states(struct net *net, struct super_block *sb)
>   #endif
>   
>   /* grace period management */
> -void nfsd4_end_grace(struct nfsd_net *nn);
> +bool nfsd4_force_end_grace(struct nfsd_net *nn);
>   
>   /* nfs4recover operations */
>   extern int nfsd4_client_tracking_init(struct net *net);
Tested-by: Li Lingfeng <lilingfeng3@huawei.com>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH RFT v2] nfsd: provide locking for v4_end_grace
  2025-07-02  7:03 ` Li Lingfeng
@ 2025-07-02  9:38   ` NeilBrown
  2025-07-02 12:48     ` Li Lingfeng
  0 siblings, 1 reply; 8+ messages in thread
From: NeilBrown @ 2025-07-02  9:38 UTC (permalink / raw)
  To: Li Lingfeng
  Cc: linux-nfs, Olga Kornievskaia, Dai Ngo, Tom Talpey, Chuck Lever,
	Jeff Layton, yangerkun, zhangyi (F), Hou Tao,
	chengzhihao1@huawei.com, yukuai (C)

On Wed, 02 Jul 2025, Li Lingfeng wrote:
> Hi Neil,
> 
> Thank you for the patch. I tested it on mainline and can confirm it
> resolves the issue I encountered without triggering any deadlocks.
> The approach looks solid, and I'll have my colleagues review it as well.

That's really good news - thanks.  And thanks for getting multiple
reviews - there could easily be details that I have missed.

> 
> However, during validation on our internal 5.10-based kernel (which
> backported disable_delayed_work/disable_delayed_work_sync), I observed an
> unexpected behavior: the laundromat_work still executed after calling
> disable_delayed_work and before enable_delayed_work could be invoked.
> I'll investigate why this occurred.
> 
> As you mentioned, disable_delayed_work_sync() was introduced in v6.10.
> Do you have plans to provide a backport solution for earlier kernel?

The following is how I would do it prior to 6.10.  It might make sense
to submit this upstream with the 'stable' tag, and then add a patch
which reverts to the simpler version for upstream only.

Let me know if this works on 5.10.

Thanks,
NeilBrown


Subject: [PATCH] nfsd: provide locking for v4_end_grace

Writing to v4_end_grace can race with server shutdown and result in
memory being accessed after it was freed - reclaim_str_hashtbl in
particularly.

We cannot hold nfsd_mutex across the nfsd4_end_grace() call as that is
held while client_tracking_op->init() is called and that can wait for
an upcall to nfsdcltrack which can write to v4_end_grace, resulting in a
deadlock.

nfsd4_end_grace() is also called by the landromat work queue and this
doesn't require locking as server shutdown will stop the work and wait
for it before freeing anything that nfsd4_end_grace() might access.

However, we must be sure that writing to v4_end_grace doesn't restart
the work item after shutdown has already waited for it.  For this we
add a new flag protected with nn->client_lock.  It is set only while it
is safe to make client tracking calls, and v4_end_grace only schedules
work while the flag is set with the spinlock held.

So this patch adds a nfsd_net field "client_tracking_active" which is
set as described.  Another field "grace_end_forced", is set when
v4_end_grace is written.  After this is set, and providing
client_tracking_active is set, the laundromat is scheduled.
This "grace_end_forced" field bypasses other checks for whether the
grace period has finished.

This resolves a race which can result in use-after-free.

Reported-by: Li Lingfeng <lilingfeng3@huawei.com>
Cc: stable@vger.kernel.org
Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/nfsd/netns.h     |  2 ++
 fs/nfsd/nfs4state.c | 29 +++++++++++++++++++++++++++--
 fs/nfsd/nfsctl.c    |  6 +++---
 fs/nfsd/state.h     |  2 +-
 4 files changed, 33 insertions(+), 6 deletions(-)

diff --git a/fs/nfsd/netns.h b/fs/nfsd/netns.h
index 3e2d0fde80a7..fe8338735e7c 100644
--- a/fs/nfsd/netns.h
+++ b/fs/nfsd/netns.h
@@ -66,6 +66,8 @@ struct nfsd_net {
 
 	struct lock_manager nfsd4_manager;
 	bool grace_ended;
+	bool grace_end_forced;
+	bool client_tracking_active;
 	time64_t boot_time;
 
 	struct dentry *nfsd_client_dir;
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index d5694987f86f..d17a40f95eb2 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -84,7 +84,7 @@ static u64 current_sessionid = 1;
 /* forward declarations */
 static bool check_for_locks(struct nfs4_file *fp, struct nfs4_lockowner *lowner);
 static void nfs4_free_ol_stateid(struct nfs4_stid *stid);
-void nfsd4_end_grace(struct nfsd_net *nn);
+static void nfsd4_end_grace(struct nfsd_net *nn);
 static void _free_cpntf_state_locked(struct nfsd_net *nn, struct nfs4_cpntf_state *cps);
 static void nfsd4_file_hash_remove(struct nfs4_file *fi);
 static void deleg_reaper(struct nfsd_net *nn);
@@ -6458,7 +6458,7 @@ nfsd4_renew(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	return nfs_ok;
 }
 
-void
+static void
 nfsd4_end_grace(struct nfsd_net *nn)
 {
 	/* do nothing if grace period already ended */
@@ -6491,6 +6491,20 @@ nfsd4_end_grace(struct nfsd_net *nn)
 	 */
 }
 
+bool
+nfsd4_force_end_grace(struct nfsd_net *nn)
+{
+	if (!nn->client_tracking_ops)
+		return false;
+	spin_lock(&nn->client_lock);
+	if (nn->client_tracking_active) {
+		nn->grace_end_forced = true;
+		mod_delayed_work(laundry_wq, &nn->laundromat_work, 0);
+	}
+	spin_unlock(&nn->client_lock);
+	return true;
+}
+
 /*
  * If we've waited a lease period but there are still clients trying to
  * reclaim, wait a little longer to give them a chance to finish.
@@ -6500,6 +6514,8 @@ static bool clients_still_reclaiming(struct nfsd_net *nn)
 	time64_t double_grace_period_end = nn->boot_time +
 					   2 * nn->nfsd4_lease;
 
+	if (nn->grace_end_forced)
+		return false;
 	if (nn->track_reclaim_completes &&
 			atomic_read(&nn->nr_reclaim_complete) ==
 			nn->reclaim_str_hashtbl_size)
@@ -8807,6 +8823,8 @@ static int nfs4_state_create_net(struct net *net)
 	nn->unconf_name_tree = RB_ROOT;
 	nn->boot_time = ktime_get_real_seconds();
 	nn->grace_ended = false;
+	nn->grace_end_forced = false;
+	nn->client_tracking_active = false;
 	nn->nfsd4_manager.block_opens = true;
 	INIT_LIST_HEAD(&nn->nfsd4_manager.list);
 	INIT_LIST_HEAD(&nn->client_lru);
@@ -8887,6 +8905,10 @@ nfs4_state_start_net(struct net *net)
 		return ret;
 	locks_start_grace(net, &nn->nfsd4_manager);
 	nfsd4_client_tracking_init(net);
+	/* safe for laundromat to run now */
+	spin_lock(&nn->client_lock);
+	nn->client_tracking_active = true;
+	spin_unlock(&nn->client_lock);
 	if (nn->track_reclaim_completes && nn->reclaim_str_hashtbl_size == 0)
 		goto skip_grace;
 	printk(KERN_INFO "NFSD: starting %lld-second grace period (net %x)\n",
@@ -8935,6 +8957,9 @@ nfs4_state_shutdown_net(struct net *net)
 
 	shrinker_free(nn->nfsd_client_shrinker);
 	cancel_work_sync(&nn->nfsd_shrinker_work);
+	spin_lock(&nn->client_lock);
+	nn->client_tracking_active = false;
+	spin_unlock(&nn->client_lock);
 	cancel_delayed_work_sync(&nn->laundromat_work);
 	locks_end_grace(&nn->nfsd4_manager);
 
diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
index 3f3e9f6c4250..658f3f86a59f 100644
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -1082,10 +1082,10 @@ static ssize_t write_v4_end_grace(struct file *file, char *buf, size_t size)
 		case 'Y':
 		case 'y':
 		case '1':
-			if (!nn->nfsd_serv)
-				return -EBUSY;
 			trace_nfsd_end_grace(netns(file));
-			nfsd4_end_grace(nn);
+			if (!nfsd4_force_end_grace(nn))
+				return -EBUSY;
+
 			break;
 		default:
 			return -EINVAL;
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index 1995bca158b8..05eabc69de40 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -836,7 +836,7 @@ static inline void nfsd4_revoke_states(struct net *net, struct super_block *sb)
 #endif
 
 /* grace period management */
-void nfsd4_end_grace(struct nfsd_net *nn);
+bool nfsd4_force_end_grace(struct nfsd_net *nn);
 
 /* nfs4recover operations */
 extern int nfsd4_client_tracking_init(struct net *net);

base-commit: e04c78d86a9699d136910cfc0bdcf01087e3267e
-- 
2.49.0


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH RFT v2] nfsd: provide locking for v4_end_grace
  2025-07-02  9:38   ` NeilBrown
@ 2025-07-02 12:48     ` Li Lingfeng
  0 siblings, 0 replies; 8+ messages in thread
From: Li Lingfeng @ 2025-07-02 12:48 UTC (permalink / raw)
  To: NeilBrown
  Cc: linux-nfs, Olga Kornievskaia, Dai Ngo, Tom Talpey, Chuck Lever,
	Jeff Layton, yangerkun, zhangyi (F), Hou Tao,
	chengzhihao1@huawei.com, yukuai (C)

Hi Neil,

Thank you for the patch. I have adapted and applied this patch, then
reproduced the original issue and verified the fix on both the
linux-5.10.y (base commit ecbc622e0f52) and our internal 5.10-based
version. Based on my testing, the issue appears resolved in both
environments, and I observed no new issues during the verification
process.
Appreciate your help with this!

Thanks,
Lingfeng.

在 2025/7/2 17:38, NeilBrown 写道:
> On Wed, 02 Jul 2025, Li Lingfeng wrote:
>> Hi Neil,
>>
>> Thank you for the patch. I tested it on mainline and can confirm it
>> resolves the issue I encountered without triggering any deadlocks.
>> The approach looks solid, and I'll have my colleagues review it as well.
> That's really good news - thanks.  And thanks for getting multiple
> reviews - there could easily be details that I have missed.
>
>> However, during validation on our internal 5.10-based kernel (which
>> backported disable_delayed_work/disable_delayed_work_sync), I observed an
>> unexpected behavior: the laundromat_work still executed after calling
>> disable_delayed_work and before enable_delayed_work could be invoked.
>> I'll investigate why this occurred.
>>
>> As you mentioned, disable_delayed_work_sync() was introduced in v6.10.
>> Do you have plans to provide a backport solution for earlier kernel?
> The following is how I would do it prior to 6.10.  It might make sense
> to submit this upstream with the 'stable' tag, and then add a patch
> which reverts to the simpler version for upstream only.
>
> Let me know if this works on 5.10.
>
> Thanks,
> NeilBrown
>
>
> Subject: [PATCH] nfsd: provide locking for v4_end_grace
>
> Writing to v4_end_grace can race with server shutdown and result in
> memory being accessed after it was freed - reclaim_str_hashtbl in
> particularly.
>
> We cannot hold nfsd_mutex across the nfsd4_end_grace() call as that is
> held while client_tracking_op->init() is called and that can wait for
> an upcall to nfsdcltrack which can write to v4_end_grace, resulting in a
> deadlock.
>
> nfsd4_end_grace() is also called by the landromat work queue and this
> doesn't require locking as server shutdown will stop the work and wait
> for it before freeing anything that nfsd4_end_grace() might access.
>
> However, we must be sure that writing to v4_end_grace doesn't restart
> the work item after shutdown has already waited for it.  For this we
> add a new flag protected with nn->client_lock.  It is set only while it
> is safe to make client tracking calls, and v4_end_grace only schedules
> work while the flag is set with the spinlock held.
>
> So this patch adds a nfsd_net field "client_tracking_active" which is
> set as described.  Another field "grace_end_forced", is set when
> v4_end_grace is written.  After this is set, and providing
> client_tracking_active is set, the laundromat is scheduled.
> This "grace_end_forced" field bypasses other checks for whether the
> grace period has finished.
>
> This resolves a race which can result in use-after-free.
>
> Reported-by: Li Lingfeng <lilingfeng3@huawei.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>   fs/nfsd/netns.h     |  2 ++
>   fs/nfsd/nfs4state.c | 29 +++++++++++++++++++++++++++--
>   fs/nfsd/nfsctl.c    |  6 +++---
>   fs/nfsd/state.h     |  2 +-
>   4 files changed, 33 insertions(+), 6 deletions(-)
>
> diff --git a/fs/nfsd/netns.h b/fs/nfsd/netns.h
> index 3e2d0fde80a7..fe8338735e7c 100644
> --- a/fs/nfsd/netns.h
> +++ b/fs/nfsd/netns.h
> @@ -66,6 +66,8 @@ struct nfsd_net {
>   
>   	struct lock_manager nfsd4_manager;
>   	bool grace_ended;
> +	bool grace_end_forced;
> +	bool client_tracking_active;
>   	time64_t boot_time;
>   
>   	struct dentry *nfsd_client_dir;
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index d5694987f86f..d17a40f95eb2 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -84,7 +84,7 @@ static u64 current_sessionid = 1;
>   /* forward declarations */
>   static bool check_for_locks(struct nfs4_file *fp, struct nfs4_lockowner *lowner);
>   static void nfs4_free_ol_stateid(struct nfs4_stid *stid);
> -void nfsd4_end_grace(struct nfsd_net *nn);
> +static void nfsd4_end_grace(struct nfsd_net *nn);
>   static void _free_cpntf_state_locked(struct nfsd_net *nn, struct nfs4_cpntf_state *cps);
>   static void nfsd4_file_hash_remove(struct nfs4_file *fi);
>   static void deleg_reaper(struct nfsd_net *nn);
> @@ -6458,7 +6458,7 @@ nfsd4_renew(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>   	return nfs_ok;
>   }
>   
> -void
> +static void
>   nfsd4_end_grace(struct nfsd_net *nn)
>   {
>   	/* do nothing if grace period already ended */
> @@ -6491,6 +6491,20 @@ nfsd4_end_grace(struct nfsd_net *nn)
>   	 */
>   }
>   
> +bool
> +nfsd4_force_end_grace(struct nfsd_net *nn)
> +{
> +	if (!nn->client_tracking_ops)
> +		return false;
> +	spin_lock(&nn->client_lock);
> +	if (nn->client_tracking_active) {
> +		nn->grace_end_forced = true;
> +		mod_delayed_work(laundry_wq, &nn->laundromat_work, 0);
> +	}
> +	spin_unlock(&nn->client_lock);
> +	return true;
> +}
> +
>   /*
>    * If we've waited a lease period but there are still clients trying to
>    * reclaim, wait a little longer to give them a chance to finish.
> @@ -6500,6 +6514,8 @@ static bool clients_still_reclaiming(struct nfsd_net *nn)
>   	time64_t double_grace_period_end = nn->boot_time +
>   					   2 * nn->nfsd4_lease;
>   
> +	if (nn->grace_end_forced)
> +		return false;
>   	if (nn->track_reclaim_completes &&
>   			atomic_read(&nn->nr_reclaim_complete) ==
>   			nn->reclaim_str_hashtbl_size)
> @@ -8807,6 +8823,8 @@ static int nfs4_state_create_net(struct net *net)
>   	nn->unconf_name_tree = RB_ROOT;
>   	nn->boot_time = ktime_get_real_seconds();
>   	nn->grace_ended = false;
> +	nn->grace_end_forced = false;
> +	nn->client_tracking_active = false;
>   	nn->nfsd4_manager.block_opens = true;
>   	INIT_LIST_HEAD(&nn->nfsd4_manager.list);
>   	INIT_LIST_HEAD(&nn->client_lru);
> @@ -8887,6 +8905,10 @@ nfs4_state_start_net(struct net *net)
>   		return ret;
>   	locks_start_grace(net, &nn->nfsd4_manager);
>   	nfsd4_client_tracking_init(net);
> +	/* safe for laundromat to run now */
> +	spin_lock(&nn->client_lock);
> +	nn->client_tracking_active = true;
> +	spin_unlock(&nn->client_lock);
>   	if (nn->track_reclaim_completes && nn->reclaim_str_hashtbl_size == 0)
>   		goto skip_grace;
>   	printk(KERN_INFO "NFSD: starting %lld-second grace period (net %x)\n",
> @@ -8935,6 +8957,9 @@ nfs4_state_shutdown_net(struct net *net)
>   
>   	shrinker_free(nn->nfsd_client_shrinker);
>   	cancel_work_sync(&nn->nfsd_shrinker_work);
> +	spin_lock(&nn->client_lock);
> +	nn->client_tracking_active = false;
> +	spin_unlock(&nn->client_lock);
>   	cancel_delayed_work_sync(&nn->laundromat_work);
>   	locks_end_grace(&nn->nfsd4_manager);
>   
> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> index 3f3e9f6c4250..658f3f86a59f 100644
> --- a/fs/nfsd/nfsctl.c
> +++ b/fs/nfsd/nfsctl.c
> @@ -1082,10 +1082,10 @@ static ssize_t write_v4_end_grace(struct file *file, char *buf, size_t size)
>   		case 'Y':
>   		case 'y':
>   		case '1':
> -			if (!nn->nfsd_serv)
> -				return -EBUSY;
>   			trace_nfsd_end_grace(netns(file));
> -			nfsd4_end_grace(nn);
> +			if (!nfsd4_force_end_grace(nn))
> +				return -EBUSY;
> +
>   			break;
>   		default:
>   			return -EINVAL;
> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> index 1995bca158b8..05eabc69de40 100644
> --- a/fs/nfsd/state.h
> +++ b/fs/nfsd/state.h
> @@ -836,7 +836,7 @@ static inline void nfsd4_revoke_states(struct net *net, struct super_block *sb)
>   #endif
>   
>   /* grace period management */
> -void nfsd4_end_grace(struct nfsd_net *nn);
> +bool nfsd4_force_end_grace(struct nfsd_net *nn);
>   
>   /* nfs4recover operations */
>   extern int nfsd4_client_tracking_init(struct net *net);
>
> base-commit: e04c78d86a9699d136910cfc0bdcf01087e3267e
Tested-by: Li Lingfeng <lilingfeng3@huawei.com>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH RFT v2] nfsd: provide locking for v4_end_grace
  2025-07-01 10:43 [PATCH RFT v2] nfsd: provide locking for v4_end_grace NeilBrown
  2025-07-02  7:03 ` Li Lingfeng
@ 2025-07-02 13:56 ` Chuck Lever
  2025-07-02 21:22   ` NeilBrown
  1 sibling, 1 reply; 8+ messages in thread
From: Chuck Lever @ 2025-07-02 13:56 UTC (permalink / raw)
  To: NeilBrown
  Cc: linux-nfs, Olga Kornievskaia, Dai Ngo, Tom Talpey, Jeff Layton,
	Li Lingfeng

Hi Neil, handful of nits below.


On 7/1/25 6:43 AM, NeilBrown wrote:
> 
> Writing to v4_end_grace can race with server shutdown and result in
> memory being accessed after it was freed - reclaim_str_hashtbl in
> particularly.
> 
> We cannot hold nfsd_mutex across the nfsd4_end_grace() call as that is
> held while client_tracking_ops->init() is called and that can wait for
> an upcall to nfsdcltrack which can write to v4_end_grace, resulting in a
> deadlock.
> 
> nfsd4_end_grace() is also called by the landromat work queue and this
> doesn't require locking as server shutdown will stop the work and wait
> for it before freeing anything that nfsd4_end_grace() might access.
> 
> However, we must be sure that writing to v4_end_grace doesn't restart
> the work item before init has completed or after shutdown has already
> waited for it.  For this we can use disable_delayed_work() after
> INIT_DELAYED_WORK(), and disable_delayed_work_sync() instead of
> cancel_delayed_work_sync().
> 
> So this patch adds a nfsd_net field "grace_end_forced", sets that when
> v4_end_grace is written, and schedules the laundromat (providing it
> hasn't been disabled).  This field bypasses other checks for whether the
> grace period has finished.  The delayed work is disabled while
> nfsd4_client_tracking_init() is running and before
> nfsd4_client_tracking_exit() is call to shutdown client tracking.
> 
> This resolves a race which can result in use-after-free.
> 
> Note that disable_delayed_work_sync() was added in v6.10.  To backport
> to an earlier kernel without that interface the exclusion could be
> provided by a spinlock and a flag in nn. The flag would be set when
> the delayed_work is enabled and cleared when it is disabled.  The
> spinlock would be used to ensure nfsd4_force_end_grace() only queues the
> work while the flag is set.
> 
> [[ 
>  v2 - disable laundromat_work while _init is running as well as while
>  _exit is running.  Don't depend on ->nfsd_serv, test
>  ->client_tracking_ops instead.
> ]]

Do you want the patch change history to appear in the commit log?
Asking because that is not usual practice.


> Reported-by: Li Lingfeng <lilingfeng3@huawei.com>

Closes:
https://lore.kernel.org/linux-nfs/20250623030015.2353515-1-neil@brown.name/T/#t

Fixes: 7f5ef2e900d9 ("nfsd: add a v4_end_grace file to /proc/fs/nfsd")


> Cc: stable@vger.kernel.org
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/nfsd/netns.h     |  1 +
>  fs/nfsd/nfs4state.c | 27 ++++++++++++++++++++++++---
>  fs/nfsd/nfsctl.c    |  6 +++---
>  fs/nfsd/state.h     |  2 +-
>  4 files changed, 29 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/nfsd/netns.h b/fs/nfsd/netns.h
> index 3e2d0fde80a7..d83c68872c4c 100644
> --- a/fs/nfsd/netns.h
> +++ b/fs/nfsd/netns.h
> @@ -66,6 +66,7 @@ struct nfsd_net {
>  
>  	struct lock_manager nfsd4_manager;
>  	bool grace_ended;
> +	bool grace_end_forced;
>  	time64_t boot_time;
>  
>  	struct dentry *nfsd_client_dir;
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index d5694987f86f..857606035f94 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -84,7 +84,7 @@ static u64 current_sessionid = 1;
>  /* forward declarations */
>  static bool check_for_locks(struct nfs4_file *fp, struct nfs4_lockowner *lowner);
>  static void nfs4_free_ol_stateid(struct nfs4_stid *stid);
> -void nfsd4_end_grace(struct nfsd_net *nn);
> +static void nfsd4_end_grace(struct nfsd_net *nn);
>  static void _free_cpntf_state_locked(struct nfsd_net *nn, struct nfs4_cpntf_state *cps);
>  static void nfsd4_file_hash_remove(struct nfs4_file *fi);
>  static void deleg_reaper(struct nfsd_net *nn);
> @@ -6458,7 +6458,7 @@ nfsd4_renew(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  	return nfs_ok;
>  }
>  
> -void
> +static void
>  nfsd4_end_grace(struct nfsd_net *nn)
>  {
>  	/* do nothing if grace period already ended */
> @@ -6491,6 +6491,20 @@ nfsd4_end_grace(struct nfsd_net *nn)
>  	 */
>  }

Can you add a kdoc comment for nfsd4_force_end_grace() ?


> +bool
> +nfsd4_force_end_grace(struct nfsd_net *nn)
> +{
> +	if (!nn->client_tracking_ops)
> +		return false;
> +	/* laundromat_work must be initialised now, though it might be disabled */
> +	nn->grace_end_forced = true;
> +	/* This is a no-op after nfs4_state_shutdown_net() has called
> +	 * disable_delayed_work_sync()
> +	 */
> +	mod_delayed_work(laundry_wq, &nn->laundromat_work, 0);
> +	return true;
> +}
> +
>  /*
>   * If we've waited a lease period but there are still clients trying to
>   * reclaim, wait a little longer to give them a chance to finish.
> @@ -6500,6 +6514,8 @@ static bool clients_still_reclaiming(struct nfsd_net *nn)
>  	time64_t double_grace_period_end = nn->boot_time +
>  					   2 * nn->nfsd4_lease;
>  
> +	if (nn->grace_end_forced)
> +		return false;
>  	if (nn->track_reclaim_completes &&
>  			atomic_read(&nn->nr_reclaim_complete) ==
>  			nn->reclaim_str_hashtbl_size)
> @@ -8807,6 +8823,7 @@ static int nfs4_state_create_net(struct net *net)
>  	nn->unconf_name_tree = RB_ROOT;
>  	nn->boot_time = ktime_get_real_seconds();
>  	nn->grace_ended = false;
> +	nn->grace_end_forced = false;
>  	nn->nfsd4_manager.block_opens = true;
>  	INIT_LIST_HEAD(&nn->nfsd4_manager.list);
>  	INIT_LIST_HEAD(&nn->client_lru);
> @@ -8821,6 +8838,8 @@ static int nfs4_state_create_net(struct net *net)
>  	INIT_LIST_HEAD(&nn->blocked_locks_lru);
>  
>  	INIT_DELAYED_WORK(&nn->laundromat_work, laundromat_main);
> +	/* Make sure his cannot run until client tracking is initialised */
> +	disable_delayed_work(&nn->laundromat_work);
>  	INIT_WORK(&nn->nfsd_shrinker_work, nfsd4_state_shrinker_worker);
>  	get_net(net);
>  
> @@ -8887,6 +8906,8 @@ nfs4_state_start_net(struct net *net)
>  		return ret;
>  	locks_start_grace(net, &nn->nfsd4_manager);
>  	nfsd4_client_tracking_init(net);
> +	/* safe for laundromat to run now */
> +	enable_delayed_work(&nn->laundromat_work);
>  	if (nn->track_reclaim_completes && nn->reclaim_str_hashtbl_size == 0)
>  		goto skip_grace;
>  	printk(KERN_INFO "NFSD: starting %lld-second grace period (net %x)\n",
> @@ -8935,7 +8956,7 @@ nfs4_state_shutdown_net(struct net *net)
>  
>  	shrinker_free(nn->nfsd_client_shrinker);
>  	cancel_work_sync(&nn->nfsd_shrinker_work);
> -	cancel_delayed_work_sync(&nn->laundromat_work);
> +	disable_delayed_work_sync(&nn->laundromat_work);
>  	locks_end_grace(&nn->nfsd4_manager);
>  
>  	INIT_LIST_HEAD(&reaplist);
> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> index 3f3e9f6c4250..658f3f86a59f 100644
> --- a/fs/nfsd/nfsctl.c
> +++ b/fs/nfsd/nfsctl.c
> @@ -1082,10 +1082,10 @@ static ssize_t write_v4_end_grace(struct file *file, char *buf, size_t size)
>  		case 'Y':
>  		case 'y':
>  		case '1':
> -			if (!nn->nfsd_serv)
> -				return -EBUSY;
>  			trace_nfsd_end_grace(netns(file));
> -			nfsd4_end_grace(nn);
> +			if (!nfsd4_force_end_grace(nn))
> +				return -EBUSY;
> +
>  			break;
>  		default:
>  			return -EINVAL;
> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> index 1995bca158b8..05eabc69de40 100644
> --- a/fs/nfsd/state.h
> +++ b/fs/nfsd/state.h
> @@ -836,7 +836,7 @@ static inline void nfsd4_revoke_states(struct net *net, struct super_block *sb)
>  #endif
>  
>  /* grace period management */
> -void nfsd4_end_grace(struct nfsd_net *nn);
> +bool nfsd4_force_end_grace(struct nfsd_net *nn);
>  
>  /* nfs4recover operations */
>  extern int nfsd4_client_tracking_init(struct net *net);


-- 
Chuck Lever

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH RFT v2] nfsd: provide locking for v4_end_grace
  2025-07-02 13:56 ` Chuck Lever
@ 2025-07-02 21:22   ` NeilBrown
  2025-07-02 23:26     ` Chuck Lever
  2025-12-02 12:34     ` Li Lingfeng
  0 siblings, 2 replies; 8+ messages in thread
From: NeilBrown @ 2025-07-02 21:22 UTC (permalink / raw)
  To: Chuck Lever
  Cc: linux-nfs, Olga Kornievskaia, Dai Ngo, Tom Talpey, Jeff Layton,
	Li Lingfeng

On Wed, 02 Jul 2025, Chuck Lever wrote:
> Hi Neil, handful of nits below.
> 
> > 
> > [[ 
> >  v2 - disable laundromat_work while _init is running as well as while
> >  _exit is running.  Don't depend on ->nfsd_serv, test
> >  ->client_tracking_ops instead.
> > ]]
> 
> Do you want the patch change history to appear in the commit log?
> Asking because that is not usual practice.

Not really.  I'll send something new, likely tomorrow morning.
What do you think is the best way to handle backporting?  Should I
submit the version that doesn't use disable_delayed_work(), then add a
patch which changes to use that instead of a flag ?

Thanks,
NeilBrown

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH RFT v2] nfsd: provide locking for v4_end_grace
  2025-07-02 21:22   ` NeilBrown
@ 2025-07-02 23:26     ` Chuck Lever
  2025-12-02 12:34     ` Li Lingfeng
  1 sibling, 0 replies; 8+ messages in thread
From: Chuck Lever @ 2025-07-02 23:26 UTC (permalink / raw)
  To: NeilBrown
  Cc: linux-nfs, Olga Kornievskaia, Dai Ngo, Tom Talpey, Jeff Layton,
	Li Lingfeng

On 7/2/25 5:22 PM, NeilBrown wrote:
> On Wed, 02 Jul 2025, Chuck Lever wrote:
>> Hi Neil, handful of nits below.
>>
>>>
>>> [[ 
>>>  v2 - disable laundromat_work while _init is running as well as while
>>>  _exit is running.  Don't depend on ->nfsd_serv, test
>>>  ->client_tracking_ops instead.
>>> ]]
>>
>> Do you want the patch change history to appear in the commit log?
>> Asking because that is not usual practice.
> 
> Not really.  I'll send something new, likely tomorrow morning.
> What do you think is the best way to handle backporting?

The best practice is to have upstream patches that can apply cleanly to
LTS kernels.

- One patch that can be backported for the fix, one or more for
  clean-ups that apply only to master;

- Identify pre-requisites that need to be backported first to make
  your fix apply cleanly; or

- Handcraft the fix for each LTS kernel


> Should I
> submit the version that doesn't use disable_delayed_work(), then add a
> patch which changes to use that instead of a flag ?

Post an RFC here, let's see what you're thinking.


-- 
Chuck Lever

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH RFT v2] nfsd: provide locking for v4_end_grace
  2025-07-02 21:22   ` NeilBrown
  2025-07-02 23:26     ` Chuck Lever
@ 2025-12-02 12:34     ` Li Lingfeng
  1 sibling, 0 replies; 8+ messages in thread
From: Li Lingfeng @ 2025-12-02 12:34 UTC (permalink / raw)
  To: NeilBrown, Chuck Lever
  Cc: linux-nfs, Olga Kornievskaia, Dai Ngo, Tom Talpey, Jeff Layton,
	yangerkun, zhangjian (CG)

Hi Neil,

在 2025/7/3 5:22, NeilBrown 写道:
> On Wed, 02 Jul 2025, Chuck Lever wrote:
>> Hi Neil, handful of nits below.
>>
>>> [[
>>>   v2 - disable laundromat_work while _init is running as well as while
>>>   _exit is running.  Don't depend on ->nfsd_serv, test
>>>   ->client_tracking_ops instead.
>>> ]]
>> Do you want the patch change history to appear in the commit log?
>> Asking because that is not usual practice.
> Not really.  I'll send something new, likely tomorrow morning.
> What do you think is the best way to handle backporting?  Should I
> submit the version that doesn't use disable_delayed_work(), then add a
> patch which changes to use that instead of a flag ?
>
> Thanks,
> NeilBrown
It seems that this issue has not been fixed in the mainline yet. I was
able to reproduce it again based on e7c375b18160:
[  508.295384][ T1056] BUG: KASAN: slab-use-after-free in 
nfs4_release_reclaim+0x346/0x360
[  508.298229][ T1056] Read of size 8 at addr ffff888400718a00 by task 
bash/1056
[  508.300679][ T1056]
[  508.301368][ T1056] CPU: 3 UID: 0 PID: 1056 Comm: bash Not tainted 
6.18.0-rc6+ #166 PREEMPT(none)
[  508.301380][ T1056] Hardware name: QEMU Standard PC (i440FX + PIIX, 
1996), BIOS 1.16.3-2.fc40 04/01/2014
[  508.301386][ T1056] Call Trace:
[  508.301393][ T1056]  <TASK>
[  508.301401][ T1056]  dump_stack_lvl+0x4b/0x70
[  508.301417][ T1056] print_address_description.constprop.0+0x88/0x320
[  508.301430][ T1056]  ? nfs4_release_reclaim+0x346/0x360
[  508.301439][ T1056]  print_report+0x106/0x1f4
[  508.301448][ T1056]  ? nfs4_release_reclaim+0x346/0x360
[  508.301457][ T1056]  ? nfs4_release_reclaim+0x346/0x360
[  508.301466][ T1056]  kasan_report+0xb9/0xf0
[  508.301478][ T1056]  ? nfs4_release_reclaim+0x346/0x360
[  508.301489][ T1056]  nfs4_release_reclaim+0x346/0x360
[  508.301501][ T1056]  nfsd4_cld_grace_done+0x171/0x198
[  508.301514][ T1056]  nfsd4_end_grace+0x160/0x176
[  508.301523][ T1056]  write_v4_end_grace+0x165/0x390
[  508.301534][ T1056]  ? __pfx_write_v4_end_grace+0x10/0x10
[  508.301542][ T1056]  nfsctl_transaction_write+0xd1/0x150
[  508.301551][ T1056]  vfs_write+0x1d3/0xcc0
[  508.301563][ T1056]  ? __pfx___handle_mm_fault+0x10/0x10
[  508.301571][ T1056]  ? lock_vma_under_rcu+0x2ac/0x640
[  508.301584][ T1056]  ? __pfx_vfs_write+0x10/0x10
[  508.301595][ T1056]  ? count_memcg_events+0x257/0x400
[  508.301606][ T1056]  ? fdget_pos+0x1cf/0x4c0
[  508.301617][ T1056]  ksys_write+0xf3/0x1c0
[  508.301626][ T1056]  ? __pfx_ksys_write+0x10/0x10
[  508.301636][ T1056]  ? do_user_addr_fault+0x830/0xd70
[  508.301648][ T1056]  do_syscall_64+0x61/0x9d0
[  508.301662][ T1056]  entry_SYSCALL_64_after_hwframe+0x76/0x7e
[  508.301672][ T1056] RIP: 0033:0x7f14257018b7
[  508.301682][ T1056] Code: 0f 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff 
eb b7 0f 1f 00 f3 0f 1e fa 64 8b 04 25 18 00 00 00 85 c0 75 10 b8 01 00 
00 00 0f 05 <48> 3d 00 f0 ff ff 77 514
[  508.301691][ T1056] RSP: 002b:00007ffd28c8b1a8 EFLAGS: 00000246 
ORIG_RAX: 0000000000000001
[  508.301703][ T1056] RAX: ffffffffffffffda RBX: 0000000000000002 RCX: 
00007f14257018b7
[  508.301709][ T1056] RDX: 0000000000000002 RSI: 00005569b744ced0 RDI: 
0000000000000001
[  508.301715][ T1056] RBP: 00005569b744ced0 R08: 0000000000000000 R09: 
00007f14257b64e0
[  508.301720][ T1056] R10: 00007f14257b63e0 R11: 0000000000000246 R12: 
0000000000000002
[  508.301725][ T1056] R13: 00007f14257fb5a0 R14: 0000000000000002 R15: 
00007f14257fb7a0

Do you have plans to submit an updated patch?

Thanks,
Lingfeng


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2025-12-02 12:34 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-07-01 10:43 [PATCH RFT v2] nfsd: provide locking for v4_end_grace NeilBrown
2025-07-02  7:03 ` Li Lingfeng
2025-07-02  9:38   ` NeilBrown
2025-07-02 12:48     ` Li Lingfeng
2025-07-02 13:56 ` Chuck Lever
2025-07-02 21:22   ` NeilBrown
2025-07-02 23:26     ` Chuck Lever
2025-12-02 12:34     ` Li Lingfeng

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox