Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1583952 > unrolled thread

Re: [PATCH] proc/sysctl: prune stale dentries during unregistering

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] proc/sysctl: prune stale dentries during unregistering Konstantin Khlebnikov <khlebnikov@yandex-team.ru> - 2017-02-18 20:00 +0100
    Re: [PATCH] proc/sysctl: prune stale dentries during unregistering Al Viro <viro@ZenIV.linux.org.uk> - 2017-02-19 09:50 +0100
      [REVIEW][PATCH] proc/sysctl: Don't grab i_lock under sysctl_lock. ebiederm@xmission.com (Eric W. Biederman) - 2017-02-21 02:50 +0100
        Re: [REVIEW][PATCH] proc/sysctl: Don't grab i_lock under sysctl_lock. Konstantin Khlebnikov <khlebnikov@yandex-team.ru> - 2017-02-21 09:50 +0100
          Re: [REVIEW][PATCH] proc/sysctl: Don't grab i_lock under sysctl_lock. ebiederm@xmission.com (Eric W. Biederman) - 2017-02-21 20:40 +0100

#1583952 — Re: [PATCH] proc/sysctl: prune stale dentries during unregistering

This patch has locking problem. I've got lockdep splat under LTP.

[ 6633.115456] ======================================================
[ 6633.115502] [ INFO: possible circular locking dependency detected ]
[ 6633.115553] 4.9.10-debug+ #9 Tainted: G             L
[ 6633.115584] -------------------------------------------------------
[ 6633.115627] ksm02/284980 is trying to acquire lock:
[ 6633.115659]  (&sb->s_type->i_lock_key#4){+.+...}, at: [<ffffffff816bc1ce>] igrab+0x1e/0x80
[ 6633.115834] but task is already holding lock:
[ 6633.115882]  (sysctl_lock){+.+...}, at: [<ffffffff817e379b>] unregister_sysctl_table+0x6b/0x110
[ 6633.116026] which lock already depends on the new lock.
[ 6633.116026]
[ 6633.116080]
[ 6633.116080] the existing dependency chain (in reverse order) is:
[ 6633.116117]
-> #2 (sysctl_lock){+.+...}:
-> #1 (&(&dentry->d_lockref.lock)->rlock){+.+...}:
-> #0 (&sb->s_type->i_lock_key#4){+.+...}:

d_lock nests inside i_lock
sysctl_lock nests inside d_lock in d_compare

This patch adds i_lock nesting inside sysctl_lock.

On 10.02.2017 10:35, Konstantin Khlebnikov wrote:
> Currently unregistering sysctl table does not prune its dentries.
> Stale dentries could slowdown sysctl operations significantly.
>
> For example, command:
>
> # for i in {1..100000} ; do unshare -n -- sysctl -a &> /dev/null ; done
>
> creates a millions of stale denties around sysctls of loopback interface:
>
> # sysctl fs.dentry-state
> fs.dentry-state = 25812579  24724135        45      0       0       0
>
> All of them have matching names thus lookup have to scan though whole
> hash chain and call d_compare (proc_sys_compare) which checks them
> under system-wide spinlock (sysctl_lock).
>
> # time sysctl -a > /dev/null
> real    1m12.806s
> user    0m0.016s
> sys     1m12.400s
>
> Currently only memory reclaimer could remove this garbage.
> But without significant memory pressure this never happens.
>
> This patch collects sysctl inodes into list on sysctl table header and
> prunes all their dentries once that table unregisters.
>
> Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
> Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
> ---
>  fs/proc/inode.c        |    3 ++
>  fs/proc/internal.h     |    7 ++++--
>  fs/proc/proc_sysctl.c  |   59 +++++++++++++++++++++++++++++++++++-------------
>  include/linux/sysctl.h |    1 +
>  4 files changed, 51 insertions(+), 19 deletions(-)
>
> diff --git a/fs/proc/inode.c b/fs/proc/inode.c
> index 842a5ff5b85c..7ad9ed7958af 100644
> --- a/fs/proc/inode.c
> +++ b/fs/proc/inode.c
> @@ -43,10 +43,11 @@ static void proc_evict_inode(struct inode *inode)
>  	de = PDE(inode);
>  	if (de)
>  		pde_put(de);
> +
>  	head = PROC_I(inode)->sysctl;
>  	if (head) {
>  		RCU_INIT_POINTER(PROC_I(inode)->sysctl, NULL);
> -		sysctl_head_put(head);
> +		proc_sys_evict_inode(inode, head);
>  	}
>  }
>
> diff --git a/fs/proc/internal.h b/fs/proc/internal.h
> index 2de5194ba378..ed1d762160e6 100644
> --- a/fs/proc/internal.h
> +++ b/fs/proc/internal.h
> @@ -65,6 +65,7 @@ struct proc_inode {
>  	struct proc_dir_entry *pde;
>  	struct ctl_table_header *sysctl;
>  	struct ctl_table *sysctl_entry;
> +	struct list_head sysctl_inodes;
>  	const struct proc_ns_operations *ns_ops;
>  	struct inode vfs_inode;
>  };
> @@ -249,10 +250,12 @@ extern void proc_thread_self_init(void);
>   */
>  #ifdef CONFIG_PROC_SYSCTL
>  extern int proc_sys_init(void);
> -extern void sysctl_head_put(struct ctl_table_header *);
> +extern void proc_sys_evict_inode(struct inode *inode,
> +				 struct ctl_table_header *head);
>  #else
>  static inline void proc_sys_init(void) { }
> -static inline void sysctl_head_put(struct ctl_table_header *head) { }
> +static inline void proc_sys_evict_inode(struct  inode *inode,
> +					struct ctl_table_header *head) { }
>  #endif
>
>  /*
> diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
> index d4e37acd4821..8efb1e10b025 100644
> --- a/fs/proc/proc_sysctl.c
> +++ b/fs/proc/proc_sysctl.c
> @@ -190,6 +190,7 @@ static void init_header(struct ctl_table_header *head,
>  	head->set = set;
>  	head->parent = NULL;
>  	head->node = node;
> +	INIT_LIST_HEAD(&head->inodes);
>  	if (node) {
>  		struct ctl_table *entry;
>  		for (entry = table; entry->procname; entry++, node++)
> @@ -259,6 +260,29 @@ static void unuse_table(struct ctl_table_header *p)
>  			complete(p->unregistering);
>  }
>
> +/* called under sysctl_lock */
> +static void proc_sys_prune_dcache(struct ctl_table_header *head)
> +{
> +	struct inode *inode, *prev = NULL;
> +	struct proc_inode *ei;
> +
> +	list_for_each_entry(ei, &head->inodes, sysctl_inodes) {
> +		inode = igrab(&ei->vfs_inode);
> +		if (inode) {
> +			spin_unlock(&sysctl_lock);
> +			iput(prev);
> +			prev = inode;
> +			d_prune_aliases(inode);
> +			spin_lock(&sysctl_lock);
> +		}
> +	}
> +	if (prev) {
> +		spin_unlock(&sysctl_lock);
> +		iput(prev);
> +		spin_lock(&sysctl_lock);
> +	}
> +}
> +
>  /* called under sysctl_lock, will reacquire if has to wait */
>  static void start_unregistering(struct ctl_table_header *p)
>  {
> @@ -278,27 +302,17 @@ static void start_unregistering(struct ctl_table_header *p)
>  		p->unregistering = ERR_PTR(-EINVAL);
>  	}
>  	/*
> +	 * Prune dentries for unregistered sysctls: namespaced sysctls
> +	 * can have duplicate names and contaminate dcache very badly.
> +	 */
> +	proc_sys_prune_dcache(p);
> +	/*
>  	 * do not remove from the list until nobody holds it; walking the
>  	 * list in do_sysctl() relies on that.
>  	 */
>  	erase_header(p);
>  }
>
> -static void sysctl_head_get(struct ctl_table_header *head)
> -{
> -	spin_lock(&sysctl_lock);
> -	head->count++;
> -	spin_unlock(&sysctl_lock);
> -}
> -
> -void sysctl_head_put(struct ctl_table_header *head)
> -{
> -	spin_lock(&sysctl_lock);
> -	if (!--head->count)
> -		kfree_rcu(head, rcu);
> -	spin_unlock(&sysctl_lock);
> -}
> -
>  static struct ctl_table_header *sysctl_head_grab(struct ctl_table_header *head)
>  {
>  	BUG_ON(!head);
> @@ -440,11 +454,15 @@ static struct inode *proc_sys_make_inode(struct super_block *sb,
>
>  	inode->i_ino = get_next_ino();
>
> -	sysctl_head_get(head);
>  	ei = PROC_I(inode);
>  	ei->sysctl = head;
>  	ei->sysctl_entry = table;
>
> +	spin_lock(&sysctl_lock);
> +	list_add(&ei->sysctl_inodes, &head->inodes);
> +	head->count++;
> +	spin_unlock(&sysctl_lock);
> +
>  	inode->i_mtime = inode->i_atime = inode->i_ctime = current_time(inode);
>  	inode->i_mode = table->mode;
>  	if (!S_ISDIR(table->mode)) {
> @@ -466,6 +484,15 @@ static struct inode *proc_sys_make_inode(struct super_block *sb,
>  	return inode;
>  }
>
> +void proc_sys_evict_inode(struct inode *inode, struct ctl_table_header *head)
> +{
> +	spin_lock(&sysctl_lock);
> +	list_del(&PROC_I(inode)->sysctl_inodes);
> +	if (!--head->count)
> +		kfree_rcu(head, rcu);
> +	spin_unlock(&sysctl_lock);
> +}
> +
>  static struct ctl_table_header *grab_header(struct inode *inode)
>  {
>  	struct ctl_table_header *head = PROC_I(inode)->sysctl;
> diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
> index adf4e51cf597..b7e82049fec7 100644
> --- a/include/linux/sysctl.h
> +++ b/include/linux/sysctl.h
> @@ -143,6 +143,7 @@ struct ctl_table_header
>  	struct ctl_table_set *set;
>  	struct ctl_dir *parent;
>  	struct ctl_node *node;
> +	struct list_head inodes; /* head for proc_inode->sysctl_inodes */
>  };
>
>  struct ctl_dir {
>

[toc] | [next] | [standalone]

#1584079

On Sat, Feb 18, 2017 at 09:55:28PM +0300, Konstantin Khlebnikov wrote:
> This patch has locking problem. I've got lockdep splat under LTP.
> 
> d_lock nests inside i_lock
> sysctl_lock nests inside d_lock in d_compare
> 
> This patch adds i_lock nesting inside sysctl_lock.

Once ->unregistering is set, you can drop sysctl_lock just fine.  So I'd
try something like this - use rcu_read_lock() in proc_sys_prune_dcache(),
drop sysctl_lock() before it and regain after.  Make sure that no inodes
are added to the list ones ->unregistering has been set and use RCU list
primitives for modifying the inode list, with sysctl_lock still used to
serialize its modifications.

Freeing struct inode is RCU-delayed (see proc_destroy_inode()), so doing
igrab() is safe there.  Since we don't drop inode reference until after we'd
passed beyond it in the list, list_for_each_entry_rcu() should be fine,
AFAICS.  Below is a completely untested modification of your patch along
those lines:

diff --git a/fs/proc/inode.c b/fs/proc/inode.c
index 842a5ff5b85c..7ad9ed7958af 100644
--- a/fs/proc/inode.c
+++ b/fs/proc/inode.c
@@ -43,10 +43,11 @@ static void proc_evict_inode(struct inode *inode)
 	de = PDE(inode);
 	if (de)
 		pde_put(de);
+
 	head = PROC_I(inode)->sysctl;
 	if (head) {
 		RCU_INIT_POINTER(PROC_I(inode)->sysctl, NULL);
-		sysctl_head_put(head);
+		proc_sys_evict_inode(inode, head);
 	}
 }
 
diff --git a/fs/proc/internal.h b/fs/proc/internal.h
index 2de5194ba378..ed1d762160e6 100644
--- a/fs/proc/internal.h
+++ b/fs/proc/internal.h
@@ -65,6 +65,7 @@ struct proc_inode {
 	struct proc_dir_entry *pde;
 	struct ctl_table_header *sysctl;
 	struct ctl_table *sysctl_entry;
+	struct list_head sysctl_inodes;
 	const struct proc_ns_operations *ns_ops;
 	struct inode vfs_inode;
 };
@@ -249,10 +250,12 @@ extern void proc_thread_self_init(void);
  */
 #ifdef CONFIG_PROC_SYSCTL
 extern int proc_sys_init(void);
-extern void sysctl_head_put(struct ctl_table_header *);
+extern void proc_sys_evict_inode(struct inode *inode,
+				 struct ctl_table_header *head);
 #else
 static inline void proc_sys_init(void) { }
-static inline void sysctl_head_put(struct ctl_table_header *head) { }
+static inline void proc_sys_evict_inode(struct  inode *inode,
+					struct ctl_table_header *head) { }
 #endif
 
 /*
diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
index 55313d994895..6477c4a2dc6c 100644
--- a/fs/proc/proc_sysctl.c
+++ b/fs/proc/proc_sysctl.c
@@ -190,6 +190,7 @@ static void init_header(struct ctl_table_header *head,
 	head->set = set;
 	head->parent = NULL;
 	head->node = node;
+	INIT_LIST_HEAD(&head->inodes);
 	if (node) {
 		struct ctl_table *entry;
 		for (entry = table; entry->procname; entry++, node++)
@@ -259,6 +260,26 @@ static void unuse_table(struct ctl_table_header *p)
 			complete(p->unregistering);
 }
 
+static void proc_sys_prune_dcache(struct ctl_table_header *head)
+{
+	struct inode *inode, *prev = NULL;
+	struct proc_inode *ei;
+
+	rcu_read_lock();
+	list_for_each_entry_rcu(ei, &head->inodes, sysctl_inodes) {
+		inode = igrab(&ei->vfs_inode);
+		if (inode) {
+			rcu_read_unlock();
+			iput(prev);
+			prev = inode;
+			d_prune_aliases(inode);
+			rcu_read_lock();
+		}
+	}
+	rcu_read_unlock();
+	iput(prev);
+}
+
 /* called under sysctl_lock, will reacquire if has to wait */
 static void start_unregistering(struct ctl_table_header *p)
 {
@@ -272,31 +293,22 @@ static void start_unregistering(struct ctl_table_header *p)
 		p->unregistering = &wait;
 		spin_unlock(&sysctl_lock);
 		wait_for_completion(&wait);
-		spin_lock(&sysctl_lock);
 	} else {
 		/* anything non-NULL; we'll never dereference it */
 		p->unregistering = ERR_PTR(-EINVAL);
+		spin_unlock(&sysctl_lock);
 	}
 	/*
+	 * Prune dentries for unregistered sysctls: namespaced sysctls
+	 * can have duplicate names and contaminate dcache very badly.
+	 */
+	proc_sys_prune_dcache(p);
+	/*
 	 * do not remove from the list until nobody holds it; walking the
 	 * list in do_sysctl() relies on that.
 	 */
-	erase_header(p);
-}
-
-static void sysctl_head_get(struct ctl_table_header *head)
-{
-	spin_lock(&sysctl_lock);
-	head->count++;
-	spin_unlock(&sysctl_lock);
-}
-
-void sysctl_head_put(struct ctl_table_header *head)
-{
 	spin_lock(&sysctl_lock);
-	if (!--head->count)
-		kfree_rcu(head, rcu);
-	spin_unlock(&sysctl_lock);
+	erase_header(p);
 }
 
 static struct ctl_table_header *sysctl_head_grab(struct ctl_table_header *head)
@@ -440,10 +452,20 @@ static struct inode *proc_sys_make_inode(struct super_block *sb,
 
 	inode->i_ino = get_next_ino();
 
-	sysctl_head_get(head);
 	ei = PROC_I(inode);
+
+	spin_lock(&sysctl_lock);
+	if (unlikely(head->unregistering)) {
+		spin_unlock(&sysctl_lock);
+		iput(inode);
+		inode = NULL;
+		goto out;
+	}
 	ei->sysctl = head;
 	ei->sysctl_entry = table;
+	list_add_rcu(&ei->sysctl_inodes, &head->inodes);
+	head->count++;
+	spin_unlock(&sysctl_lock);
 
 	inode->i_mtime = inode->i_atime = inode->i_ctime = current_time(inode);
 	inode->i_mode = table->mode;
@@ -466,6 +488,15 @@ static struct inode *proc_sys_make_inode(struct super_block *sb,
 	return inode;
 }
 
+void proc_sys_evict_inode(struct inode *inode, struct ctl_table_header *head)
+{
+	spin_lock(&sysctl_lock);
+	list_del_rcu(&PROC_I(inode)->sysctl_inodes);
+	if (!--head->count)
+		kfree_rcu(head, rcu);
+	spin_unlock(&sysctl_lock);
+}
+
 static struct ctl_table_header *grab_header(struct inode *inode)
 {
 	struct ctl_table_header *head = PROC_I(inode)->sysctl;
diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
index adf4e51cf597..b7e82049fec7 100644
--- a/include/linux/sysctl.h
+++ b/include/linux/sysctl.h
@@ -143,6 +143,7 @@ struct ctl_table_header
 	struct ctl_table_set *set;
 	struct ctl_dir *parent;
 	struct ctl_node *node;
+	struct list_head inodes; /* head for proc_inode->sysctl_inodes */
 };
 
 struct ctl_dir {

[toc] | [prev] | [next] | [standalone]

#1584991 — [REVIEW][PATCH] proc/sysctl: Don't grab i_lock under sysctl_lock.

Konstantin Khlebnikov <khlebnikov@yandex-team.ru> writes:
> This patch has locking problem. I've got lockdep splat under LTP.
>
> [ 6633.115456] ======================================================
> [ 6633.115502] [ INFO: possible circular locking dependency detected ]
> [ 6633.115553] 4.9.10-debug+ #9 Tainted: G             L
> [ 6633.115584] -------------------------------------------------------
> [ 6633.115627] ksm02/284980 is trying to acquire lock:
> [ 6633.115659]  (&sb->s_type->i_lock_key#4){+.+...}, at: [<ffffffff816bc1ce>] igrab+0x1e/0x80
> [ 6633.115834] but task is already holding lock:
> [ 6633.115882]  (sysctl_lock){+.+...}, at: [<ffffffff817e379b>] unregister_sysctl_table+0x6b/0x110
> [ 6633.116026] which lock already depends on the new lock.
> [ 6633.116026]
> [ 6633.116080]
> [ 6633.116080] the existing dependency chain (in reverse order) is:
> [ 6633.116117]
> -> #2 (sysctl_lock){+.+...}:
> -> #1 (&(&dentry->d_lockref.lock)->rlock){+.+...}:
> -> #0 (&sb->s_type->i_lock_key#4){+.+...}:
>
> d_lock nests inside i_lock
> sysctl_lock nests inside d_lock in d_compare
>
> This patch adds i_lock nesting inside sysctl_lock.

Al Viro <viro@ZenIV.linux.org.uk> replied:
> Once ->unregistering is set, you can drop sysctl_lock just fine.  So I'd
> try something like this - use rcu_read_lock() in proc_sys_prune_dcache(),
> drop sysctl_lock() before it and regain after.  Make sure that no inodes
> are added to the list ones ->unregistering has been set and use RCU list
> primitives for modifying the inode list, with sysctl_lock still used to
> serialize its modifications.
>
> Freeing struct inode is RCU-delayed (see proc_destroy_inode()), so doing
> igrab() is safe there.  Since we don't drop inode reference until after we'd
> passed beyond it in the list, list_for_each_entry_rcu() should be fine.

I agree with Al Viro's analsysis of the situtation.

Fixes: 802e348c6b77 ("proc/sysctl: prune stale dentries during unregistering")
Reported-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
Suggested-by: Al Viro <viro@ZenIV.linux.org.uk>
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---

This is my cleaned up version of Al Viro's proposed fix.
I have tested it and the lockdep warnings go away, and
I have fixed a few trivial to ensure things work as intended.

Unless someone sees a problem I am going to add this fix to my tree and
then send a pull request to Linus.

 fs/proc/proc_sysctl.c | 31 ++++++++++++++++++-------------
 1 file changed, 18 insertions(+), 13 deletions(-)

diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
index 8efb1e10b025..3e64c6502dc8 100644
--- a/fs/proc/proc_sysctl.c
+++ b/fs/proc/proc_sysctl.c
@@ -266,21 +266,19 @@ static void proc_sys_prune_dcache(struct ctl_table_header *head)
 	struct inode *inode, *prev = NULL;
 	struct proc_inode *ei;
 
-	list_for_each_entry(ei, &head->inodes, sysctl_inodes) {
+	rcu_read_lock();
+	list_for_each_entry_rcu(ei, &head->inodes, sysctl_inodes) {
 		inode = igrab(&ei->vfs_inode);
 		if (inode) {
-			spin_unlock(&sysctl_lock);
+			rcu_read_unlock();
 			iput(prev);
 			prev = inode;
 			d_prune_aliases(inode);
-			spin_lock(&sysctl_lock);
+			rcu_read_lock();
 		}
 	}
-	if (prev) {
-		spin_unlock(&sysctl_lock);
-		iput(prev);
-		spin_lock(&sysctl_lock);
-	}
+	rcu_read_unlock();
+	iput(prev);
 }
 
 /* called under sysctl_lock, will reacquire if has to wait */
@@ -296,10 +294,10 @@ static void start_unregistering(struct ctl_table_header *p)
 		p->unregistering = &wait;
 		spin_unlock(&sysctl_lock);
 		wait_for_completion(&wait);
-		spin_lock(&sysctl_lock);
 	} else {
 		/* anything non-NULL; we'll never dereference it */
 		p->unregistering = ERR_PTR(-EINVAL);
+		spin_unlock(&sysctl_lock);
 	}
 	/*
 	 * Prune dentries for unregistered sysctls: namespaced sysctls
@@ -310,6 +308,7 @@ static void start_unregistering(struct ctl_table_header *p)
 	 * do not remove from the list until nobody holds it; walking the
 	 * list in do_sysctl() relies on that.
 	 */
+	spin_lock(&sysctl_lock);
 	erase_header(p);
 }
 
@@ -455,11 +454,17 @@ static struct inode *proc_sys_make_inode(struct super_block *sb,
 	inode->i_ino = get_next_ino();
 
 	ei = PROC_I(inode);
-	ei->sysctl = head;
-	ei->sysctl_entry = table;
 
 	spin_lock(&sysctl_lock);
-	list_add(&ei->sysctl_inodes, &head->inodes);
+	if (unlikely(head->unregistering)) {
+		spin_unlock(&sysctl_lock);
+		iput(inode);
+		inode = NULL;
+		goto out;
+	}
+	ei->sysctl = head;
+	ei->sysctl_entry = table;
+	list_add_rcu(&ei->sysctl_inodes, &head->inodes);
 	head->count++;
 	spin_unlock(&sysctl_lock);
 
@@ -487,7 +492,7 @@ static struct inode *proc_sys_make_inode(struct super_block *sb,
 void proc_sys_evict_inode(struct inode *inode, struct ctl_table_header *head)
 {
 	spin_lock(&sysctl_lock);
-	list_del(&PROC_I(inode)->sysctl_inodes);
+	list_del_rcu(&PROC_I(inode)->sysctl_inodes);
 	if (!--head->count)
 		kfree_rcu(head, rcu);
 	spin_unlock(&sysctl_lock);
-- 
2.10.1

[toc] | [prev] | [next] | [standalone]

#1585133 — Re: [REVIEW][PATCH] proc/sysctl: Don't grab i_lock under sysctl_lock.

On 21.02.2017 04:41, Eric W. Biederman wrote:
>
> Konstantin Khlebnikov <khlebnikov@yandex-team.ru> writes:
>> This patch has locking problem. I've got lockdep splat under LTP.
>>
>> [ 6633.115456] ======================================================
>> [ 6633.115502] [ INFO: possible circular locking dependency detected ]
>> [ 6633.115553] 4.9.10-debug+ #9 Tainted: G             L
>> [ 6633.115584] -------------------------------------------------------
>> [ 6633.115627] ksm02/284980 is trying to acquire lock:
>> [ 6633.115659]  (&sb->s_type->i_lock_key#4){+.+...}, at: [<ffffffff816bc1ce>] igrab+0x1e/0x80
>> [ 6633.115834] but task is already holding lock:
>> [ 6633.115882]  (sysctl_lock){+.+...}, at: [<ffffffff817e379b>] unregister_sysctl_table+0x6b/0x110
>> [ 6633.116026] which lock already depends on the new lock.
>> [ 6633.116026]
>> [ 6633.116080]
>> [ 6633.116080] the existing dependency chain (in reverse order) is:
>> [ 6633.116117]
>> -> #2 (sysctl_lock){+.+...}:
>> -> #1 (&(&dentry->d_lockref.lock)->rlock){+.+...}:
>> -> #0 (&sb->s_type->i_lock_key#4){+.+...}:
>>
>> d_lock nests inside i_lock
>> sysctl_lock nests inside d_lock in d_compare
>>
>> This patch adds i_lock nesting inside sysctl_lock.
>
> Al Viro <viro@ZenIV.linux.org.uk> replied:
>> Once ->unregistering is set, you can drop sysctl_lock just fine.  So I'd
>> try something like this - use rcu_read_lock() in proc_sys_prune_dcache(),
>> drop sysctl_lock() before it and regain after.  Make sure that no inodes
>> are added to the list ones ->unregistering has been set and use RCU list
>> primitives for modifying the inode list, with sysctl_lock still used to
>> serialize its modifications.
>>
>> Freeing struct inode is RCU-delayed (see proc_destroy_inode()), so doing
>> igrab() is safe there.  Since we don't drop inode reference until after we'd
>> passed beyond it in the list, list_for_each_entry_rcu() should be fine.
>
> I agree with Al Viro's analsysis of the situtation.
>
> Fixes: 802e348c6b77 ("proc/sysctl: prune stale dentries during unregistering")
> Reported-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
> Suggested-by: Al Viro <viro@ZenIV.linux.org.uk>
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
> ---
>
> This is my cleaned up version of Al Viro's proposed fix.
> I have tested it and the lockdep warnings go away, and
> I have fixed a few trivial to ensure things work as intended.
>
> Unless someone sees a problem I am going to add this fix to my tree and
> then send a pull request to Linus.

I've tested the same patch and found no problems.

Except proc_sys_prune_dcache() is no longer called under sysctl_lock like says comment above it.

>
>  fs/proc/proc_sysctl.c | 31 ++++++++++++++++++-------------
>  1 file changed, 18 insertions(+), 13 deletions(-)
>
> diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
> index 8efb1e10b025..3e64c6502dc8 100644
> --- a/fs/proc/proc_sysctl.c
> +++ b/fs/proc/proc_sysctl.c
> @@ -266,21 +266,19 @@ static void proc_sys_prune_dcache(struct ctl_table_header *head)
>  	struct inode *inode, *prev = NULL;
>  	struct proc_inode *ei;
>
> -	list_for_each_entry(ei, &head->inodes, sysctl_inodes) {
> +	rcu_read_lock();
> +	list_for_each_entry_rcu(ei, &head->inodes, sysctl_inodes) {
>  		inode = igrab(&ei->vfs_inode);
>  		if (inode) {
> -			spin_unlock(&sysctl_lock);
> +			rcu_read_unlock();
>  			iput(prev);
>  			prev = inode;
>  			d_prune_aliases(inode);
> -			spin_lock(&sysctl_lock);
> +			rcu_read_lock();
>  		}
>  	}
> -	if (prev) {
> -		spin_unlock(&sysctl_lock);
> -		iput(prev);
> -		spin_lock(&sysctl_lock);
> -	}
> +	rcu_read_unlock();
> +	iput(prev);
>  }
>
>  /* called under sysctl_lock, will reacquire if has to wait */
> @@ -296,10 +294,10 @@ static void start_unregistering(struct ctl_table_header *p)
>  		p->unregistering = &wait;
>  		spin_unlock(&sysctl_lock);
>  		wait_for_completion(&wait);
> -		spin_lock(&sysctl_lock);
>  	} else {
>  		/* anything non-NULL; we'll never dereference it */
>  		p->unregistering = ERR_PTR(-EINVAL);
> +		spin_unlock(&sysctl_lock);
>  	}
>  	/*
>  	 * Prune dentries for unregistered sysctls: namespaced sysctls
> @@ -310,6 +308,7 @@ static void start_unregistering(struct ctl_table_header *p)
>  	 * do not remove from the list until nobody holds it; walking the
>  	 * list in do_sysctl() relies on that.
>  	 */
> +	spin_lock(&sysctl_lock);
>  	erase_header(p);
>  }
>
> @@ -455,11 +454,17 @@ static struct inode *proc_sys_make_inode(struct super_block *sb,
>  	inode->i_ino = get_next_ino();
>
>  	ei = PROC_I(inode);
> -	ei->sysctl = head;
> -	ei->sysctl_entry = table;
>
>  	spin_lock(&sysctl_lock);
> -	list_add(&ei->sysctl_inodes, &head->inodes);
> +	if (unlikely(head->unregistering)) {
> +		spin_unlock(&sysctl_lock);
> +		iput(inode);
> +		inode = NULL;
> +		goto out;
> +	}
> +	ei->sysctl = head;
> +	ei->sysctl_entry = table;
> +	list_add_rcu(&ei->sysctl_inodes, &head->inodes);
>  	head->count++;
>  	spin_unlock(&sysctl_lock);
>
> @@ -487,7 +492,7 @@ static struct inode *proc_sys_make_inode(struct super_block *sb,
>  void proc_sys_evict_inode(struct inode *inode, struct ctl_table_header *head)
>  {
>  	spin_lock(&sysctl_lock);
> -	list_del(&PROC_I(inode)->sysctl_inodes);
> +	list_del_rcu(&PROC_I(inode)->sysctl_inodes);
>  	if (!--head->count)
>  		kfree_rcu(head, rcu);
>  	spin_unlock(&sysctl_lock);
>

[toc] | [prev] | [next] | [standalone]

#1585686 — Re: [REVIEW][PATCH] proc/sysctl: Don't grab i_lock under sysctl_lock.

Konstantin Khlebnikov <khlebnikov@yandex-team.ru> writes:

> On 21.02.2017 04:41, Eric W. Biederman wrote:
>>
>> Konstantin Khlebnikov <khlebnikov@yandex-team.ru> writes:
>>> This patch has locking problem. I've got lockdep splat under LTP.
>>>
>>> [ 6633.115456] ======================================================
>>> [ 6633.115502] [ INFO: possible circular locking dependency detected ]
>>> [ 6633.115553] 4.9.10-debug+ #9 Tainted: G             L
>>> [ 6633.115584] -------------------------------------------------------
>>> [ 6633.115627] ksm02/284980 is trying to acquire lock:
>>> [ 6633.115659]  (&sb->s_type->i_lock_key#4){+.+...}, at: [<ffffffff816bc1ce>] igrab+0x1e/0x80
>>> [ 6633.115834] but task is already holding lock:
>>> [ 6633.115882]  (sysctl_lock){+.+...}, at: [<ffffffff817e379b>] unregister_sysctl_table+0x6b/0x110
>>> [ 6633.116026] which lock already depends on the new lock.
>>> [ 6633.116026]
>>> [ 6633.116080]
>>> [ 6633.116080] the existing dependency chain (in reverse order) is:
>>> [ 6633.116117]
>>> -> #2 (sysctl_lock){+.+...}:
>>> -> #1 (&(&dentry->d_lockref.lock)->rlock){+.+...}:
>>> -> #0 (&sb->s_type->i_lock_key#4){+.+...}:
>>>
>>> d_lock nests inside i_lock
>>> sysctl_lock nests inside d_lock in d_compare
>>>
>>> This patch adds i_lock nesting inside sysctl_lock.
>>
>> Al Viro <viro@ZenIV.linux.org.uk> replied:
>>> Once ->unregistering is set, you can drop sysctl_lock just fine.  So I'd
>>> try something like this - use rcu_read_lock() in proc_sys_prune_dcache(),
>>> drop sysctl_lock() before it and regain after.  Make sure that no inodes
>>> are added to the list ones ->unregistering has been set and use RCU list
>>> primitives for modifying the inode list, with sysctl_lock still used to
>>> serialize its modifications.
>>>
>>> Freeing struct inode is RCU-delayed (see proc_destroy_inode()), so doing
>>> igrab() is safe there.  Since we don't drop inode reference until after we'd
>>> passed beyond it in the list, list_for_each_entry_rcu() should be fine.
>>
>> I agree with Al Viro's analsysis of the situtation.
>>
>> Fixes: 802e348c6b77 ("proc/sysctl: prune stale dentries during unregistering")
>> Reported-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
>> Suggested-by: Al Viro <viro@ZenIV.linux.org.uk>
>> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
>> ---
>>
>> This is my cleaned up version of Al Viro's proposed fix.
>> I have tested it and the lockdep warnings go away, and
>> I have fixed a few trivial to ensure things work as intended.
>>
>> Unless someone sees a problem I am going to add this fix to my tree and
>> then send a pull request to Linus.
>
> I've tested the same patch and found no problems.
>
> Except proc_sys_prune_dcache() is no longer called under sysctl_lock
> like says comment above it.

Thank you.  I will add your Tested-by line to the patch.

Eric

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web