grace: only add lock_manager to grace_list if it's not already there

Vasily, would this patch fix the panic you're seeing? We may still need to do something saner here, but this seems like it should at least prevent a double list_add.

Quoted 3 lines · Jeff Layton

Vasily, would this patch fix the panic you're seeing? We may still need to do something saner here, but this seems like it should at least prevent a double list_add.

Jeff, I think your patch is wrong.

Double list_add is not a real problem, it is just a marker of its presence. It's great that such marker exist, it allows to detect the problems.

Yes, your patch prevent a double list_add and fix the reported panic, however it does not fix the root of problems, and it still can cause another troubles.

I see 2 separate problems here:

1) when nfsd is started in new net namespace it enables delayed_work grace_period_end and adds lockd_manager into list of this net namespace.
However nobody revert these actions on stop of this nfsd.

It can lead to double list_add if nfsd is started again.
Also we can remove net namespace, then not disarmed delayed_work can access freed memory.

It was broken by Commit efda760fe95ea ("lockd: fix lockd shutdown race"):
you cannot move cancel_delayed_work_sync() and lock_end_grace() into lockd(). lockd can be not executed at all (if nfsd was started in init_net or in any other net namespace) and even if it was executed -- it calls these functions with wrong net:
lockd uses hardcoded init_net and does not know which net was provided to lockd_up on start.

My patch "[PATCH] lockd: fix lockd shutdown race with signal" fixes this problem.

2) restart_grace() works with init_net only.
It can call set_grace_period() for init_net when it was not required (i.e. when nfsd in init_net was stopped) and cause double list_add on its start.

However main problem here is that it does nothing for any other net namespaces.
This part of lockd was not namespace-ified, and I would like to clarify how it's better to complete this task.

Quoted 9 lines · Vasily Averin

Quoted 3 lines · Jeff Layton

Vasily, would this patch fix the panic you're seeing? We may still need to do something saner here, but this seems like it should at least prevent a double list_add.

Jeff, I think your patch is wrong.

Double list_add is not a real problem, it is just a marker of its presence. It's great that such marker exist, it allows to detect the problems.

Quoted 6 lines

2) restart_grace() works with init_net only.
It can call set_grace_period() for init_net when it was not required (i.e. when nfsd in init_net was stopped) and cause double list_add on its start.

However main problem here is that it does nothing for any other net namespaces.
This part of lockd was not namespace-ified, and I would like to clarify how it's better to complete this task.

I'm not sure. The original idea was that the grace period is global to the host (hypervisor), so as long as a server in any network namespace needs a grace period, normal locking should be blocked across all namespaces. (This is really only necessarily if we know that a filesystem is exported from all namespaces; but since we don't keep track of that, we assume the worst.)

The signal interface to lockd I think of as a legacy interface. As you say it might be risky to just rip it out completely. But I'd be fine with it being limited to legacy use cases. So if it doesn't play well with network namespaces, that's OK (as long as it doesn't crash).

Quoted 10 lines · J. Bruce Fields

Quoted 9 lines · Vasily Averin

Quoted 3 lines · Jeff Layton

Vasily, would this patch fix the panic you're seeing? We may still need to do something saner here, but this seems like it should at least prevent a double list_add.

Jeff, I think your patch is wrong.

Double list_add is not a real problem, it is just a marker of its presence. It's great that such marker exist, it allows to detect the problems.

Jeff, what do you about about following hunk ?

fs/nfs_common/grace.c +5 -1

@@ -30,7 +30,11 @@ locks_start_grace(struct net *net, struct lock_manager *lm)
        struct list_head *grace_list = net_generic(net, grace_net_id);
 
        spin_lock(&grace_lock);
-       list_add(&lm->list, grace_list);
+       if (list_empty(&lm->list))
+               list_add(&lm->list, grace_list);
+       else
+               WARN(1, "double list_add attempt detected in %s %p\n",
+                    (net == &init_net) ? "init_net" : "net", net);
        spin_unlock(&grace_lock);
 }
 EXPORT_SYMBOL_GPL(locks_start_grace);

It allows to avoid list corruption and panic
but will report about detected problem

