* [PATCH] tmpfs: Fix race on handling dquot rbtree
@ 2024-03-07 17:42 cem
2024-03-08 23:26 ` Jan Kara
0 siblings, 1 reply; 2+ messages in thread
From: cem @ 2024-03-07 17:42 UTC (permalink / raw)
To: hughd; +Cc: jack, linux-mm, linux-fsdevel
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(-)
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
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] tmpfs: Fix race on handling dquot rbtree
2024-03-07 17:42 [PATCH] tmpfs: Fix race on handling dquot rbtree cem
@ 2024-03-08 23:26 ` Jan Kara
0 siblings, 0 replies; 2+ messages in thread
From: Jan Kara @ 2024-03-08 23:26 UTC (permalink / raw)
To: cem; +Cc: hughd, jack, linux-mm, linux-fsdevel
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
>
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2024-03-08 23:26 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-03-07 17:42 [PATCH] tmpfs: Fix race on handling dquot rbtree cem
2024-03-08 23:26 ` Jan Kara
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).