diff mbox series

tmpfs: Fix race on handling dquot rbtree

Message ID 20240307174226.627962-1-cem@kernel.org (mailing list archive)
State New, archived
Headers show
Series tmpfs: Fix race on handling dquot rbtree | expand

Commit Message

Carlos Maiolino March 7, 2024, 5:42 p.m. UTC
From: Carlos Maiolino <cem@kernel.org>

A syzkaller reproducer found a race while attempting to remove dquot information
from the rb tree.
Fetching the rb_tree root node must also be protected by the dqopt->dqio_sem,
otherwise, giving the right timing, shmem_release_dquot() will trigger a warning
because it couldn't find a node in the tree, when the real reason was the root
node changing before the search starts:

Thread 1				Thread 2
- shmem_release_dquot()			- shmem_{acquire,release}_dquot()

- fetch ROOT				- Fetch ROOT

					- acquire dqio_sem
- wait dqio_sem

					- do something, triger a tree rebalance
					- release dqio_sem

- acquire dqio_sem
- start searching for the node, but
  from the wrong location, missing
  the node, and triggering a warning.

Fixes: eafc474e202978 ("shmem: prepare shmem quota infrastructure")
Reported-by: Ubisectech Sirius <bugreport@ubisectech.com>
Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---

I had a chat with Aristeu Rozanski and Jan Kara about this issue, which made me
stop pursuing the wrong direction and reach the root cause faster, thanks guys.
 
 mm/shmem_quota.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

Comments

Jan Kara March 8, 2024, 11:26 p.m. UTC | #1
On Thu 07-03-24 18:42:10, cem@kernel.org wrote:
> From: Carlos Maiolino <cem@kernel.org>
> 
> A syzkaller reproducer found a race while attempting to remove dquot information
> from the rb tree.
> Fetching the rb_tree root node must also be protected by the dqopt->dqio_sem,
> otherwise, giving the right timing, shmem_release_dquot() will trigger a warning
> because it couldn't find a node in the tree, when the real reason was the root
> node changing before the search starts:
> 
> Thread 1				Thread 2
> - shmem_release_dquot()			- shmem_{acquire,release}_dquot()
> 
> - fetch ROOT				- Fetch ROOT
> 
> 					- acquire dqio_sem
> - wait dqio_sem
> 
> 					- do something, triger a tree rebalance
> 					- release dqio_sem
> 
> - acquire dqio_sem
> - start searching for the node, but
>   from the wrong location, missing
>   the node, and triggering a warning.
> 
> Fixes: eafc474e202978 ("shmem: prepare shmem quota infrastructure")
> Reported-by: Ubisectech Sirius <bugreport@ubisectech.com>
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>

