mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* Re: [PATCH] net: Fix potential memory leak in proto_register()
@ 2020-08-12  9:21 linmiaohe
  2020-08-12 17:57 ` Cong Wang
  0 siblings, 1 reply; 6+ messages in thread
From: linmiaohe @ 2020-08-12  9:21 UTC (permalink / raw)
  To: David Miller, xiyou.wangcong
  Cc: kuba, edumazet, kafai, daniel, jakub, keescook, zhang.lin16,
	netdev, linux-kernel

Hi all:
David Miller <davem@davemloft.net> wrote:
>From: Cong Wang <xiyou.wangcong@gmail.com>
>Date: Tue, 11 Aug 2020 16:02:51 -0700
>
>>> @@ -3406,6 +3406,16 @@ static void sock_inuse_add(struct net *net, 
>>> int val)  }  #endif
>>>
>>> +static void tw_prot_cleanup(struct timewait_sock_ops *twsk_prot) {
>>> +       if (!twsk_prot)
>>> +               return;
>>> +       kfree(twsk_prot->twsk_slab_name);
>>> +       twsk_prot->twsk_slab_name = NULL;
>>> +       kmem_cache_destroy(twsk_prot->twsk_slab);
>> 
>> Hmm, are you sure you can free the kmem cache name before 
>> kmem_cache_destroy()? To me, it seems kmem_cache_destroy() frees the 
>> name via slab_kmem_cache_release() via kfree_const().
>> With your patch, we have a double-free on the name?
>> 
>> Or am I missing anything?
>
>Yep, there is a double free here.
>
>Please fix this.

Many thanks for both of you to point this issue out. But I'am not really understand, could you please explain it more?
As far as I can see, the double free path is:
1. kfree(twsk_prot->twsk_slab_name)
2. kmem_cache_destroy 
	--> shutdown_memcg_caches
		--> shutdown_cache
			--> slab_kmem_cache_release
				--> kfree_const(s->name)
But twsk_prot->twsk_slab_name is allocated from kasprintf via kmalloc_track_caller while twsk_prot->twsk_slab->name is allocated 
via kstrdup_const. So I think twsk_prot->twsk_slab_name and twsk_prot->twsk_slab->name point to different memory, and there is no double free.

Or am I missing anything?

By the way, req_prot_cleanup() do the same things, i.e. free the slab_name before involve kmem_cache_destroy(). If there is a double
free, so as here?

Thanks.


^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH] net: Fix potential memory leak in proto_register()
@ 2020-08-10 12:16 Miaohe Lin
  2020-08-11 22:37 ` David Miller
  2020-08-11 23:02 ` Cong Wang
  0 siblings, 2 replies; 6+ messages in thread
From: Miaohe Lin @ 2020-08-10 12:16 UTC (permalink / raw)
  To: davem, kuba, edumazet, kafai, daniel, jakub, keescook, zhang.lin16
  Cc: netdev, linux-kernel, linmiaohe

If we failed to assign proto idx, we free the twsk_slab_name but forget to
free the twsk_slab. Add a helper function tw_prot_cleanup() to free these
together and also use this helper function in proto_unregister().

Fixes: b45ce32135d1 ("sock: fix potential memory leak in proto_register()")
Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 net/core/sock.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/net/core/sock.c b/net/core/sock.c
index 49cd5ffe673e..c9083ad44ea1 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -3406,6 +3406,16 @@ static void sock_inuse_add(struct net *net, int val)
 }
 #endif
 
+static void tw_prot_cleanup(struct timewait_sock_ops *twsk_prot)
+{
+	if (!twsk_prot)
+		return;
+	kfree(twsk_prot->twsk_slab_name);
+	twsk_prot->twsk_slab_name = NULL;
+	kmem_cache_destroy(twsk_prot->twsk_slab);
+	twsk_prot->twsk_slab = NULL;
+}
+
 static void req_prot_cleanup(struct request_sock_ops *rsk_prot)
 {
 	if (!rsk_prot)
@@ -3476,7 +3486,7 @@ int proto_register(struct proto *prot, int alloc_slab)
 						  prot->slab_flags,
 						  NULL);
 			if (prot->twsk_prot->twsk_slab == NULL)
-				goto out_free_timewait_sock_slab_name;
+				goto out_free_timewait_sock_slab;
 		}
 	}
 
@@ -3484,15 +3494,15 @@ int proto_register(struct proto *prot, int alloc_slab)
 	ret = assign_proto_idx(prot);
 	if (ret) {
 		mutex_unlock(&proto_list_mutex);
-		goto out_free_timewait_sock_slab_name;
+		goto out_free_timewait_sock_slab;
 	}
 	list_add(&prot->node, &proto_list);
 	mutex_unlock(&proto_list_mutex);
 	return ret;
 
-out_free_timewait_sock_slab_name:
+out_free_timewait_sock_slab:
 	if (alloc_slab && prot->twsk_prot)
-		kfree(prot->twsk_prot->twsk_slab_name);
+		tw_prot_cleanup(prot->twsk_prot);
 out_free_request_sock_slab:
 	if (alloc_slab) {
 		req_prot_cleanup(prot->rsk_prot);
@@ -3516,12 +3526,7 @@ void proto_unregister(struct proto *prot)
 	prot->slab = NULL;
 
 	req_prot_cleanup(prot->rsk_prot);
-
-	if (prot->twsk_prot != NULL && prot->twsk_prot->twsk_slab != NULL) {
-		kmem_cache_destroy(prot->twsk_prot->twsk_slab);
-		kfree(prot->twsk_prot->twsk_slab_name);
-		prot->twsk_prot->twsk_slab = NULL;
-	}
+	tw_prot_cleanup(prot->twsk_prot);
 }
 EXPORT_SYMBOL(proto_unregister);
 
-- 
2.19.1


^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-08-12 17:57 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-12  9:21 [PATCH] net: Fix potential memory leak in proto_register() linmiaohe
2020-08-12 17:57 ` Cong Wang
  -- strict thread matches above, loose matches on Subject: below --
2020-08-10 12:16 Miaohe Lin
2020-08-11 22:37 ` David Miller
2020-08-11 23:02 ` Cong Wang
2020-08-11 23:10   ` David Miller

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®