| Message ID | 20171030182932.3282-1-jlayton@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 2017-10-30 21:29, Jeff Layton wrote: > 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. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Oct 31, 2017 at 10:31:35AM +0300, Vasily Averin wrote: > On 2017-10-30 21:29, Jeff Layton wrote: > > 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. 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). --b. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/nfs_common/grace.c b/fs/nfs_common/grace.c index 420d3a0ab258..874ff25e12ac 100644 --- a/fs/nfs_common/grace.c +++ b/fs/nfs_common/grace.c @@ -30,7 +30,8 @@ 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); spin_unlock(&grace_lock); } EXPORT_SYMBOL_GPL(locks_start_grace); diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index 0c04f81aa63b..4aa78bd6f0bb 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -6944,6 +6944,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); @@ -7001,9 +7005,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 %p)\n",