Linux-Block Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] nbd: Fix memory leak in nbd_add_socket
@ 2020-06-12  9:04 Zheng Bin
  2020-06-25  0:16 ` Chaitanya Kulkarni
  0 siblings, 1 reply; 3+ messages in thread
From: Zheng Bin @ 2020-06-12  9:04 UTC (permalink / raw)
  To: josef, axboe, linux-block, nbd; +Cc: yi.zhang, zhengbin13

nbd_add_socket
  socks = krealloc(num_connections+1) -->if num_connections is 0, alloc 1
  nsock = kzalloc                     -->If fail, will return

nbd_config_put
  if (config->num_connections)        -->0, not free
    kfree(config->socks)

Thus memleak happens, this patch fixes that.

Signed-off-by: Zheng Bin <zhengbin13@huawei.com>
---
 drivers/block/nbd.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 43cff01a5a67..3e7709317b17 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -1037,21 +1037,22 @@ static int nbd_add_socket(struct nbd_device *nbd, unsigned long arg,
 		return -EBUSY;
 	}

+	nsock = kzalloc(sizeof(struct nbd_sock), GFP_KERNEL);
+	if (!nsock) {
+		sockfd_put(sock);
+		return -ENOMEM;
+	}
+
 	socks = krealloc(config->socks, (config->num_connections + 1) *
 			 sizeof(struct nbd_sock *), GFP_KERNEL);
 	if (!socks) {
 		sockfd_put(sock);
+		kfree(nsock);
 		return -ENOMEM;
 	}

 	config->socks = socks;

-	nsock = kzalloc(sizeof(struct nbd_sock), GFP_KERNEL);
-	if (!nsock) {
-		sockfd_put(sock);
-		return -ENOMEM;
-	}
-
 	nsock->fallback_index = -1;
 	nsock->dead = false;
 	mutex_init(&nsock->tx_lock);
--
2.26.0.106.g9fadedd


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] nbd: Fix memory leak in nbd_add_socket
  2020-06-12  9:04 [PATCH] nbd: Fix memory leak in nbd_add_socket Zheng Bin
@ 2020-06-25  0:16 ` Chaitanya Kulkarni
  2020-06-28 15:58   ` Eric Biggers
  0 siblings, 1 reply; 3+ messages in thread
From: Chaitanya Kulkarni @ 2020-06-25  0:16 UTC (permalink / raw)
  To: Zheng Bin, josef, axboe, linux-block, nbd; +Cc: yi.zhang

On 6/12/20 1:57 AM, Zheng Bin wrote:
> nbd_add_socket
>    socks = krealloc(num_connections+1) -->if num_connections is 0, alloc 1
>    nsock = kzalloc                     -->If fail, will return
> 
> nbd_config_put
>    if (config->num_connections)        -->0, not free
>      kfree(config->socks)
> 
> Thus memleak happens, this patch fixes that.
> 
> Signed-off-by: Zheng Bin<zhengbin13@huawei.com>

Not an nbd expert but wouldn't it be easier use following which matches 
the + 1 in the nbd_add_socket() :-

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 01794cd2b6ca..e67c790039c9 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -1209,9 +1209,9 @@ static void nbd_config_put(struct nbd_device *nbd)
                         device_remove_file(disk_to_dev(nbd->disk), 
&pid_attr);
                 nbd->task_recv = NULL;
                 nbd_clear_sock(nbd);
-               if (config->num_connections) {
+               if (config->num_connections + 1) {
                         int i;
-                       for (i = 0; i < config->num_connections; i++) {
+                       for (i = 0; i < (config->num_connections + 1); 
i++) {
                                 sockfd_put(config->socks[i]->sock);
                                 kfree(config->socks[i]);
                         }

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] nbd: Fix memory leak in nbd_add_socket
  2020-06-25  0:16 ` Chaitanya Kulkarni
@ 2020-06-28 15:58   ` Eric Biggers
  0 siblings, 0 replies; 3+ messages in thread
From: Eric Biggers @ 2020-06-28 15:58 UTC (permalink / raw)
  To: Chaitanya Kulkarni; +Cc: Zheng Bin, josef, axboe, linux-block, nbd, yi.zhang

On Thu, Jun 25, 2020 at 12:16:33AM +0000, Chaitanya Kulkarni wrote:
> On 6/12/20 1:57 AM, Zheng Bin wrote:
> > nbd_add_socket
> >    socks = krealloc(num_connections+1) -->if num_connections is 0, alloc 1
> >    nsock = kzalloc                     -->If fail, will return
> > 
> > nbd_config_put
> >    if (config->num_connections)        -->0, not free
> >      kfree(config->socks)
> > 
> > Thus memleak happens, this patch fixes that.
> > 
> > Signed-off-by: Zheng Bin<zhengbin13@huawei.com>

This appears to address the syzbot report "memory leak in nbd_add_socket"
https://syzkaller.appspot.com/bug?id=08283193956ab772623e65242b3ed6d0e7c7d9ce
Can you please add:

	Reported-by: syzbot+934037347002901b8d2a@syzkaller.appspotmail.com

> 
> Not an nbd expert but wouldn't it be easier use following which matches 
> the + 1 in the nbd_add_socket() :-
> 
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index 01794cd2b6ca..e67c790039c9 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -1209,9 +1209,9 @@ static void nbd_config_put(struct nbd_device *nbd)
>                          device_remove_file(disk_to_dev(nbd->disk), 
> &pid_attr);
>                  nbd->task_recv = NULL;
>                  nbd_clear_sock(nbd);
> -               if (config->num_connections) {
> +               if (config->num_connections + 1) {
>                          int i;
> -                       for (i = 0; i < config->num_connections; i++) {
> +                       for (i = 0; i < (config->num_connections + 1); 
> i++) {
>                                  sockfd_put(config->socks[i]->sock);
>                                  kfree(config->socks[i]);
>                          }

That looks wrong.  The + 1 is just nbd_add_socket() preparing to append an entry
to the array.

Why not just check whether the pointer is NULL or not:

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 43cff01a5a67..cb8e86174edb 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -1206,7 +1206,7 @@ static void nbd_config_put(struct nbd_device *nbd)
 			device_remove_file(disk_to_dev(nbd->disk), &pid_attr);
 		nbd->task_recv = NULL;
 		nbd_clear_sock(nbd);
-		if (config->num_connections) {
+		if (config->socks) {
 			int i;
 			for (i = 0; i < config->num_connections; i++) {
 				sockfd_put(config->socks[i]->sock);

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-12  9:04 [PATCH] nbd: Fix memory leak in nbd_add_socket Zheng Bin
2020-06-25  0:16 ` Chaitanya Kulkarni
2020-06-28 15:58   ` Eric Biggers

Linux-Block Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-block/0 linux-block/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-block linux-block/ https://lore.kernel.org/linux-block \
		linux-block@vger.kernel.org
	public-inbox-index linux-block

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-block


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git