[PATCH -next v2 0/2] md/raid5-cache: fix a deadlock in r5l_exit_log()

[PATCH -next v2 0/2] md/raid5-cache: fix a deadlock in r5l_exit_log()

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

Changes in v2:
 - remove a now unused local variable in patch 2;

Commit b13015af94cf ("md/raid5-cache: Clear conf->log after finishing
work") introduce a new problem:

// caller hold reconfig_mutex
r5l_exit_log
 flush_work(&log->disable_writeback_work)
			r5c_disable_writeback_async
			 wait_event
			  /*
			   * conf->log is not NULL, and mddev_trylock()
			   * will fail, wait_event() can never pass.
			   */
 conf->log = NULL

patch 1 revert this patch, an patch 2 fix the original problem in a
different way.

Noted this problem is just found by code review, and I think this is
probably the reason that some mdadm tests is broken.

Yu Kuai (2):
  md/raid5-cache: Revert "md/raid5-cache: Clear conf->log after
    finishing work"
  md/raid5-cache: fix null-ptr-deref in r5l_reclaim_thread()

 drivers/md/raid5-cache.c | 25 ++++++++++---------------
 1 file changed, 10 insertions(+), 15 deletions(-)

-- 
2.39.2

[PATCH -next v2 1/2] md/raid5-cache: Revert "md/raid5-cache: Clear conf->log after finishing work"

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

This reverts commit b13015af94cf405f73ff64ce0797269554020c37.

Because this will cause that r5c_disable_writeback_async() to wait
forever, since caller hold reconfig_mutex and conf->log is not NULL:

wait_event
 conf->log == NULL ||
 (!test_bit(MD_SB_CHANGE_PENDING, &mddev->sb_flags) &&
  (locked = mddev_trylock(mddev)))

This problem is found by code review, and the null-ptr-deref this patch
fixed will be fixed by another approch in the next patch.

Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 drivers/md/raid5-cache.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/md/raid5-cache.c b/drivers/md/raid5-cache.c
index 47ba7d9e81e1..083288e36949 100644
--- a/drivers/md/raid5-cache.c
+++ b/drivers/md/raid5-cache.c
@@ -3168,13 +3168,12 @@ void r5l_exit_log(struct r5conf *conf)
 {
 	struct r5l_log *log = conf->log;
 
+	conf->log = NULL;
+
 	/* Ensure disable_writeback_work wakes up and exits */
 	wake_up(&conf->mddev->sb_wait);
 	flush_work(&log->disable_writeback_work);
 	md_unregister_thread(&log->reclaim_thread);
-
-	conf->log = NULL;
-
 	mempool_exit(&log->meta_pool);
 	bioset_exit(&log->bs);
 	mempool_exit(&log->io_pool);
-- 
2.39.2

[PATCH -next v2 2/2] md/raid5-cache: fix null-ptr-deref in r5l_reclaim_thread()

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

r5l_reclaim_thread() already check that 'conf->log' is not NULL in the
beginning, however, r5c_do_reclaim() and r5l_do_reclaim() will
dereference 'conf->log' again, which will cause null-ptr-deref if
'conf->log' is set to NULL from r5l_exit_log() concurrently.

Fix this problem by don't dereference 'conf->log' again in
r5c_do_reclaim() and r5c_do_reclaim().

Fixes: a39f7afde358 ("md/r5cache: write-out phase and reclaim support")
Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 drivers/md/raid5-cache.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/drivers/md/raid5-cache.c b/drivers/md/raid5-cache.c
index 083288e36949..ba6fc146d265 100644
--- a/drivers/md/raid5-cache.c
+++ b/drivers/md/raid5-cache.c
@@ -1148,10 +1148,9 @@ static void r5l_run_no_space_stripes(struct r5l_log *log)
  * for write through mode, returns log->next_checkpoint
  * for write back, returns log_start of first sh in stripe_in_journal_list
  */
-static sector_t r5c_calculate_new_cp(struct r5conf *conf)
+static sector_t r5c_calculate_new_cp(struct r5l_log *log)
 {
 	struct stripe_head *sh;
-	struct r5l_log *log = conf->log;
 	sector_t new_cp;
 	unsigned long flags;
 
@@ -1159,12 +1158,12 @@ static sector_t r5c_calculate_new_cp(struct r5conf *conf)
 		return log->next_checkpoint;
 
 	spin_lock_irqsave(&log->stripe_in_journal_lock, flags);
-	if (list_empty(&conf->log->stripe_in_journal_list)) {
+	if (list_empty(&log->stripe_in_journal_list)) {
 		/* all stripes flushed */
 		spin_unlock_irqrestore(&log->stripe_in_journal_lock, flags);
 		return log->next_checkpoint;
 	}
-	sh = list_first_entry(&conf->log->stripe_in_journal_list,
+	sh = list_first_entry(&log->stripe_in_journal_list,
 			      struct stripe_head, r5c);
 	new_cp = sh->log_start;
 	spin_unlock_irqrestore(&log->stripe_in_journal_lock, flags);
@@ -1173,10 +1172,8 @@ static sector_t r5c_calculate_new_cp(struct r5conf *conf)
 
 static sector_t r5l_reclaimable_space(struct r5l_log *log)
 {
-	struct r5conf *conf = log->rdev->mddev->private;
-
 	return r5l_ring_distance(log, log->last_checkpoint,
-				 r5c_calculate_new_cp(conf));
+				 r5c_calculate_new_cp(log));
 }
 
 static void r5l_run_no_mem_stripe(struct r5l_log *log)
@@ -1419,9 +1416,9 @@ void r5c_flush_cache(struct r5conf *conf, int num)
 	}
 }
 
-static void r5c_do_reclaim(struct r5conf *conf)
+static void r5c_do_reclaim(struct r5l_log *log)
 {
-	struct r5l_log *log = conf->log;
+	struct r5conf *conf = log->rdev->mddev->private;
 	struct stripe_head *sh;
 	int count = 0;
 	unsigned long flags;
@@ -1496,7 +1493,6 @@ static void r5c_do_reclaim(struct r5conf *conf)
 
 static void r5l_do_reclaim(struct r5l_log *log)
 {
-	struct r5conf *conf = log->rdev->mddev->private;
 	sector_t reclaim_target = xchg(&log->reclaim_target, 0);
 	sector_t reclaimable;
 	sector_t next_checkpoint;
@@ -1525,7 +1521,7 @@ static void r5l_do_reclaim(struct r5l_log *log)
 				    log->io_list_lock);
 	}
 
-	next_checkpoint = r5c_calculate_new_cp(conf);
+	next_checkpoint = r5c_calculate_new_cp(log);
 	spin_unlock_irq(&log->io_list_lock);
 
 	if (reclaimable == 0 || !write_super)
@@ -1554,7 +1550,7 @@ static void r5l_reclaim_thread(struct md_thread *thread)
 
 	if (!log)
 		return;
-	r5c_do_reclaim(conf);
+	r5c_do_reclaim(log);
 	r5l_do_reclaim(log);
 }
 
-- 
2.39.2

Re: [PATCH -next v2 2/2] md/raid5-cache: fix null-ptr-deref in r5l_reclaim_thread()

[Song Liu]

On Wed, Jun 28, 2023 at 9:08 AM Yu Kuai <yukuai1@huaweicloud.com> wrote:
>
> From: Yu Kuai <yukuai3@huawei.com>
>
> r5l_reclaim_thread() already check that 'conf->log' is not NULL in the
> beginning, however, r5c_do_reclaim() and r5l_do_reclaim() will
> dereference 'conf->log' again, which will cause null-ptr-deref if
> 'conf->log' is set to NULL from r5l_exit_log() concurrently.

r5l_exit_log() will wait until reclaim_thread() finishes, and then set
conf->log to NULL. So this is not a problem, no?

Thanks,
Song

>
> Fix this problem by don't dereference 'conf->log' again in
> r5c_do_reclaim() and r5c_do_reclaim().
>
> Fixes: a39f7afde358 ("md/r5cache: write-out phase and reclaim support")
> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
> ---
>  drivers/md/raid5-cache.c | 20 ++++++++------------
>  1 file changed, 8 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/md/raid5-cache.c b/drivers/md/raid5-cache.c
> index 083288e36949..ba6fc146d265 100644
> --- a/drivers/md/raid5-cache.c
> +++ b/drivers/md/raid5-cache.c
> @@ -1148,10 +1148,9 @@ static void r5l_run_no_space_stripes(struct r5l_log *log)
>   * for write through mode, returns log->next_checkpoint
>   * for write back, returns log_start of first sh in stripe_in_journal_list
>   */
> -static sector_t r5c_calculate_new_cp(struct r5conf *conf)
> +static sector_t r5c_calculate_new_cp(struct r5l_log *log)
>  {
>         struct stripe_head *sh;
> -       struct r5l_log *log = conf->log;
>         sector_t new_cp;
>         unsigned long flags;
>
> @@ -1159,12 +1158,12 @@ static sector_t r5c_calculate_new_cp(struct r5conf *conf)
>                 return log->next_checkpoint;
>
>         spin_lock_irqsave(&log->stripe_in_journal_lock, flags);
> -       if (list_empty(&conf->log->stripe_in_journal_list)) {
> +       if (list_empty(&log->stripe_in_journal_list)) {
>                 /* all stripes flushed */
>                 spin_unlock_irqrestore(&log->stripe_in_journal_lock, flags);
>                 return log->next_checkpoint;
>         }
> -       sh = list_first_entry(&conf->log->stripe_in_journal_list,
> +       sh = list_first_entry(&log->stripe_in_journal_list,
>                               struct stripe_head, r5c);
>         new_cp = sh->log_start;
>         spin_unlock_irqrestore(&log->stripe_in_journal_lock, flags);
> @@ -1173,10 +1172,8 @@ static sector_t r5c_calculate_new_cp(struct r5conf *conf)
>
>  static sector_t r5l_reclaimable_space(struct r5l_log *log)
>  {
> -       struct r5conf *conf = log->rdev->mddev->private;
> -
>         return r5l_ring_distance(log, log->last_checkpoint,
> -                                r5c_calculate_new_cp(conf));
> +                                r5c_calculate_new_cp(log));
>  }
>
>  static void r5l_run_no_mem_stripe(struct r5l_log *log)
> @@ -1419,9 +1416,9 @@ void r5c_flush_cache(struct r5conf *conf, int num)
>         }
>  }
>
> -static void r5c_do_reclaim(struct r5conf *conf)
> +static void r5c_do_reclaim(struct r5l_log *log)
>  {
> -       struct r5l_log *log = conf->log;
> +       struct r5conf *conf = log->rdev->mddev->private;
>         struct stripe_head *sh;
>         int count = 0;
>         unsigned long flags;
> @@ -1496,7 +1493,6 @@ static void r5c_do_reclaim(struct r5conf *conf)
>
>  static void r5l_do_reclaim(struct r5l_log *log)
>  {
> -       struct r5conf *conf = log->rdev->mddev->private;
>         sector_t reclaim_target = xchg(&log->reclaim_target, 0);
>         sector_t reclaimable;
>         sector_t next_checkpoint;
> @@ -1525,7 +1521,7 @@ static void r5l_do_reclaim(struct r5l_log *log)
>                                     log->io_list_lock);
>         }
>
> -       next_checkpoint = r5c_calculate_new_cp(conf);
> +       next_checkpoint = r5c_calculate_new_cp(log);
>         spin_unlock_irq(&log->io_list_lock);
>
>         if (reclaimable == 0 || !write_super)
> @@ -1554,7 +1550,7 @@ static void r5l_reclaim_thread(struct md_thread *thread)
>
>         if (!log)
>                 return;
> -       r5c_do_reclaim(conf);
> +       r5c_do_reclaim(log);
>         r5l_do_reclaim(log);
>  }
>
> --
> 2.39.2
>

