From 3b4ca3ab0f99d15794a3eee60b5735f834fd898c Mon Sep 17 00:00:00 2001 From: Sebastian Huber Date: Thu, 27 Nov 2014 14:41:17 +0100 Subject: 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 --- cpukit/libblock/src/bdbuf.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) (limited to 'cpukit/libblock') diff --git a/cpukit/libblock/src/bdbuf.c b/cpukit/libblock/src/bdbuf.c index bae57e2adf..8ea8cbada5 100644 --- a/cpukit/libblock/src/bdbuf.c +++ b/cpukit/libblock/src/bdbuf.c @@ -2749,9 +2749,15 @@ 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 @@ -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; else { @@ -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.modified, &transfer->bds, - bdbuf_cache.sync_active, + sync_active, update_timers, timer_delta); @@ -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 (); -- cgit v1.2.3