| Message ID | 20240307174226.627962-1-cem@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | tmpfs: Fix race on handling dquot rbtree | expand |
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 --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);