[  344.722040] double list_add attempt detected in init_net ffffffffa4fd9800
[  344.722108] ------------[ cut here ]------------
[  344.722142] WARNING: CPU: 3 PID: 1505 at fs/nfs_common/grace.c:37 locks_start_grace+0xa2/0xb0 [grace]

I think by same way we can also detect and disarm lost delayed_work on stop of network namespace. (in lockd_exit_net)

restart_grace() uses hardcoded init_net.
It can cause to "list_add double add" in following scenario:

1) nfsd and lockd was started in several net namespaces
2) nfsd in init_net was stopped (lockd was not stopped because it have users from another net namespaces)
3) lockd got signal, called restart_grace() -> set_grace_period() and enabled lock_manager in hardcoded init_net.
4) nfsd in init_net is started again,
its lockd_up() calls set_grace_period() and tries to add lock_manager into init_net 2nd time.

Jeff Layton suggest:
"Make it safe to call locks_start_grace multiple times on the same lock_manager. If it's already on the global grace_list, then don't try to add it again.

With this change, we also need to ensure that the nfsd4 lock manager initializes the list before we call locks_start_grace. While we're at it, move the rest of the nfsd_net initialization into nfs4_state_create_net. I see no reason to have it spread over two functions like it is today."

Suggested patch was updated to generate warning in described situation.

Suggested-by Jeff Layton <[email protected]>

Signed-off-by Vasily Averin <[email protected]>

---
 fs/nfs_common/grace.c | 6 +++++-
 fs/nfsd/nfs4state.c   | 7 ++++---
 2 files changed, 9 insertions(+), 4 deletions(-)

fs/nfs_common/grace.c +0 -0

index bd3e2d3..5be08f0 100644
@@ -30,7 +30,11 @@ locks_start_grace(struct net *net, struct lock_manager *lm)
 	struct list_head *grace_list = net_generic(net, grace_net_id);

fs/nfs_common/grace.c +0 -0

 EXPORT_SYMBOL_GPL(locks_start_grace);

fs/nfsd/nfs4state.c +4 -3

index 7345143..b29b5a1 100644
@@ -7103,6 +7103,10 @@ static int nfs4_state_create_net(struct net *net)
 		INIT_LIST_HEAD(&nn->sessionid_hashtbl[i]);
 	nn->conf_name_tree = RB_ROOT;
 	nn->unconf_name_tree = RB_ROOT;
+	nn->boot_time = get_seconds();
+	nn->grace_ended = false;
+	nn->nfsd4_manager.block_opens = true;
+	INIT_LIST_HEAD(&nn->nfsd4_manager.list);
 	INIT_LIST_HEAD(&nn->client_lru);
 	INIT_LIST_HEAD(&nn->close_lru);
 	INIT_LIST_HEAD(&nn->del_recall_lru);

@@ -7160,9 +7164,6 @@ nfs4_state_start_net(struct net *net)
 	ret = nfs4_state_create_net(net);
 	if (ret)
 		return ret;
-	nn->boot_time = get_seconds();
-	nn->grace_ended = false;
-	nn->nfsd4_manager.block_opens = true;
 	locks_start_grace(net, &nn->nfsd4_manager);
 	nfsd4_client_tracking_init(net);
 	printk(KERN_INFO "NFSD: starting %ld-second grace period (net %x)\n",

Quoted 4 lines · Vasily Averin

fs/nfs_common/grace.c +0 -0

@@ -30,7 +30,11 @@ locks_start_grace(struct net *net, struct lock_manager *lm)
 	struct list_head *grace_list = net_generic(net, grace_net_id);

fs/nfs_common/grace.c locks_start_grace(struct net *net, struct lock_manager *lm) · line 39

Quoted 10 lines

fs/nfs_common/grace.c +5 -1

 
 	spin_lock(&grace_lock);
-	list_add(&lm->list, grace_list);
+	if (list_empty(&lm->list))
+		list_add(&lm->list, grace_list);
+	else
+		WARN(1, "double list_add attempt detected in net %x %s\n",
+		     net->ns.inum, (net == &init_net) ? "(init_net)" : "");
 	spin_unlock(&grace_lock);
 }

