BUG? a possible race due to the absence of memory barrier

BUG? a possible race due to the absence of memory barrier

[홍신 shin hong]

Hello. I am reporting possible data race
due to the the absence of memory barriers.

I reported a similar issue. Although the previous one turns out to be safe,
please examine this issue and let me know your opinion.

In btrfs_init_new_device(), a btrfs_device object is allocated and initialized
and then links to &root->fs_info->fs_devcies->alloc_list.

It seems that a memory barrier is necessary
between the initialization and the linking to the list.

If these two operations are re-ordered so that executed opposite orders,
it may result data race where uninitialized values are read by other threads.

For btrfs_init_new_device(), i think __btfs_alloc_chunk() is a suspected
to be possible to contribute data race by concurrent execution.

Thank you

Sincerely
Shin Hong

Re: BUG? a possible race due to the absence of memory barrier

[Chris Mason]

On Thu, Nov 12, 2009 at 12:07:05AM +0900, =ED=99=8D=EC=8B=A0 shin hong =
wrote:
> Hello. I am reporting possible data race
> due to the the absence of memory barriers.
>=20
> I reported a similar issue. Although the previous one turns out to be=
 safe,
> please examine this issue and let me know your opinion.
>=20
> In btrfs_init_new_device(), a btrfs_device object is allocated and in=
itialized
> and then links to &root->fs_info->fs_devcies->alloc_list.
>=20
> It seems that a memory barrier is necessary
> between the initialization and the linking to the list.
>=20
> If these two operations are re-ordered so that executed opposite orde=
rs,
> it may result data race where uninitialized values are read by other =
threads.
>=20
> For btrfs_init_new_device(), i think __btfs_alloc_chunk() is a suspec=
ted
> to be possible to contribute data race by concurrent execution.

Thanks for searching for races in this code, it definitely has a lot of
locks to go through.

In this case, btrfs_init_new_device has the chunk mutex held (from
lock_chunks), and __btrfs_alloc_chunk should always be called by with
the chunk mutex held as well.

In general the btrfs locking tries not to rely on barriers and ordering
unless a given area of the code is very performance sensitive.  It's
very easy for subtle bugs to creep in with barriers only, so I try to
use mutexes and spinlocks everywhere that I can get away with it.

-chris
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" =
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: BUG? a possible race due to the absence of memory barrier

[홍신 shin hong]

Thank you for the review.

I did not notice that lock_chunks() is a locking function.

I am using my own static analysis for finding bugs.
As I register lock_chunks() as a locking functions,
the bug alarm is disappeared.

On Thu, Nov 12, 2009 at 1:06 AM, Chris Mason <chris.mason@oracle.com> w=
rote:
> On Thu, Nov 12, 2009 at 12:07:05AM +0900, =ED=99=8D=EC=8B=A0 shin hon=
g wrote:
>> Hello. I am reporting possible data race
>> due to the the absence of memory barriers.
>>
>> I reported a similar issue. Although the previous one turns out to b=
e safe,
>> please examine this issue and let me know your opinion.
>>
>> In btrfs_init_new_device(), a btrfs_device object is allocated and i=
nitialized
>> and then links to &root->fs_info->fs_devcies->alloc_list.
>>
>> It seems that a memory barrier is necessary
>> between the initialization and the linking to the list.
>>
>> If these two operations are re-ordered so that executed opposite ord=
ers,
>> it may result data race where uninitialized values are read by other=
 threads.
>>
>> For btrfs_init_new_device(), i think __btfs_alloc_chunk() is a suspe=
cted
>> to be possible to contribute data race by concurrent execution.
>
> Thanks for searching for races in this code, it definitely has a lot =
of
> locks to go through.
>
> In this case, btrfs_init_new_device has the chunk mutex held (from
> lock_chunks), and __btrfs_alloc_chunk should always be called by with
> the chunk mutex held as well.
>
> In general the btrfs locking tries not to rely on barriers and orderi=
ng
> unless a given area of the code is very performance sensitive. =C2=A0=
It's
> very easy for subtle bugs to creep in with barriers only, so I try to
> use mutexes and spinlocks everywhere that I can get away with it.
>
> -chris
>
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" =
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html