BUG? a suspected race bug at check_journal_end()

[홍신 shin hong]

Hello. I am reporting atomic instructions usages
which are suspected to be misused in reiserfs/journal.c
of Linux 2.6.31

I do not have much background on reiserfs
so that I am not certain whether it is correct or not.
But I hope this report is helpful. Please examine the code.

In check_journal_end(), there are following codes:

    if (atomic_read(&(journal->j_wcount)) > 0) {
        atomic_dec(&(journal->j_wcount)) ;


It first checks journal->j_wcount and then increments its value by one.

If a function which changes journal->j_wcount executes concurrently
for the same journal->j_wcount, race condition might be possible.

I think it would be better to combine two atomic operations
into one atomic operation (e.g. atomic_dec_and_test)

Thank you.

Sincerely
Shin Hong
--
To unsubscribe from this list: send the line "unsubscribe reiserfs-devel" in
the body of a message to majordomo@...
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Sergey Senozhatsky-2]

On (11/03/09 22:08), 홍신 shin hong wrote:

> Hello. I am reporting atomic instructions usages
> which are suspected to be misused in reiserfs/journal.c
> of Linux 2.6.31
>
> I do not have much background on reiserfs
> so that I am not certain whether it is correct or not.
> But I hope this report is helpful. Please examine the code.
>
> In check_journal_end(), there are following codes:
>
>     if (atomic_read(&(journal->j_wcount)) > 0) {
>         atomic_dec(&(journal->j_wcount)) ;
>
>
> It first checks journal->j_wcount and then increments its value by one.
>
> If a function which changes journal->j_wcount executes concurrently
> for the same journal->j_wcount, race condition might be possible.
>
> I think it would be better to combine two atomic operations
> into one atomic operation (e.g. atomic_dec_and_test)
>

Hello,
Isn't it performed on locked journal?

journal.c:3973:
        lock_journal(sb);
        ...
 
 
        Sergey