Re: [PATCH -next v2 2/2] md/raid5-cache: fix null-ptr-deref in r5l_reclaim_thread()

[Yu Kuai]

Hi,

在 2023/07/07 16:52, Song Liu 写道:
> On Wed, Jun 28, 2023 at 9:08 AM Yu Kuai <yukuai1@huaweicloud.com> wrote:
>>
>> From: Yu Kuai <yukuai3@huawei.com>
>>
>> r5l_reclaim_thread() already check that 'conf->log' is not NULL in the
>> beginning, however, r5c_do_reclaim() and r5l_do_reclaim() will
>> dereference 'conf->log' again, which will cause null-ptr-deref if
>> 'conf->log' is set to NULL from r5l_exit_log() concurrently.
> 
> r5l_exit_log() will wait until reclaim_thread() finishes, and then set
> conf->log to NULL. So this is not a problem, no?

Patch one just revert this, wait until reclaim_thread() then set
conf->log to NULL will cause deadlock, as I sescribled in patch 0.

Thanks,
Kuai
> 
> Thanks,
> Song
> 
>>
>> Fix this problem by don't dereference 'conf->log' again in
>> r5c_do_reclaim() and r5c_do_reclaim().
>>
>> Fixes: a39f7afde358 ("md/r5cache: write-out phase and reclaim support")
>> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
>> ---
>>   drivers/md/raid5-cache.c | 20 ++++++++------------
>>   1 file changed, 8 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/md/raid5-cache.c b/drivers/md/raid5-cache.c
>> index 083288e36949..ba6fc146d265 100644
>> --- a/drivers/md/raid5-cache.c
>> +++ b/drivers/md/raid5-cache.c
>> @@ -1148,10 +1148,9 @@ static void r5l_run_no_space_stripes(struct r5l_log *log)
>>    * for write through mode, returns log->next_checkpoint
>>    * for write back, returns log_start of first sh in stripe_in_journal_list
>>    */
>> -static sector_t r5c_calculate_new_cp(struct r5conf *conf)
>> +static sector_t r5c_calculate_new_cp(struct r5l_log *log)
>>   {
>>          struct stripe_head *sh;
>> -       struct r5l_log *log = conf->log;
>>          sector_t new_cp;
>>          unsigned long flags;
>>
>> @@ -1159,12 +1158,12 @@ static sector_t r5c_calculate_new_cp(struct r5conf *conf)
>>                  return log->next_checkpoint;
>>
>>          spin_lock_irqsave(&log->stripe_in_journal_lock, flags);
>> -       if (list_empty(&conf->log->stripe_in_journal_list)) {
>> +       if (list_empty(&log->stripe_in_journal_list)) {
>>                  /* all stripes flushed */
>>                  spin_unlock_irqrestore(&log->stripe_in_journal_lock, flags);
>>                  return log->next_checkpoint;
>>          }
>> -       sh = list_first_entry(&conf->log->stripe_in_journal_list,
>> +       sh = list_first_entry(&log->stripe_in_journal_list,
>>                                struct stripe_head, r5c);
>>          new_cp = sh->log_start;
>>          spin_unlock_irqrestore(&log->stripe_in_journal_lock, flags);
>> @@ -1173,10 +1172,8 @@ static sector_t r5c_calculate_new_cp(struct r5conf *conf)
>>
>>   static sector_t r5l_reclaimable_space(struct r5l_log *log)
>>   {
>> -       struct r5conf *conf = log->rdev->mddev->private;
>> -
>>          return r5l_ring_distance(log, log->last_checkpoint,
>> -                                r5c_calculate_new_cp(conf));
>> +                                r5c_calculate_new_cp(log));
>>   }
>>
>>   static void r5l_run_no_mem_stripe(struct r5l_log *log)
>> @@ -1419,9 +1416,9 @@ void r5c_flush_cache(struct r5conf *conf, int num)
>>          }
>>   }
>>
>> -static void r5c_do_reclaim(struct r5conf *conf)
>> +static void r5c_do_reclaim(struct r5l_log *log)
>>   {
>> -       struct r5l_log *log = conf->log;
>> +       struct r5conf *conf = log->rdev->mddev->private;
>>          struct stripe_head *sh;
>>          int count = 0;
>>          unsigned long flags;
>> @@ -1496,7 +1493,6 @@ static void r5c_do_reclaim(struct r5conf *conf)
>>
>>   static void r5l_do_reclaim(struct r5l_log *log)
>>   {
>> -       struct r5conf *conf = log->rdev->mddev->private;
>>          sector_t reclaim_target = xchg(&log->reclaim_target, 0);
>>          sector_t reclaimable;
>>          sector_t next_checkpoint;
>> @@ -1525,7 +1521,7 @@ static void r5l_do_reclaim(struct r5l_log *log)
>>                                      log->io_list_lock);
>>          }
>>
>> -       next_checkpoint = r5c_calculate_new_cp(conf);
>> +       next_checkpoint = r5c_calculate_new_cp(log);
>>          spin_unlock_irq(&log->io_list_lock);
>>
>>          if (reclaimable == 0 || !write_super)
>> @@ -1554,7 +1550,7 @@ static void r5l_reclaim_thread(struct md_thread *thread)
>>
>>          if (!log)
>>                  return;
>> -       r5c_do_reclaim(conf);
>> +       r5c_do_reclaim(log);
>>          r5l_do_reclaim(log);
>>   }
>>
>> --
>> 2.39.2
>>
> .
> 

