[PATCH v2] fs: jfs: fix a possible data race in metapage_writepage()

[PATCH v2] fs: jfs: fix a possible data race in metapage_writepage()

[Jia-Ju Bai]

The functions metapage_writepage() and lmPostGC() can be concurrently 
executed in the following call contexts:

Thread1:
  metapage_writepage()

Thread2:
  lbmIODone()
    lmPostGC()

In metapage_writepage():
  if (mp->log && !(mp->log->cflag & logGC_PAGEOUT))

In lmPostGC():
  spin_lock_irqsave(&log->gclock, flags);
  ...
  log->cflag &= ~logGC_PAGEOUT
  ...
  spin_unlock_irqrestore(&log->gclock, flags);

The memory addresses of mp->log->cflag and log->cflag can be identical,
and thus a data race can occur.

This data race is found by our concurrency fuzzer.

Thus use the spin lock "mp->log->gclock" for the assignment of 
the data structure member "log->cflag" to a local variable 
in this function implementation.

Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com>
---
v2:
* Change the description.
  Thank Markus Elfring for good advice.

---
 fs/jfs/jfs_metapage.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/fs/jfs/jfs_metapage.c b/fs/jfs/jfs_metapage.c
index a2f5338a5ea1..026c11b2572d 100644
--- a/fs/jfs/jfs_metapage.c
+++ b/fs/jfs/jfs_metapage.c
@@ -351,6 +351,7 @@ static int metapage_writepage(struct page *page, struct writeback_control *wbc)
 	unsigned long bio_offset = 0;
 	int offset;
 	int bad_blocks = 0;
+	uint cflag;
 
 	page_start = (sector_t)page->index <<
 		     (PAGE_SHIFT - inode->i_blkbits);
@@ -370,8 +371,14 @@ static int metapage_writepage(struct page *page, struct writeback_control *wbc)
 			 * Make sure this page isn't blocked indefinitely.
 			 * If the journal isn't undergoing I/O, push it
 			 */
-			if (mp->log && !(mp->log->cflag & logGC_PAGEOUT))
-				jfs_flush_journal(mp->log, 0);
+
+			if (mp->log) {
+				spin_lock_irq(&mp->log->gclock);
+				cflag = mp->log->cflag;
+				spin_unlock_irq(&mp->log->gclock);
+				if (!(cflag & logGC_PAGEOUT))
+					jfs_flush_journal(mp->log, 0);
+			}
 			continue;
 		}
 
-- 
2.17.1

Re: [PATCH v2] fs: jfs: fix a possible data race in metapage_writepage()

[Markus Elfring]

> This data race is found by our concurrency fuzzer.

* How do you think about to replace the word “is” by “was”?

* Is this analysis tool publicly available?


…
> ---
>  fs/jfs/jfs_metapage.c | 11 +++++++++--

I suggest to omit the triple dashes before this information.

Regards,
Markus

Re: [PATCH v2] fs: jfs: fix a possible data race in metapage_writepage()

[Jia-Ju Bai]

On 2020/5/5 22:15, Markus Elfring wrote:
>> This data race is found by our concurrency fuzzer.
> * How do you think about to replace the word “is” by “was”?

Okay.

> * Is this analysis tool publicly available?

Not yet, because we are still developing this tool...

>
> …
>> ---
>>   fs/jfs/jfs_metapage.c | 11 +++++++++--
> I suggest to omit the triple dashes before this information.

Okay, thanks.


Best wishes,
Jia-Ju Bai

Re: [PATCH v2] fs: jfs: fix a possible data race in metapage_writepage()

[Dave Kleikamp]

On 5/5/20 9:15 AM, Markus Elfring wrote:
>> This data race is found by our concurrency fuzzer.
> 
> * How do you think about to replace the word “is” by “was”?
> 
> * Is this analysis tool publicly available?
> 
> 
> …
>> ---
>>  fs/jfs/jfs_metapage.c | 11 +++++++++--
> 
> I suggest to omit the triple dashes before this information.

That's standard. There's no need to the diffstat to persist in the
commit message.

> 
> Regards,
> Markus
> 

Re: [v2] fs: jfs: fix a possible data race in metapage_writepage()

[Markus Elfring]

>>> This data race is found by our concurrency fuzzer.
>> * How do you think about to replace the word “is” by “was”?
>
> Okay.

Can such a positive feedback influence the change descriptions
for any of your other patches?

Regards,
Markus

Re: [v2] fs: jfs: fix a possible data race in metapage_writepage()

[Jia-Ju Bai]

On 2020/5/5 22:32, Markus Elfring wrote:
>>>> This data race is found by our concurrency fuzzer.
>>> * How do you think about to replace the word “is” by “was”?
>> Okay.
> Can such a positive feedback influence the change descriptions
> for any of your other patches?

Okay, sure, thanks for the advice.
I will resend my patches for jfs.


Best wishes,
Jia-Ju Bai

[PATCH v2] fs: jfs: fix a possible data race in metapage_writepage()

[Jia-Ju Bai]

The functions metapage_writepage() and lmPostGC() can be concurrently 
executed in the following call contexts:

Thread1:
  metapage_writepage()

Thread2:
  lbmIODone()
    lmPostGC()

In metapage_writepage():
  if (mp->log && !(mp->log->cflag & logGC_PAGEOUT))

In lmPostGC():
  spin_lock_irqsave(&log->gclock, flags);
  ...
  log->cflag &= ~logGC_PAGEOUT
  ...
  spin_unlock_irqrestore(&log->gclock, flags);

The memory addresses of mp->log->cflag and log->cflag can be identical,
and thus a data race can occur.
This data race was found by our concurrency fuzzer.

Thus use the spin lock "mp->log->gclock" for the assignment of 
the data structure member "log->cflag" to a local variable 
in this function implementation.

Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com>
---
v2:
* Change the description.
  Thank Markus Elfring for good advice.

 fs/jfs/jfs_metapage.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/fs/jfs/jfs_metapage.c b/fs/jfs/jfs_metapage.c
index a2f5338a5ea1..026c11b2572d 100644
--- a/fs/jfs/jfs_metapage.c
+++ b/fs/jfs/jfs_metapage.c
@@ -351,6 +351,7 @@ static int metapage_writepage(struct page *page, struct writeback_control *wbc)
 	unsigned long bio_offset = 0;
 	int offset;
 	int bad_blocks = 0;
+	uint cflag;
 
 	page_start = (sector_t)page->index <<
 		     (PAGE_SHIFT - inode->i_blkbits);
@@ -370,8 +371,14 @@ static int metapage_writepage(struct page *page, struct writeback_control *wbc)
 			 * Make sure this page isn't blocked indefinitely.
 			 * If the journal isn't undergoing I/O, push it
 			 */
-			if (mp->log && !(mp->log->cflag & logGC_PAGEOUT))
-				jfs_flush_journal(mp->log, 0);
+
+			if (mp->log) {
+				spin_lock_irq(&mp->log->gclock);
+				cflag = mp->log->cflag;
+				spin_unlock_irq(&mp->log->gclock);
+				if (!(cflag & logGC_PAGEOUT))
+					jfs_flush_journal(mp->log, 0);
+			}
 			continue;
 		}
 
-- 
2.17.1