Skip to contentSkip to conversation

dm cache: fix up IS_ERR vs NULL confusion

PATCH In review Dan Carpenter 2 participants 3 messages Jan 28, 2015

Subsystems (3)
Kernel janitors MD RAID Device mapper
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/1 dm cache: fix up IS_ERR vs NULL confusion

It used to be that this code used ERR_PTRs consistently, but a recent change mixed it up. The code sometimes returns NULL, sometimes an ERR_PTR, some code assumes everything is an ERR_PTR and some assumes it returns NULL on error. I've changed it back so that now everything is an ERR_PTR again.

These new bugs were caught by static checking:

drivers/md/dm-cache-target.c:2409 cache_create() warn: 'cmd' isn't an ERR_PTR drivers/md/dm-cache-metadata.c:754 lookup_or_open() error: 'cmd' dereferencing possible ERR_PTR() drivers/md/dm-cache-metadata.c:757 lookup_or_open() error: 'cmd' dereferencing possible ERR_PTR()

I also reversed some tests for failure so that it was more clear and had fewer indent levels.

Fixes 9b1cc9f251af ('dm cache: share cache-metadata object across inactive and active DM tables')
Signed-off-by Dan Carpenter <[email protected]>

---
The one question I have is that I made dm_cache_metadata_open() either preserve the error code or return -EINVAL. I haven't tested this so I guess review carefully. This patch should probably be back ported to stable or folded into Joe's.

