[patch] drbd: fix memory leak

[patch] drbd: fix memory leak

[Dan Carpenter]

We leak memory if "--dry-run" is not supported by the peer.

Signed-off-by: Dan Carpenter <error27@gmail.com>

diff --git a/drivers/block/drbd/drbd_main.c b/drivers/block/drbd/drbd_main.c
index 67e0fc5..93d1f9b 100644
--- a/drivers/block/drbd/drbd_main.c
+++ b/drivers/block/drbd/drbd_main.c
@@ -1695,6 +1695,7 @@ int drbd_send_protocol(struct drbd_conf *mdev)
 			cf |= CF_DRY_RUN;
 		else {
 			dev_err(DEV, "--dry-run is not supported by peer");
+			kfree(p);
 			return 0;
 		}
 	}

[Drbd-dev] [patch] drbd: fix memory leak

[Dan Carpenter]

We leak memory if "--dry-run" is not supported by the peer.

Signed-off-by: Dan Carpenter <error27@gmail.com>

diff --git a/drivers/block/drbd/drbd_main.c b/drivers/block/drbd/drbd_main.c
index 67e0fc5..93d1f9b 100644
--- a/drivers/block/drbd/drbd_main.c
+++ b/drivers/block/drbd/drbd_main.c
@@ -1695,6 +1695,7 @@ int drbd_send_protocol(struct drbd_conf *mdev)
 			cf |= CF_DRY_RUN;
 		else {
 			dev_err(DEV, "--dry-run is not supported by peer");
+			kfree(p);
 			return 0;
 		}
 	}

Re: [patch] drbd: fix memory leak

[Jens Axboe]

On Thu, Apr 22 2010, Dan Carpenter wrote:
> We leak memory if "--dry-run" is not supported by the peer.
> 
> Signed-off-by: Dan Carpenter <error27@gmail.com>
> 
> diff --git a/drivers/block/drbd/drbd_main.c b/drivers/block/drbd/drbd_main.c
> index 67e0fc5..93d1f9b 100644
> --- a/drivers/block/drbd/drbd_main.c
> +++ b/drivers/block/drbd/drbd_main.c
> @@ -1695,6 +1695,7 @@ int drbd_send_protocol(struct drbd_conf *mdev)
>  			cf |= CF_DRY_RUN;
>  		else {
>  			dev_err(DEV, "--dry-run is not supported by peer");
> +			kfree(p);
>  			return 0;
>  		}
>  	}

Yep, that's a leak. I have applied it for 2.6.34.

-- 
Jens Axboe

Re: [Drbd-dev] [patch] drbd: fix memory leak

[Jens Axboe]

On Thu, Apr 22 2010, Dan Carpenter wrote:
> We leak memory if "--dry-run" is not supported by the peer.
> 
> Signed-off-by: Dan Carpenter <error27@gmail.com>
> 
> diff --git a/drivers/block/drbd/drbd_main.c b/drivers/block/drbd/drbd_main.c
> index 67e0fc5..93d1f9b 100644
> --- a/drivers/block/drbd/drbd_main.c
> +++ b/drivers/block/drbd/drbd_main.c
> @@ -1695,6 +1695,7 @@ int drbd_send_protocol(struct drbd_conf *mdev)
>  			cf |= CF_DRY_RUN;
>  		else {
>  			dev_err(DEV, "--dry-run is not supported by peer");
> +			kfree(p);
>  			return 0;
>  		}
>  	}

Yep, that's a leak. I have applied it for 2.6.34.

-- 
Jens Axboe

Re: [patch] drbd: fix memory leak

[Philipp Reisner]

Hi Jens,

Please apply the following patch as well.

PS: I have put both at git://git.drbd.org/linux-2.6-drbd.git for-jens

Best,
 Phil

Re: [Drbd-dev] [patch] drbd: fix memory leak

[Philipp Reisner]

Hi Jens,

Please apply the following patch as well.

PS: I have put both at git://git.drbd.org/linux-2.6-drbd.git for-jens

Best,
 Phil

Re: [patch] drbd: fix memory leak

[Jens Axboe]

On Thu, Apr 22 2010, Philipp Reisner wrote:
> Hi Jens,
> 
> Please apply the following patch as well.
> 
> PS: I have put both at git://git.drbd.org/linux-2.6-drbd.git for-jens

Thanks, I just hand applied the 2nd one since the first one was already
there.

-- 
Jens Axboe

Re: [Drbd-dev] [patch] drbd: fix memory leak

[Jens Axboe]

On Thu, Apr 22 2010, Philipp Reisner wrote:
> Hi Jens,
> 
> Please apply the following patch as well.
> 
> PS: I have put both at git://git.drbd.org/linux-2.6-drbd.git for-jens

Thanks, I just hand applied the 2nd one since the first one was already
there.

-- 
Jens Axboe

[PATCH 1/1] drbd: Terminate a connection early if sending the protocol fails

[Philipp Reisner]