Ah, good catch! Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
> 
> I had a chat with Aristeu Rozanski and Jan Kara about this issue, which made me
> stop pursuing the wrong direction and reach the root cause faster, thanks guys.
>  
>  mm/shmem_quota.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/shmem_quota.c b/mm/shmem_quota.c
> index 062d1c1097ae3..ce514e700d2f6 100644
> --- a/mm/shmem_quota.c
> +++ b/mm/shmem_quota.c
> @@ -116,7 +116,7 @@ static int shmem_free_file_info(struct super_block *sb, int type)
>  static int shmem_get_next_id(struct super_block *sb, struct kqid *qid)
>  {
>  	struct mem_dqinfo *info = sb_dqinfo(sb, qid->type);
> -	struct rb_node *node = ((struct rb_root *)info->dqi_priv)->rb_node;
> +	struct rb_node *node;
>  	qid_t id = from_kqid(&init_user_ns, *qid);
>  	struct quota_info *dqopt = sb_dqopt(sb);
>  	struct quota_id *entry = NULL;
> @@ -126,6 +126,7 @@ static int shmem_get_next_id(struct super_block *sb, struct kqid *qid)
>  		return -ESRCH;
>  
>  	down_read(&dqopt->dqio_sem);
> +	node = ((struct rb_root *)info->dqi_priv)->rb_node;
>  	while (node) {
>  		entry = rb_entry(node, struct quota_id, node);
>  
> @@ -165,7 +166,7 @@ static int shmem_get_next_id(struct super_block *sb, struct kqid *qid)
>  static int shmem_acquire_dquot(struct dquot *dquot)
>  {
>  	struct mem_dqinfo *info = sb_dqinfo(dquot->dq_sb, dquot->dq_id.type);
> -	struct rb_node **n = &((struct rb_root *)info->dqi_priv)->rb_node;
> +	struct rb_node **n;
>  	struct shmem_sb_info *sbinfo = dquot->dq_sb->s_fs_info;
>  	struct rb_node *parent = NULL, *new_node = NULL;
>  	struct quota_id *new_entry, *entry;
> @@ -176,6 +177,8 @@ static int shmem_acquire_dquot(struct dquot *dquot)
>  	mutex_lock(&dquot->dq_lock);
>  
>  	down_write(&dqopt->dqio_sem);
> +	n = &((struct rb_root *)info->dqi_priv)->rb_node;
> +
>  	while (*n) {
>  		parent = *n;
>  		entry = rb_entry(parent, struct quota_id, node);
> @@ -264,7 +267,7 @@ static bool shmem_is_empty_dquot(struct dquot *dquot)
>  static int shmem_release_dquot(struct dquot *dquot)
>  {
>  	struct mem_dqinfo *info = sb_dqinfo(dquot->dq_sb, dquot->dq_id.type);
> -	struct rb_node *node = ((struct rb_root *)info->dqi_priv)->rb_node;
> +	struct rb_node *node;
>  	qid_t id = from_kqid(&init_user_ns, dquot->dq_id);
>  	struct quota_info *dqopt = sb_dqopt(dquot->dq_sb);
>  	struct quota_id *entry = NULL;
> @@ -275,6 +278,7 @@ static int shmem_release_dquot(struct dquot *dquot)
>  		goto out_dqlock;
>  
>  	down_write(&dqopt->dqio_sem);
> +	node = ((struct rb_root *)info->dqi_priv)->rb_node;
>  	while (node) {
>  		entry = rb_entry(node, struct quota_id, node);
>  
> -- 
> 2.44.0
>
diff mbox series

Patch

diff --git a/mm/shmem_quota.c b/mm/shmem_quota.c
index 062d1c1097ae3..ce514e700d2f6 100644
--- a/mm/shmem_quota.c
+++ b/mm/shmem_quota.c
@@ -116,7 +116,7 @@  static int shmem_free_file_info(struct super_block *sb, int type)
 static int shmem_get_next_id(struct super_block *sb, struct kqid *qid)
 {
 	struct mem_dqinfo *info = sb_dqinfo(sb, qid->type);
-	struct rb_node *node = ((struct rb_root *)info->dqi_priv)->rb_node;
+	struct rb_node *node;
 	qid_t id = from_kqid(&init_user_ns, *qid);
 	struct quota_info *dqopt = sb_dqopt(sb);
 	struct quota_id *entry = NULL;
@@ -126,6 +126,7 @@  static int shmem_get_next_id(struct super_block *sb, struct kqid *qid)
 		return -ESRCH;
 
 	down_read(&dqopt->dqio_sem);
+	node = ((struct rb_root *)info->dqi_priv)->rb_node;
 	while (node) {
 		entry = rb_entry(node, struct quota_id, node);
 
@@ -165,7 +166,7 @@  static int shmem_get_next_id(struct super_block *sb, struct kqid *qid)
 static int shmem_acquire_dquot(struct dquot *dquot)
 {
 	struct mem_dqinfo *info = sb_dqinfo(dquot->dq_sb, dquot->dq_id.type);
-	struct rb_node **n = &((struct rb_root *)info->dqi_priv)->rb_node;
+	struct rb_node **n;
 	struct shmem_sb_info *sbinfo = dquot->dq_sb->s_fs_info;
 	struct rb_node *parent = NULL, *new_node = NULL;
 	struct quota_id *new_entry, *entry;
@@ -176,6 +177,8 @@  static int shmem_acquire_dquot(struct dquot *dquot)
 	mutex_lock(&dquot->dq_lock);
 
 	down_write(&dqopt->dqio_sem);
+	n = &((struct rb_root *)info->dqi_priv)->rb_node;
+
 	while (*n) {
 		parent = *n;
 		entry = rb_entry(parent, struct quota_id, node);
@@ -264,7 +267,7 @@  static bool shmem_is_empty_dquot(struct dquot *dquot)
 static int shmem_release_dquot(struct dquot *dquot)
 {
 	struct mem_dqinfo *info = sb_dqinfo(dquot->dq_sb, dquot->dq_id.type);
-	struct rb_node *node = ((struct rb_root *)info->dqi_priv)->rb_node;
+	struct rb_node *node;
 	qid_t id = from_kqid(&init_user_ns, dquot->dq_id);
 	struct quota_info *dqopt = sb_dqopt(dquot->dq_sb);
 	struct quota_id *entry = NULL;
@@ -275,6 +278,7 @@  static int shmem_release_dquot(struct dquot *dquot)
 		goto out_dqlock;
 
 	down_write(&dqopt->dqio_sem);
+	node = ((struct rb_root *)info->dqi_priv)->rb_node;
 	while (node) {
 		entry = rb_entry(node, struct quota_id, node);