Re: [PATCH -next v2 2/2] md/raid5-cache: fix null-ptr-deref in r5l_reclaim_thread()

[Yu Kuai]

Hi,

在 2023/07/07 17:06, Yu Kuai 写道:
> Hi,
> 
> 在 2023/07/07 16:52, Song Liu 写道:
>> On Wed, Jun 28, 2023 at 9:08 AM Yu Kuai <yukuai1@huaweicloud.com> wrote:
>>>
>>> From: Yu Kuai <yukuai3@huawei.com>
>>>
>>> r5l_reclaim_thread() already check that 'conf->log' is not NULL in the
>>> beginning, however, r5c_do_reclaim() and r5l_do_reclaim() will
>>> dereference 'conf->log' again, which will cause null-ptr-deref if
>>> 'conf->log' is set to NULL from r5l_exit_log() concurrently.
>>
>> r5l_exit_log() will wait until reclaim_thread() finishes, and then set
>> conf->log to NULL. So this is not a problem, no?

Perhaps you means this order?

r5l_exit_log
  flush_work(&log->disable_writeback_work)
  conf->log = NULL
  md_unregister_thread(&log->reclaim_thread)

I think this is better indeed.

Thanks,
Kuai
> 
> Patch one just revert this, wait until reclaim_thread() then set
> conf->log to NULL will cause deadlock, as I sescribled in patch 0.
> 
> Thanks,
> Kuai
>>
>> Thanks,
>> Song
>>
>>>
>>> Fix this problem by don't dereference 'conf->log' again in
>>> r5c_do_reclaim() and r5c_do_reclaim().
>>>
>>> Fixes: a39f7afde358 ("md/r5cache: write-out phase and reclaim support")
>>> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
>>> ---
>>>   drivers/md/raid5-cache.c | 20 ++++++++------------
>>>   1 file changed, 8 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/md/raid5-cache.c b/drivers/md/raid5-cache.c
>>> index 083288e36949..ba6fc146d265 100644
>>> --- a/drivers/md/raid5-cache.c
>>> +++ b/drivers/md/raid5-cache.c
>>> @@ -1148,10 +1148,9 @@ static void r5l_run_no_space_stripes(struct 
>>> r5l_log *log)
>>>    * for write through mode, returns log->next_checkpoint
>>>    * for write back, returns log_start of first sh in 
>>> stripe_in_journal_list
>>>    */
>>> -static sector_t r5c_calculate_new_cp(struct r5conf *conf)
>>> +static sector_t r5c_calculate_new_cp(struct r5l_log *log)
>>>   {
>>>          struct stripe_head *sh;
>>> -       struct r5l_log *log = conf->log;
>>>          sector_t new_cp;
>>>          unsigned long flags;
>>>
>>> @@ -1159,12 +1158,12 @@ static sector_t r5c_calculate_new_cp(struct 
>>> r5conf *conf)
>>>                  return log->next_checkpoint;
>>>
>>>          spin_lock_irqsave(&log->stripe_in_journal_lock, flags);
>>> -       if (list_empty(&conf->log->stripe_in_journal_list)) {
>>> +       if (list_empty(&log->stripe_in_journal_list)) {
>>>                  /* all stripes flushed */
>>>                  spin_unlock_irqrestore(&log->stripe_in_journal_lock, 
>>> flags);
>>>                  return log->next_checkpoint;
>>>          }
>>> -       sh = list_first_entry(&conf->log->stripe_in_journal_list,
>>> +       sh = list_first_entry(&log->stripe_in_journal_list,
>>>                                struct stripe_head, r5c);
>>>          new_cp = sh->log_start;
>>>          spin_unlock_irqrestore(&log->stripe_in_journal_lock, flags);
>>> @@ -1173,10 +1172,8 @@ static sector_t r5c_calculate_new_cp(struct 
>>> r5conf *conf)
>>>
>>>   static sector_t r5l_reclaimable_space(struct r5l_log *log)
>>>   {
>>> -       struct r5conf *conf = log->rdev->mddev->private;
>>> -
>>>          return r5l_ring_distance(log, log->last_checkpoint,
>>> -                                r5c_calculate_new_cp(conf));
>>> +                                r5c_calculate_new_cp(log));
>>>   }
>>>
>>>   static void r5l_run_no_mem_stripe(struct r5l_log *log)
>>> @@ -1419,9 +1416,9 @@ void r5c_flush_cache(struct r5conf *conf, int num)
>>>          }
>>>   }
>>>
>>> -static void r5c_do_reclaim(struct r5conf *conf)
>>> +static void r5c_do_reclaim(struct r5l_log *log)
>>>   {
>>> -       struct r5l_log *log = conf->log;
>>> +       struct r5conf *conf = log->rdev->mddev->private;
>>>          struct stripe_head *sh;
>>>          int count = 0;
>>>          unsigned long flags;
>>> @@ -1496,7 +1493,6 @@ static void r5c_do_reclaim(struct r5conf *conf)
>>>
>>>   static void r5l_do_reclaim(struct r5l_log *log)
>>>   {
>>> -       struct r5conf *conf = log->rdev->mddev->private;
>>>          sector_t reclaim_target = xchg(&log->reclaim_target, 0);
>>>          sector_t reclaimable;
>>>          sector_t next_checkpoint;
>>> @@ -1525,7 +1521,7 @@ static void r5l_do_reclaim(struct r5l_log *log)
>>>                                      log->io_list_lock);
>>>          }
>>>
>>> -       next_checkpoint = r5c_calculate_new_cp(conf);
>>> +       next_checkpoint = r5c_calculate_new_cp(log);
>>>          spin_unlock_irq(&log->io_list_lock);
>>>
>>>          if (reclaimable == 0 || !write_super)
>>> @@ -1554,7 +1550,7 @@ static void r5l_reclaim_thread(struct md_thread 
>>> *thread)
>>>
>>>          if (!log)
>>>                  return;
>>> -       r5c_do_reclaim(conf);
>>> +       r5c_do_reclaim(log);
>>>          r5l_do_reclaim(log);
>>>   }
>>>
>>> -- 
>>> 2.39.2
>>>
>> .
>>
> 
> .
> 