I'm not sure that warning really means much.

It's not _really_ a bug to request that a new grace period start while it's already in one. In general, it's ok to request a new grace period while it's currently enforcing one. That should just have the effect of extending the existing grace period.

Quoted 18 lines · Jeff Layton

Quoted 4 lines · Vasily Averin

fs/nfs_common/grace.c +0 -0

@@ -30,7 +30,11 @@ locks_start_grace(struct net *net, struct lock_manager *lm)
 	struct list_head *grace_list = net_generic(net, grace_net_id);

fs/nfs_common/grace.c locks_start_grace(struct net *net, struct lock_manager *lm) · line 39

Quoted 10 lines

fs/nfs_common/grace.c +5 -1

 
 	spin_lock(&grace_lock);
-	list_add(&lm->list, grace_list);
+	if (list_empty(&lm->list))
+		list_add(&lm->list, grace_list);
+	else
+		WARN(1, "double list_add attempt detected in net %x %s\n",
+		     net->ns.inum, (net == &init_net) ? "(init_net)" : "");
 	spin_unlock(&grace_lock);
 }

I'm not sure that warning really means much.

Quoted 4 lines

It's not _really_ a bug to request that a new grace period start while it's already in one. In general, it's ok to request a new grace period while it's currently enforcing one. That should just have the effect of extending the existing grace period.

"double list_add" can happen in init_net when legacy signal in lockd was used. It should not happen during usual extending of existing grace period, because restart_grace() calls locks_end_grace() before set_grace_period() but it can race with start of lockd_up_net() in init_net.
I'm agree: we do not have any bugs in this scenario, all should work correctly.

However I would like to keep WARN to properly detect lost locks_end_grace()/ cancel_delayed_work().

If you worry about real false positive and do not worry about abstract future troubles in init_net, I can move WARN under (net != &init_net) check.

However I would like to keep this warning here.

On the other hand if you disagree and still believe that WARN is not required here I'm ready to agree with your original patch version.

Quoted 33 lines · Vasily Averin

Quoted 18 lines · Jeff Layton

Quoted 4 lines · Vasily Averin

fs/nfs_common/grace.c +0 -0

@@ -30,7 +30,11 @@ locks_start_grace(struct net *net, struct lock_manager *lm)
 	struct list_head *grace_list = net_generic(net, grace_net_id);

fs/nfs_common/grace.c locks_start_grace(struct net *net, struct lock_manager *lm) · line 39

Quoted 10 lines

fs/nfs_common/grace.c +5 -1

 
 	spin_lock(&grace_lock);
-	list_add(&lm->list, grace_list);
+	if (list_empty(&lm->list))
+		list_add(&lm->list, grace_list);
+	else
+		WARN(1, "double list_add attempt detected in net %x %s\n",
+		     net->ns.inum, (net == &init_net) ? "(init_net)" : "");
 	spin_unlock(&grace_lock);
 }

I'm not sure that warning really means much.

Quoted 4 lines

It's not _really_ a bug to request that a new grace period start while it's already in one. In general, it's ok to request a new grace period while it's currently enforcing one. That should just have the effect of extending the existing grace period.

"double list_add" can happen in init_net when legacy signal in lockd was used. It should not happen during usual extending of existing grace period, because restart_grace() calls locks_end_grace() before set_grace_period() but it can race with start of lockd_up_net() in init_net.
I'm agree: we do not have any bugs in this scenario, all should work correctly.

However I would like to keep WARN to properly detect lost locks_end_grace()/ cancel_delayed_work().

Quoted 7 lines

If you worry about real false positive and do not worry about abstract future troubles in init_net, I can move WARN under (net != &init_net) check.

However I would like to keep this warning here.

On the other hand if you disagree and still believe that WARN is not required here I'm ready to agree with your original patch version.

Fair enough. I don't feel strongly about it. I just have been doing some investigation lately into clustered grace period management, so it's a little on my mind. [1]

For now though, you're certainly correct that we'll never attempt to set the grace period while we're already in it. If we ever want to do more complex grace period handling in the kernel, we may need to drop that WARN, however.