Skip to contentSkip to conversation

netlink: fix memory leak

PATCH In review Shaochun Chen 3 participants 4 messages Jul 22, 2018

Subsystems (3)
Networking Network drivers netfilter
lore
Share & tools
lore

Shaochun Chen

author
#0 lore raw

PATCH 1/1 netlink: fix memory leak

when netlink_dump start failed, netlink_callback will not be called, and the memory which pointed by control->data will leak. so if netlink_dump start fail, call control->done to free the memory.

Signed-off-by Shaochun Chen <[email protected]>
---
 include/linux/netlink.h       | 10 ++++++++++
 net/netfilter/nf_tables_api.c |  4 +++-
 net/netlink/af_netlink.c      |  4 ++++
 3 files changed, 17 insertions(+), 1 deletion(-)
include/linux/netlink.h +10 -0
index f3075d6c7e82..9d6b3edc5a5b 100644
@@ -214,6 +214,16 @@ static inline int netlink_dump_start(struct sock *ssk, struct sk_buff *skb,
 	return __netlink_dump_start(ssk, skb, nlh, control);
 }
 
+static inline void netlink_dump_start_fail(struct netlink_dump_control *control)
+{
+	struct netlink_callback cb = {
+		.data = control->data,
+	};
+
+	if (control->done)
+		control->done(&cb);
+}
+
 struct netlink_tap {
 	struct net_device *dev;
 	struct module *module;
net/netfilter/nf_tables_api.c +3 -1
index 896d4a36081d..dc30a329f785 100644
@@ -588,8 +588,10 @@ static int nft_netlink_dump_start_rcu(struct sock *nlsk, struct sk_buff *skb,
 {
 	int err;
 
-	if (!try_module_get(THIS_MODULE))
+	if (!try_module_get(THIS_MODULE)) {
+		netlink_dump_start_fail(c);
 		return -EINVAL;
+	}
 
 	rcu_read_unlock();
 	err = netlink_dump_start(nlsk, skb, nlh, c);
net/netlink/af_netlink.c +4 -0
index 393573a99a5a..7b85176cf9bb 100644
@@ -2275,6 +2275,7 @@ int __netlink_dump_start(struct sock *ssk, struct sk_buff *skb,
 	struct netlink_callback *cb;
 	struct sock *sk;
 	struct netlink_sock *nlk;
+	bool cb_running = false;
 	int ret;
 
 	refcount_inc(&skb->users);

@@ -2317,6 +2318,7 @@ int __netlink_dump_start(struct sock *ssk, struct sk_buff *skb,
 
 	nlk->cb_running = true;
 	nlk->dump_done_errno = INT_MAX;
+	cb_running = true;
 
 	mutex_unlock(nlk->cb_mutex);
 

@@ -2339,6 +2341,8 @@ int __netlink_dump_start(struct sock *ssk, struct sk_buff *skb,
 	mutex_unlock(nlk->cb_mutex);
 error_free:
 	kfree_skb(skb);
+	if (cb_running)
+		netlink_dump_start_fail(control);
 	return ret;
 }
 EXPORT_SYMBOL(__netlink_dump_start);
Signature · 1 line
2.17.1
#1 lore raw
Quoted 21 lines · Shaochun Chen
net/netlink/af_netlink.c +2 -0
index 393573a99a5a..7b85176cf9bb 100644
@@ -2275,6 +2275,7 @@ int __netlink_dump_start(struct sock *ssk, struct sk_buff *skb,
        struct netlink_callback *cb;
        struct sock *sk;
        struct netlink_sock *nlk;
+       bool cb_running = false;
        int ret;
 
        refcount_inc(&skb->users);

@@ -2317,6 +2318,7 @@ int __netlink_dump_start(struct sock *ssk, struct sk_buff *skb,
 
        nlk->cb_running = true;
        nlk->dump_done_errno = INT_MAX;
+       cb_running = true;
 
        mutex_unlock(nlk->cb_mutex);

net/netlink/af_netlink.c int __netlink_dump_start(struct sock *ssk, struct sk_buff *skb, · line 2345

Quoted 5 lines
net/netlink/af_netlink.c +2 -0
@@ -2339,6 +2341,8 @@ int __netlink_dump_start(struct sock *ssk, struct sk_buff *skb,
        mutex_unlock(nlk->cb_mutex);
 error_free:
        kfree_skb(skb);
+       if (cb_running)
+               netlink_dump_start_fail(control);

cb_running is never true here, since nothing jumps to error_free after you set it to be true. Pasting more code for context:

       nlk->cb_running = true;
       nlk->dump_done_errno = INT_MAX;

1) ----> cb_runnning = true;

       mutex_unlock(nlk->cb_mutex);

       ret = netlink_dump(sk);

       sock_put(sk);

       if (ret)

A) return ret;

       /* We successfully started a dump, by returning -EINTR we
        * signal not to send ACK even if it was requested.
        */

B) return -EINTR;

error_put:

       module_put(control->module);
error_unlock:
       sock_put(sk);
       mutex_unlock(nlk->cb_mutex);
error_free:
       kfree_skb(skb);
2) ----> if (cb_running) netlink_dump_start_fail(control);
       return ret;

After (1) is set, the function exits via (A) or (B), and so (2) is never hit.

But even if you moved it somehow to the if(ret), I'm still not sure it'd be correct; start cbs should either succeed, or they should error out and cleanup entirely after themselves.

Quoted 6 lines
net/netlink/af_netlink.c +0 -1
        return ret;
 }
 EXPORT_SYMBOL(__netlink_dump_start);
--

2.17.1

maintainer
#2 lore raw
Quoted 4 lines · Jason A. Donenfeld

On Sun, Jul 22, 2018 at 4:51 AM Shaochun Chen <[email protected]> wrote:
But even if you moved it somehow to the if(ret), I'm still not sure it'd be correct; start cbs should either succeed, or they should error out and cleanup entirely after themselves.

I agree, ->done() should not be called if cb->dump() was never invoked.

All ctnetlink and nf_tables spots need to cleanup after themselves.

maintainer
#3 lore raw
In reply to Shaochun Chen

Shaochun Chen <[email protected]> wrote:

[ CC Tom Herbert ]

Quoted 2 lines

and the memory which pointed by control->data will leak. so if netlink_dump start fail, call control->done to free the memory.

Tom, I was about to suggest moving extra allocations for dumps into a ->start() callback whereever possible.

However, it looks like ->done() is not guaranteed to be called even if ->start() was invoked, but it seems at least ila assumes ->done always cleans up after ->start.

I am looking at netlink_dump(); it calls ->done() only after the dump callback was invoked.

In nf_tables_api.c case it might be possible to defer allocations until ->dump() is called for first time via cb_args but I don't think its going to be any better than cleaning up manually after netlink_dump_start() returned an error.

Any better ideas or advice on how to procceed?

Thanks!