Re: [PATCH -next v2 2/2] md/raid5-cache: fix null-ptr-deref in r5l_reclaim_thread()

[Yu Kuai]

Hi,

在 2023/07/07 17:16, Yu Kuai 写道:
> Perhaps you means this order?
> 
> r5l_exit_log
>   flush_work(&log->disable_writeback_work)
>   conf->log = NULL
>   md_unregister_thread(&log->reclaim_thread)
> 
> I think this is better indeed.
Never mind, this is wrong, I got confused...

Please ignore this and take a look at my original fix.

Thanks,
Kuai

Re: [PATCH -next v2 2/2] md/raid5-cache: fix null-ptr-deref in r5l_reclaim_thread()

[Song Liu]

On Fri, Jul 7, 2023 at 5:19 PM Yu Kuai <yukuai1@huaweicloud.com> wrote:
>
> Hi,
>
> 在 2023/07/07 17:16, Yu Kuai 写道:
> > Perhaps you means this order?
> >
> > r5l_exit_log
> >   flush_work(&log->disable_writeback_work)
> >   conf->log = NULL
> >   md_unregister_thread(&log->reclaim_thread)
> >
> > I think this is better indeed.
> Never mind, this is wrong, I got confused...
>
> Please ignore this and take a look at my original fix.

How about

