| Message ID | 20200612090437.77977-1-zhengbin13@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | nbd: Fix memory leak in nbd_add_socket | expand |
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]); }
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);
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);
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(-) -- 2.26.0.106.g9fadedd