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] [thread-next>] [day] [month] [year] [list]

Message-ID: <53DDC5EB.3010209@gmail.com>
Date:	Sun, 03 Aug 2014 01:17:31 -0400
From:	Pranith Kumar <bobby.prani@...il.com>
To:	anssi.hannula@....fi
CC:	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 08/03/2014 12:57 AM, Pranith Kumar wrote:
> On 08/03/2014 12:46 AM, Pranith Kumar wrote:
>> On 08/02/2014 10:10 PM, Pranith Kumar wrote:
>>> Corrently adding Anssi this time.
>>>
>>> On Sat, Aug 2, 2014 at 10:00 PM, Pranith Kumar <bobby.prani@...il.com> wrote:
>>>> Hello Anssi, Joe, Mike,
>>>>
>>>> I just found your patch in the latest rc and wanted to ask a few
>>>> questions. I am not sure how this patch helps at all other than luck in
>>>> that dm_cblock_t type is of type int32_t, which should guarantee that it
>>>> is atomic on most platforms. Which begs the question, what platform did
>>>> you encounter this problem?
>>>>
>>>> The patch purports to solve a race condition by making use of atomic_t.
>>>> I am not sure that is enough. If indeed there is a race you need to use
>>>> smp_mb__{before/after}_atomic() for both your uses of atomic_inc() and
>>>> atomic_set().
>>>>
>>>> Also I have a concern about why this mail was not CC'ed on LKML. I had
>>>> to go to some difficulty in finding this patch. So please CC LKML for
>>>> such patches.
>>>>
>>>> Thanks,
>>>> --
>>>> Pranith
>>>>
>>>> -- Begin forwarded Message --
>>>>
>>>>
>>>> nr_dirty is updated without locking, causing it to drift so that it is
>>>> non-zero (either a small positive integer, or a very large one when an
>>>> underflow occurs) even when there are no actual dirty blocks.
>>>>
>>>> Fix that by using an atomic type for nr_dirty.
>>>>
>>>> Signed-off-by: Anssi Hannula <anssi hannula iki fi>
>>>> Cc: Joe Thornber <ejt redhat com>
>>>> Cc: stable vger kernel org
> 

There are more in the following patch. I think you need to really check what
else I might be missing.

From: Pranith Kumar <bobby.prani@...il.com>
Date: Sun, 3 Aug 2014 01:15:10 -0400
Subject: [PATCH 1/1] dm cache: Fix more incorrect pointer assignments

Fix incorrect pointer uses and assignments.

Signed-off-by: Pranith Kumar <bobby.prani@...il.com>
---
 drivers/md/dm-cache-target.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/md/dm-cache-target.c b/drivers/md/dm-cache-target.c
index 49e47e7..1627035 100644
--- a/drivers/md/dm-cache-target.c
+++ b/drivers/md/dm-cache-target.c
@@ -1777,13 +1777,13 @@ static void destroy(struct cache *cache)
 	if (cache->cmd)
 		dm_cache_metadata_close(cache->cmd);
 
-	if (cache->metadata_dev)
+	if (cache.metadata_dev)
 		dm_put_device(cache->ti, cache->metadata_dev);
 
-	if (cache->origin_dev)
+	if (cache.origin_dev)
 		dm_put_device(cache->ti, cache->origin_dev);
 
-	if (cache->cache_dev)
+	if (cache.cache_dev)
 		dm_put_device(cache->ti, cache->cache_dev);
 
 	if (cache->policy)
@@ -1861,13 +1861,13 @@ struct cache_args {
 
 static void destroy_cache_args(struct cache_args *ca)
 {
-	if (ca->metadata_dev)
+	if (ca.metadata_dev)
 		dm_put_device(ca->ti, ca->metadata_dev);
 
-	if (ca->cache_dev)
+	if (ca.cache_dev)
 		dm_put_device(ca->ti, ca->cache_dev);
 
-	if (ca->origin_dev)
+	if (ca.origin_dev)
 		dm_put_device(ca->ti, ca->origin_dev);
 
 	kfree(ca);
@@ -2190,7 +2190,7 @@ static int cache_create(struct cache_args *ca, struct cache **result)
 	cache->origin_dev = ca->origin_dev;
 	cache->cache_dev = ca->cache_dev;
 
-	ca->metadata_dev = ca->origin_dev = ca->cache_dev = NULL;
+	ca.metadata_dev = ca.origin_dev = ca.cache_dev = NULL;
 
 	/* FIXME: factor out this whole section */
 	origin_blocks = cache->origin_sectors = ca->origin_sectors;
-- 
1.9.1


--
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/