Signed-off-by: Philipp Reisner <philipp.reisner@linbit.com>
Signed-off-by: Lars Ellenberg <lars.ellenberg@linbit.com>
---
 drivers/block/drbd/drbd_receiver.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/block/drbd/drbd_receiver.c b/drivers/block/drbd/drbd_receiver.c
index ed9f1de..3f096e7 100644
--- a/drivers/block/drbd/drbd_receiver.c
+++ b/drivers/block/drbd/drbd_receiver.c
@@ -899,7 +899,8 @@ retry:
 
 	drbd_thread_start(&mdev->asender);
 
-	drbd_send_protocol(mdev);
+	if (!drbd_send_protocol(mdev))
+		return -1;
 	drbd_send_sync_param(mdev, &mdev->sync_conf);
 	drbd_send_sizes(mdev, 0);
 	drbd_send_uuids(mdev);
-- 
1.6.3.3

[Drbd-dev] [PATCH 1/1] drbd: Terminate a connection early if sending the protocol fails

[Philipp Reisner]

Signed-off-by: Philipp Reisner <philipp.reisner@linbit.com>
Signed-off-by: Lars Ellenberg <lars.ellenberg@linbit.com>
---
 drivers/block/drbd/drbd_receiver.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/block/drbd/drbd_receiver.c b/drivers/block/drbd/drbd_receiver.c
index ed9f1de..3f096e7 100644
--- a/drivers/block/drbd/drbd_receiver.c
+++ b/drivers/block/drbd/drbd_receiver.c
@@ -899,7 +899,8 @@ retry:
 
 	drbd_thread_start(&mdev->asender);
 
-	drbd_send_protocol(mdev);
+	if (!drbd_send_protocol(mdev))
+		return -1;
 	drbd_send_sync_param(mdev, &mdev->sync_conf);
 	drbd_send_sizes(mdev, 0);
 	drbd_send_uuids(mdev);
-- 
1.6.3.3

[PATCH] drbd: Fix memory leak

[zhengbing.huang]

In the output of kmemleak, we have the followe backtrace:

unreferenced object 0xffff8885b57cda80 (size 64):
  comm "drbd_r_testimg4", pid 37104, jiffies 4494192827 (age 127162.843s)
  hex dump (first 32 bytes):
    31 20 6f 66 20 32 20 6e 6f 64 65 73 20 76 69 73  1 of 2 nodes vis
    69 62 6c 65 2c 20 6e 65 65 64 20 32 20 66 6f 72  ible, need 2 for
  backtrace:
    [<000000006d641d68>] __kmalloc_track_caller+0x15c/0x270
    [<000000006a7ffbcf>] kvasprintf+0xa7/0x120
    [<000000002d2f15b3>] drbd_state_err+0xa9/0x190 [drbd]
    [<000000006aa2f3df>] __is_valid_soft_transition+0xe99/0xec0 [drbd]
    [<0000000009d68cc7>] try_state_change+0x4f0/0x840 [drbd]
    [<00000000d5640f06>] ___end_state_change+0x140/0x12a0 [drbd]
    [<000000009f4b8d71>] __end_state_change+0xa1/0x130 [drbd]
    [<000000001c6de1a7>] change_connection_state+0x5ee/0xbd0 [drbd]
    [<00000000ce4408d6>] process_twopc+0x1d3e/0x2ce0 [drbd]
    [<00000000df3af6e8>] receive_twopc+0x17b/0x2b0 [drbd]
    [<000000009701f919>] drbd_receiver+0x311/0x6e0 [drbd]
    [<0000000092c4aeb1>] drbd_thread_setup+0x19d/0x430 [drbd]
    [<0000000098e316ab>] kthread+0x19c/0x1c0
    [<000000004c72b3a8>] ret_from_fork+0x1f/0x40

This is a memory leak.

In drbd_state_err() function, if resource->state_change_err_str is a null pointer,
the err_str will not be free.
And _drbd_state_err() has same issues.

So, if err_str has not put to up layer, free it in current function.

