Ocfs2 devel - Aug 2010 - BUG? a racy code at o2hb_heartbeat_group_drop_item()

Hello. I am reporting an atomic instructions usage
which seem be racy at fs/ocfs2/cluster/heartbeat.c in Linux 2.6.35.

I am reporting this issue while I read the recent code.
Since I do not have much background, I am not certain whether it is
correct or not.
But I hope this report is helpful. Please examine the code.

In o2hb_heartbeat_group_drop_item(),  there are following codes:

1626        if (atomic_read(&reg->hr_steady_iterations) != 0) {
1627                atomic_set(&reg->hr_steady_iterations, 0);


It first checks &reg->hr_steady_iterations and then updates its value.

In the case where other threads manipulate the same
&reg->hr_steady_iterations concurrently,
race condition might be possible.

I think it would be better to guarantee consecutive executions of
read and write by special purposed atomic operations (e.g. atomic_xchg)

Please examine the issue and let me know your opinion. Thank you.

Sincerely
Shin Hong

On 08/17/2010 05:51 AM, ?? shin hong wrote:
> Hello. I am reporting an atomic instructions usage
> which seem be racy at fs/ocfs2/cluster/heartbeat.c in Linux 2.6.35.
>
> I am reporting this issue while I read the recent code.
> Since I do not have much background, I am not certain whether it is
> correct or not.
> But I hope this report is helpful. Please examine the code.
>
> In o2hb_heartbeat_group_drop_item(),  there are following codes:
>
> 1626        if (atomic_read(&reg->hr_steady_iterations) != 0) {
> 1627                atomic_set(&reg->hr_steady_iterations, 0);
>
>
> It first checks&reg->hr_steady_iterations and then updates its
value.
>
> In the case where other threads manipulate the same
> &reg->hr_steady_iterations concurrently,
> race condition might be possible.
>
> I think it would be better to guarantee consecutive executions of
> read and write by special purposed atomic operations (e.g. atomic_xchg)
>
> Please examine the issue and let me know your opinion. Thank you.
>    
This should be safe. The only thread that could be decrementing
hr_steady_iterations is stopped before this check.