linux-kernel - Re: [PATCH] dm cache: fix race affecting dirty block count

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAJhHMCAskizhfCN5SLk7cd5QY3FNoz6fN+YTWOMWH4ezTw0Lww@mail.gmail.com>
Date:	Mon, 4 Aug 2014 11:02:05 -0400
From:	Pranith Kumar <bobby.prani@...il.com>
To:	Pranith Kumar <bobby.prani@...il.com>,
	Anssi Hannula <anssi.hannula@....fi>, ejt@...hat.com,
	snitzer@...hat.com, LKML <linux-kernel@...r.kernel.org>,
	torvalds <torvalds@...ux-foundation.org>
Subject: Re: [PATCH] dm cache: fix race affecting dirty block count

On Mon, Aug 4, 2014 at 6:48 AM, Joe Thornber <thornber@...hat.com> wrote:
> On Sun, Aug 03, 2014 at 12:01:17AM -0400, Pranith Kumar wrote:
>> Also dm_cblock_t is uint32_t, but atomic_t changes that to int. You
>> should correct that to atomic64_t to preserve original semantics.
>
> atomic_t used to have only 24 bits of range due to the Sparc
> implementation holding a lock in one of the bytes.  I understand this
> limitation was removed during 2.6 and the full 32 bits are now
> available.
>

I meant to point out that atomic_t is a signed integer (int) type
using the full 32 bits with signed operations. dm_cblock_t is unsgined
int.

>
>> These increments and decrements will still be lost if you do not use
>> barriers in presence of concurrent accesses. Please see
>> Documentation/memory-barriers.txt.
>
> You do not need to use barriers for plain atomic_inc/dec().
>
>     https://github.com/jthornber/linux-2.6/blob/thin-dev/Documentation/atomic_ops.txt#L187

That talks about implementation of atomic_inc/dec() for arch porters.
Users of atomic_inc/dec() should use memory barriers.

>
> You _do_ need to use a memory barrier for the ops that return a value
> (such as atomic_dec_and_test()), But only if there's some other state
> that needs synchronising.  See the nice example in atomic_ops.txt:
>
>     https://github.com/jthornber/linux-2.6/blob/thin-dev/Documentation/atomic_ops.txt#L321

Again when it says it needs explicit memory barriers, it is for the
arch porters. So atomic_dec_and_test(), atomic_dec_return() etc., have
implicit memory barriers.

>
> We just trigger a stateless event when the counter hits zero, so the
> patch is fine.
>

Your use of atomic_dec_return() is what is fixing the race issue here
I guess , as it has implicit memory barriers. But I suggest checking
out if you need barriers for atomic_inc() and atomic_set() too.

-- 
Pranith
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/