r5l_exit_log
  md_unregister_thread(&log->reclaim_thread)
  conf->log = NULL
  flush_work(&log->disable_writeback_work)

?

Thanks,
Song

Re: [PATCH -next v2 2/2] md/raid5-cache: fix null-ptr-deref in r5l_reclaim_thread()

[Yu Kuai]

Hi,

在 2023/07/07 17:36, Song Liu 写道:
> On Fri, Jul 7, 2023 at 5:19 PM Yu Kuai <yukuai1@huaweicloud.com> wrote:
>>
>> Hi,
>>
>> 在 2023/07/07 17:16, Yu Kuai 写道:
>>> Perhaps you means this order?
>>>
>>> r5l_exit_log
>>>    flush_work(&log->disable_writeback_work)
>>>    conf->log = NULL
>>>    md_unregister_thread(&log->reclaim_thread)
>>>
>>> I think this is better indeed.
>> Never mind, this is wrong, I got confused...
>>
>> Please ignore this and take a look at my original fix.
> 
> How about
> 
> r5l_exit_log
>    md_unregister_thread(&log->reclaim_thread)
>    conf->log = NULL
>    flush_work(&log->disable_writeback_work)
> 
> ?

This looks correct, expect that wake_up() should be moved together.

I'll send a v2.

Thanks,
Kuai
> 
> Thanks,
> Song
> .
> 

