[PATCH] netlink: fix memory leak

[PATCH] netlink: fix memory leak

[Shaochun Chen]

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 <cscnull@gmail.com>
---
 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(-)

diff --git a/include/linux/netlink.h b/include/linux/netlink.h
index f3075d6c7e82..9d6b3edc5a5b 100644
--- a/include/linux/netlink.h
+++ b/include/linux/netlink.h
@@ -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;
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 896d4a36081d..dc30a329f785 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -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);
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 393573a99a5a..7b85176cf9bb 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -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);
-- 
2.17.1

Re: [PATCH] netlink: fix memory leak

[Jason A. Donenfeld]

On Sun, Jul 22, 2018 at 4:51 AM Shaochun Chen <cscnull@gmail.com> wrote:
> diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
> index 393573a99a5a..7b85176cf9bb 100644
> --- a/net/netlink/af_netlink.c
> +++ b/net/netlink/af_netlink.c
> @@ -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);

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.


>         return ret;
>  }
>  EXPORT_SYMBOL(__netlink_dump_start);
> --
> 2.17.1
>

Re: [PATCH] netlink: fix memory leak

[Florian Westphal]

Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> On Sun, Jul 22, 2018 at 4:51 AM Shaochun Chen <cscnull@gmail.com> 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.

Re: [PATCH] netlink: fix memory leak

[Florian Westphal]

Shaochun Chen <cscnull@gmail.com> wrote:

[ CC Tom Herbert ]

> 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!