[rtems commit] bdbuf: Fix race condition with sync active flag

Sebastian Huber sebh at rtems.org
Fri Nov 28 10:12:59 UTC 2014

Module:    rtems
Branch:    master
Commit:    3b4ca3ab0f99d15794a3eee60b5735f834fd898c
Changeset: http://git.rtems.org/rtems/commit/?id=3b4ca3ab0f99d15794a3eee60b5735f834fd898c

Author:    Sebastian Huber <sebastian.huber at embedded-brains.de>
Date:      Thu Nov 27 14:41:17 2014 +0100

bdbuf: Fix race condition with sync active flag

Bug report by Oleg Kravtsov:

In rtems_bdbuf_swapout_processing() function there is the following

if (bdbuf_cache.sync_active && !transfered_buffers)

    rtems_id sync_requester;
    rtems_bdbuf_lock_cache ();


Here access to bdbuf_cache.sync_active is not protected with anything.
Imagine the following test case:

1. Task1 releases buffer(s) with bdbuf_release_modified() calls;

2. After a while swapout task starts and flushes all buffers;

3. In the end of that swapout flush we are before that part of code, and
assume there is task switching (just before "if (bdbuf_cache.sync_active
&& !transfered_buffers)");

4. Some other task (with higher priority) does bdbuf_release_modified
and rtems_bdbuf_syncdev().

This task successfully gets both locks sync and pool (in
rtems_bdbuf_syncdev() function), sets sync_active to true and starts
waiting for RTEMS_BDBUF_TRANSFER_SYNC event with only sync lock got.

5. Task switching happens again and we are again before "if
(bdbuf_cache.sync_active && !transfered_buffers)".

As the result we check sync_active and we come inside that "if"

6. The result is that we send RTEMS_BDBUF_TRANSFER_SYNC event! Though
ALL modified messages of that task are not flushed yet!

close #1485


 cpukit/libblock/src/bdbuf.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/cpukit/libblock/src/bdbuf.c b/cpukit/libblock/src/bdbuf.c
index bae57e2..8ea8cba 100644
--- a/cpukit/libblock/src/bdbuf.c
+++ b/cpukit/libblock/src/bdbuf.c
@@ -2749,10 +2749,16 @@ rtems_bdbuf_swapout_processing (unsigned long                 timer_delta,
   rtems_bdbuf_swapout_worker* worker;
   bool                        transfered_buffers = false;
+  bool                        sync_active;
   rtems_bdbuf_lock_cache ();
+   * To set this to true you need the cache and the sync lock.
+   */
+  sync_active = bdbuf_cache.sync_active;
+  /*
    * If a sync is active do not use a worker because the current code does not
    * cleaning up after. We need to know the buffers have been written when
    * syncing to release sync lock and currently worker threads do not return to
@@ -2761,7 +2767,7 @@ rtems_bdbuf_swapout_processing (unsigned long                 timer_delta,
    * lock. The simplest solution is to get the main swap out task perform all
    * sync operations.
-  if (bdbuf_cache.sync_active)
+  if (sync_active)
     worker = NULL;
@@ -2773,14 +2779,14 @@ rtems_bdbuf_swapout_processing (unsigned long                 timer_delta,
   rtems_chain_initialize_empty (&transfer->bds);
   transfer->dd = BDBUF_INVALID_DEV;
-  transfer->syncing = bdbuf_cache.sync_active;
+  transfer->syncing = sync_active;
    * When the sync is for a device limit the sync to that device. If the sync
    * is for a buffer handle process the devices in the order on the sync
    * list. This means the dev is BDBUF_INVALID_DEV.
-  if (bdbuf_cache.sync_active)
+  if (sync_active)
     transfer->dd = bdbuf_cache.sync_device;
@@ -2799,7 +2805,7 @@ rtems_bdbuf_swapout_processing (unsigned long                 timer_delta,
   rtems_bdbuf_swapout_modified_processing (&transfer->dd,
-                                           bdbuf_cache.sync_active,
+                                           sync_active,
@@ -2830,7 +2836,7 @@ rtems_bdbuf_swapout_processing (unsigned long                 timer_delta,
     transfered_buffers = true;
-  if (bdbuf_cache.sync_active && !transfered_buffers)
+  if (sync_active && !transfered_buffers)
     rtems_id sync_requester;
     rtems_bdbuf_lock_cache ();

More information about the vc mailing list