Re: [PATCH -next v2 0/2] md/raid5-cache: fix a deadlock in r5l_exit_log()

[Yu Kuai]

Hi,

在 2023/06/28 9:07, Yu Kuai 写道:
> From: Yu Kuai <yukuai3@huawei.com>
> 
> Changes in v2:
>   - remove a now unused local variable in patch 2;
> 
> Commit b13015af94cf ("md/raid5-cache: Clear conf->log after finishing
> work") introduce a new problem:
> 
> // caller hold reconfig_mutex
> r5l_exit_log
>   flush_work(&log->disable_writeback_work)
> 			r5c_disable_writeback_async
> 			 wait_event
> 			  /*
> 			   * conf->log is not NULL, and mddev_trylock()
> 			   * will fail, wait_event() can never pass.
> 			   */
>   conf->log = NULL
> 
> patch 1 revert this patch, an patch 2 fix the original problem in a
> different way.
> 
> Noted this problem is just found by code review, and I think this is
> probably the reason that some mdadm tests is broken.

Any suggestions?

By the way, while taking another look at this problem, I think probably
read and write 'conf->log' should use READ_ONCE and WRITE_ONCE.

Thanks,
Kuai
> 
> Yu Kuai (2):
>    md/raid5-cache: Revert "md/raid5-cache: Clear conf->log after
>      finishing work"
>    md/raid5-cache: fix null-ptr-deref in r5l_reclaim_thread()
> 
>   drivers/md/raid5-cache.c | 25 ++++++++++---------------
>   1 file changed, 10 insertions(+), 15 deletions(-)
>