drivers/md/dm-cache-metadata.c +22 -18
index 21b1562..3b08cdf 100644
@@ -681,10 +681,8 @@ static struct dm_cache_metadata *metadata_open(struct block_device *bdev,
 	struct dm_cache_metadata *cmd;
 
 	cmd = kzalloc(sizeof(*cmd), GFP_KERNEL);
-	if (!cmd) {
-		DMERR("could not allocate metadata struct");
-		return NULL;
-	}
+	if (!cmd)
+		return ERR_PTR(-ENOMEM);
 
 	atomic_set(&cmd->ref_count, 1);
 	init_rwsem(&cmd->root_lock);

@@ -745,18 +743,19 @@ static struct dm_cache_metadata *lookup_or_open(struct block_device *bdev,
 		return cmd;
 
 	cmd = metadata_open(bdev, data_block_size, may_format_device, policy_hint_size);
-	if (cmd) {
-		mutex_lock(&table_lock);
-		cmd2 = lookup(bdev);
-		if (cmd2) {
-			mutex_unlock(&table_lock);
-			__destroy_persistent_data_objects(cmd);
-			kfree(cmd);
-			return cmd2;
-		}
-		list_add(&cmd->list, &table);
+	if (IS_ERR(cmd))
+		return cmd;
+
+	mutex_lock(&table_lock);
+	cmd2 = lookup(bdev);
+	if (cmd2) {
 		mutex_unlock(&table_lock);
+		__destroy_persistent_data_objects(cmd);
+		kfree(cmd);
+		return cmd2;
 	}
+	list_add(&cmd->list, &table);
+	mutex_unlock(&table_lock);
 
 	return cmd;
 }

@@ -778,11 +777,16 @@ struct dm_cache_metadata *dm_cache_metadata_open(struct block_device *bdev,
 						 bool may_format_device,
 						 size_t policy_hint_size)
 {
-	struct dm_cache_metadata *cmd = lookup_or_open(bdev, data_block_size,
-						       may_format_device, policy_hint_size);
-	if (cmd && !same_params(cmd, data_block_size)) {
+	struct dm_cache_metadata *cmd;
+
+	cmd = lookup_or_open(bdev, data_block_size,
+			     may_format_device, policy_hint_size);
+	if (IS_ERR(cmd))
+		return cmd;
+
+	if (!same_params(cmd, data_block_size)) {
 		dm_cache_metadata_close(cmd);
-		return NULL;
+		return ERR_PTR(-EINVAL);
 	}
 
 	return cmd;
#1 lore raw

Dan,

Thanks. I hate errptrs so my natural inclination is just return NULLs. I'll fix up and retest.

- Joe

Quoted 96 lines · Dan Carpenter

It used to be that this code used ERR_PTRs consistently, but a recent change mixed it up. The code sometimes returns NULL, sometimes an ERR_PTR, some code assumes everything is an ERR_PTR and some assumes it returns NULL on error. I've changed it back so that now everything is an ERR_PTR again.

These new bugs were caught by static checking:

drivers/md/dm-cache-target.c:2409 cache_create() warn: 'cmd' isn't an ERR_PTR drivers/md/dm-cache-metadata.c:754 lookup_or_open() error: 'cmd' dereferencing possible ERR_PTR() drivers/md/dm-cache-metadata.c:757 lookup_or_open() error: 'cmd' dereferencing possible ERR_PTR()

I also reversed some tests for failure so that it was more clear and had fewer indent levels.

Fixes 9b1cc9f251af ('dm cache: share cache-metadata object across inactive and active DM tables')
Signed-off-by Dan Carpenter <[email protected]>

---
The one question I have is that I made dm_cache_metadata_open() either preserve the error code or return -EINVAL. I haven't tested this so I guess review carefully. This patch should probably be back ported to stable or folded into Joe's.

drivers/md/dm-cache-metadata.c +22 -19
index 21b1562..3b08cdf 100644
@@ -681,10 +681,8 @@ static struct dm_cache_metadata *metadata_open(struct block_device *bdev,
 	struct dm_cache_metadata *cmd;
 
 	cmd = kzalloc(sizeof(*cmd), GFP_KERNEL);
-	if (!cmd) {
-		DMERR("could not allocate metadata struct");
-		return NULL;
-	}
+	if (!cmd)
+		return ERR_PTR(-ENOMEM);
 
 	atomic_set(&cmd->ref_count, 1);
 	init_rwsem(&cmd->root_lock);

@@ -745,18 +743,19 @@ static struct dm_cache_metadata *lookup_or_open(struct block_device *bdev,
 		return cmd;
 
 	cmd = metadata_open(bdev, data_block_size, may_format_device, policy_hint_size);
-	if (cmd) {
-		mutex_lock(&table_lock);
-		cmd2 = lookup(bdev);
-		if (cmd2) {
-			mutex_unlock(&table_lock);
-			__destroy_persistent_data_objects(cmd);
-			kfree(cmd);
-			return cmd2;
-		}
-		list_add(&cmd->list, &table);
+	if (IS_ERR(cmd))
+		return cmd;
+
+	mutex_lock(&table_lock);
+	cmd2 = lookup(bdev);
+	if (cmd2) {
 		mutex_unlock(&table_lock);
+		__destroy_persistent_data_objects(cmd);
+		kfree(cmd);
+		return cmd2;
 	}
+	list_add(&cmd->list, &table);
+	mutex_unlock(&table_lock);
 
 	return cmd;
 }

@@ -778,11 +777,16 @@ struct dm_cache_metadata *dm_cache_metadata_open(struct block_device *bdev,
 						 bool may_format_device,
 						 size_t policy_hint_size)
 {
-	struct dm_cache_metadata *cmd = lookup_or_open(bdev, data_block_size,
-						       may_format_device, policy_hint_size);
-	if (cmd && !same_params(cmd, data_block_size)) {
+	struct dm_cache_metadata *cmd;
+
+	cmd = lookup_or_open(bdev, data_block_size,
+			     may_format_device, policy_hint_size);
+	if (IS_ERR(cmd))
+		return cmd;
+
+	if (!same_params(cmd, data_block_size)) {
 		dm_cache_metadata_close(cmd);
-		return NULL;
+		return ERR_PTR(-EINVAL);
 	}
 
 	return cmd;
 
--

dm-devel mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/dm-devel

#2 lore raw

Here's my take. You were destroying the metadata object with the mutex still held, which I'd been trying to avoid.

commit ea87d6105c1b13c13ece0fc8d9add09c78b1aa4c

Author: Joe Thornber <[email protected]>
Date:   Wed Jan 28 12:07:46 2015 +0000

dm-cache: fixup evil err ptrs that were ommitted from earlier patch.

drivers/md/dm-cache-metadata.c +4 -4
index 8876250..b6dc968 100644
@@ -683,7 +683,7 @@ static struct dm_cache_metadata *metadata_open(struct block_device *bdev,
        cmd = kzalloc(sizeof(*cmd), GFP_KERNEL);
        if (!cmd) {
                DMERR("could not allocate metadata struct");
-               return NULL;
+               return ERR_PTR(-ENOMEM);
        }
 
        atomic_set(&cmd->ref_count, 1);

@@ -745,7 +745,7 @@ static struct dm_cache_metadata *lookup_or_open(struct block_device *bdev,
                return cmd;
 
        cmd = metadata_open(bdev, data_block_size, may_format_device, policy_hint_size);
-       if (cmd) {
+       if (!IS_ERR(cmd)) {
                mutex_lock(&table_lock);
                cmd2 = lookup(bdev);
                if (cmd2) {

@@ -779,9 +779,9 @@ struct dm_cache_metadata *dm_cache_metadata_open(struct block_device *bdev,
                                                 size_t policy_hint_size)
 {
        struct dm_cache_metadata *cmd = lookup_or_open(bdev, data_block_size, may_format_device, policy_hint_size);
-       if (cmd && !same_params(cmd, data_block_size)) {
+       if (!IS_ERR(cmd) && !same_params(cmd, data_block_size)) {
                dm_cache_metadata_close(cmd);
-               return NULL;
+               return ERR_PTR(-EINVAL);
        }
 
        return cmd;