diff options
author | Sebastian Huber <sebastian.huber@embedded-brains.de> | 2014-11-27 14:41:17 +0100 |
---|---|---|
committer | Sebastian Huber <sebastian.huber@embedded-brains.de> | 2014-11-28 11:15:18 +0100 |
commit | f0f2a3dc19e8caddccbb756898b5a413dad9de6a (patch) | |
tree | f8a15b2387a4b40ff56b2ca1d80365c6b49fdeab | |
parent | SPI SD-Card: adapt common driver code to block devices core API changes. (diff) | |
download | rtems-f0f2a3dc19e8caddccbb756898b5a413dad9de6a.tar.bz2 |
bdbuf: Fix race condition with sync active flag
Bug report by Oleg Kravtsov:
In rtems_bdbuf_swapout_processing() function there is the following
lines:
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"
statement.
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
-rw-r--r-- | cpukit/libblock/src/bdbuf.c | 20 |
1 files changed, 13 insertions, 7 deletions
diff --git a/cpukit/libblock/src/bdbuf.c b/cpukit/libblock/src/bdbuf.c index 71086a2709..125c10e6cf 100644 --- a/cpukit/libblock/src/bdbuf.c +++ b/cpukit/libblock/src/bdbuf.c @@ -2534,10 +2534,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 @@ -2546,7 +2552,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; else { @@ -2558,16 +2564,16 @@ rtems_bdbuf_swapout_processing (unsigned long timer_delta, rtems_chain_initialize_empty (&transfer->bds); transfer->dev = 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->dev = bdbuf_cache.sync_device; - + /* * If we have any buffers in the sync queue move them to the modified * list. The first sync buffer will select the device we use. @@ -2584,7 +2590,7 @@ rtems_bdbuf_swapout_processing (unsigned long timer_delta, rtems_bdbuf_swapout_modified_processing (&transfer->dev, &bdbuf_cache.modified, &transfer->bds, - bdbuf_cache.sync_active, + sync_active, update_timers, timer_delta); @@ -2615,7 +2621,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 (); |