Signed-off-by: zhengbing.huang <zhengbing.huang@easystack.cn>
---
 drbd/drbd_main.c  |  2 ++
 drbd/drbd_state.c | 10 +++++++++-
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/drbd/drbd_main.c b/drbd/drbd_main.c
index 86535080f..48c9588eb 100644
--- a/drbd/drbd_main.c
+++ b/drbd/drbd_main.c
@@ -3765,6 +3765,8 @@ struct drbd_resource *drbd_create_resource(const char *name,
 
 	list_add_tail_rcu(&resource->resources, &drbd_resources);
 
+	resource->state_change_err_str = NULL;
+
 	return resource;
 
 fail_free_pages:
diff --git a/drbd/drbd_state.c b/drbd/drbd_state.c
index 24ff7ab30..4102f2a04 100644
--- a/drbd/drbd_state.c
+++ b/drbd/drbd_state.c
@@ -1566,6 +1566,9 @@ static __printf(2, 3) void _drbd_state_err(struct change_context *context, const
 		*context->err_str = err_str;
 	if (context->flags & CS_VERBOSE)
 		drbd_err(resource, "%s\n", err_str);
+
+	if (!context->err_str)
+		kfree(err_str);
 }
 
 static __printf(2, 3) void drbd_state_err(struct drbd_resource *resource, const char *fmt, ...)
@@ -1582,6 +1585,9 @@ static __printf(2, 3) void drbd_state_err(struct drbd_resource *resource, const
 		*resource->state_change_err_str = err_str;
 	if (resource->state_change_flags & CS_VERBOSE)
 		drbd_err(resource, "%s\n", err_str);
+
+	if (!resource->state_change_err_str)
+		kfree(err_str);
 }
 
 static enum drbd_state_rv __is_valid_soft_transition(struct drbd_resource *resource)
@@ -5586,6 +5592,7 @@ static enum drbd_state_rv twopc_after_lost_peer(struct drbd_resource *resource,
 		.target_node_id = -1,
 		.flags = flags | (resource->res_opts.quorum != QOU_OFF ? CS_FORCE_RECALC : 0),
 		.change_local_state_last = false,
+		.err_str = NULL,
 	};
 
 	/* The other nodes get the request for an empty state change. I.e. they
@@ -5915,7 +5922,8 @@ enum drbd_state_rv change_repl_state(struct drbd_peer_device *peer_device,
 			.mask = { { .conn = conn_MASK } },
 			.val = { { .conn = new_repl_state } },
 			.target_node_id = peer_device->node_id,
-			.flags = flags
+			.flags = flags,
+			.err_str = NULL,
 		},
 		.peer_device = peer_device
 	};
-- 
2.43.0

Re: [PATCH] drbd: Fix memory leak

[Joel Colledge]

> In the output of kmemleak, we have the followe backtrace:
> ...
> This is a memory leak.

Thanks for testing with kmemleak and reporting this.

> diff --git a/drbd/drbd_main.c b/drbd/drbd_main.c
> index 86535080f..48c9588eb 100644
> --- a/drbd/drbd_main.c
> +++ b/drbd/drbd_main.c
> @@ -3765,6 +3765,8 @@ struct drbd_resource *drbd_create_resource(const char *name,
>
>         list_add_tail_rcu(&resource->resources, &drbd_resources);
>
> +       resource->state_change_err_str = NULL;

This is not necessary. "resource" is allocated with "kzalloc", which
zero-initializes the memory.

> diff --git a/drbd/drbd_state.c b/drbd/drbd_state.c
> index 24ff7ab30..4102f2a04 100644
> --- a/drbd/drbd_state.c
> +++ b/drbd/drbd_state.c
> @@ -1566,6 +1566,9 @@ static __printf(2, 3) void _drbd_state_err(struct change_context *context, const
>                 *context->err_str = err_str;
>         if (context->flags & CS_VERBOSE)
>                 drbd_err(resource, "%s\n", err_str);
> +
> +       if (!context->err_str)
> +               kfree(err_str);
>  }

Good spot. I find it cleaner to rearrange the code and make this an
"else", as follows. Do you agree that this works too? Changing the
order of printing the error and assigning the "err_str" field should
not have any impact on the behavior of DRBD.

@@ -1557,10 +1557,13 @@ static __printf(2, 3) void
_drbd_state_err(struct change_context *context, const
        va_end(args);
        if (!err_str)
                return;
-       if (context->err_str)
-               *context->err_str = err_str;
        if (context->flags & CS_VERBOSE)
                drbd_err(resource, "%s\n", err_str);
+
+       if (context->err_str)
+               *context->err_str = err_str;
+       else
+               kfree(err_str);
 }


>  static __printf(2, 3) void drbd_state_err(struct drbd_resource *resource, const char *fmt, ...)
> @@ -1582,6 +1585,9 @@ static __printf(2, 3) void drbd_state_err(struct drbd_resource *resource, const
>                 *resource->state_change_err_str = err_str;
>         if (resource->state_change_flags & CS_VERBOSE)
>                 drbd_err(resource, "%s\n", err_str);
> +
> +       if (!resource->state_change_err_str)
> +               kfree(err_str);
>  }

Rearrange into "if-else" as above.

>  static enum drbd_state_rv __is_valid_soft_transition(struct drbd_resource *resource)
> @@ -5586,6 +5592,7 @@ static enum drbd_state_rv twopc_after_lost_peer(struct drbd_resource *resource,
>                 .target_node_id = -1,
>                 .flags = flags | (resource->res_opts.quorum != QOU_OFF ? CS_FORCE_RECALC : 0),
>                 .change_local_state_last = false,
> +               .err_str = NULL,

This is not necessary. Omitted fields are initialized to zero.

>         };
>
>         /* The other nodes get the request for an empty state change. I.e. they
> @@ -5915,7 +5922,8 @@ enum drbd_state_rv change_repl_state(struct drbd_peer_device *peer_device,
>                         .mask = { { .conn = conn_MASK } },
>                         .val = { { .conn = new_repl_state } },
>                         .target_node_id = peer_device->node_id,
> -                       .flags = flags
> +                       .flags = flags,
> +                       .err_str = NULL,

This is not necessary. Omitted fields are initialized to zero.

Best regards,
Joel