forked from Imagelibrary/rtems
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
This commit is contained in:
@@ -2534,9 +2534,15 @@ rtems_bdbuf_swapout_processing (unsigned long timer_delta,
|
|||||||
{
|
{
|
||||||
rtems_bdbuf_swapout_worker* worker;
|
rtems_bdbuf_swapout_worker* worker;
|
||||||
bool transfered_buffers = false;
|
bool transfered_buffers = false;
|
||||||
|
bool sync_active;
|
||||||
|
|
||||||
rtems_bdbuf_lock_cache ();
|
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
|
* 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
|
* cleaning up after. We need to know the buffers have been written when
|
||||||
@@ -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
|
* lock. The simplest solution is to get the main swap out task perform all
|
||||||
* sync operations.
|
* sync operations.
|
||||||
*/
|
*/
|
||||||
if (bdbuf_cache.sync_active)
|
if (sync_active)
|
||||||
worker = NULL;
|
worker = NULL;
|
||||||
else
|
else
|
||||||
{
|
{
|
||||||
@@ -2558,16 +2564,16 @@ rtems_bdbuf_swapout_processing (unsigned long timer_delta,
|
|||||||
|
|
||||||
rtems_chain_initialize_empty (&transfer->bds);
|
rtems_chain_initialize_empty (&transfer->bds);
|
||||||
transfer->dev = BDBUF_INVALID_DEV;
|
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
|
* 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
|
* is for a buffer handle process the devices in the order on the sync
|
||||||
* list. This means the dev is BDBUF_INVALID_DEV.
|
* list. This means the dev is BDBUF_INVALID_DEV.
|
||||||
*/
|
*/
|
||||||
if (bdbuf_cache.sync_active)
|
if (sync_active)
|
||||||
transfer->dev = bdbuf_cache.sync_device;
|
transfer->dev = bdbuf_cache.sync_device;
|
||||||
|
|
||||||
/*
|
/*
|
||||||
* If we have any buffers in the sync queue move them to the modified
|
* 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.
|
* 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,
|
rtems_bdbuf_swapout_modified_processing (&transfer->dev,
|
||||||
&bdbuf_cache.modified,
|
&bdbuf_cache.modified,
|
||||||
&transfer->bds,
|
&transfer->bds,
|
||||||
bdbuf_cache.sync_active,
|
sync_active,
|
||||||
update_timers,
|
update_timers,
|
||||||
timer_delta);
|
timer_delta);
|
||||||
|
|
||||||
@@ -2615,7 +2621,7 @@ rtems_bdbuf_swapout_processing (unsigned long timer_delta,
|
|||||||
transfered_buffers = true;
|
transfered_buffers = true;
|
||||||
}
|
}
|
||||||
|
|
||||||
if (bdbuf_cache.sync_active && !transfered_buffers)
|
if (sync_active && !transfered_buffers)
|
||||||
{
|
{
|
||||||
rtems_id sync_requester;
|
rtems_id sync_requester;
|
||||||
rtems_bdbuf_lock_cache ();
|
rtems_bdbuf_lock_cache ();
|
||||||
|
|||||||
Reference in New Issue
Block a user