[PATCH] bdbuf: Fix race condition with sync active flag

[Gedare Bloom]

Looks OK. Need to apply to releases?

On Thu, Nov 27, 2014 at 8:43 AM, Sebastian Huber
<sebastian.huber at embedded-brains.de> wrote:
> 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(-)
>
> 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;
>    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 ();
> --
> 1.8.4.5
>
> _______________________________________________
> devel mailing list
> devel at rtems.org
> http://lists.rtems.org/mailman/listinfo/devel