netlink: fix memory leak of dump

PATCH In review Shaochun Chen 4 participants 15 messages Jul 22, 2018 – Jul 23, 2018

Subsystems (3)

Networking Network drivers netfilter

lore

Share & tools

lore

Shaochun Chen

author

2018-07-22 14:33 UTC #0 lore raw

PATCH 1/1 netlink: fix memory leak of dump

1) if netlink_dump_start start fail, the memory of c->data will leak. so free manually after netlink_dump_start return error.

2) In netlink_dump_start, ignore the return of netlink_dump. Because if cb_running is set to true, cb->dump will be call in anyway. so if netlink_dump_start start successfully, just return -EINTR.

Signed-off-by Shaochun Chen <[email protected]>

---
 net/netfilter/nf_conntrack_netlink.c |  8 ++++--
 net/netfilter/nf_tables_api.c        | 41 ++++++++++++++++++++--------
 net/netfilter/nfnetlink_acct.c       |  8 ++++--
 net/netlink/af_netlink.c             |  5 +---
 4 files changed, 41 insertions(+), 21 deletions(-)

net/netfilter/nf_conntrack_netlink.c +5 -3

index 20a2e37c76d1..31db758bdb7a 100644
@@ -1243,17 +1243,19 @@ static int ctnetlink_get_conntrack(struct net *net, struct sock *ctnl,
 			.dump = ctnetlink_dump_table,
 			.done = ctnetlink_done,
 		};
+		struct ctnetlink_filter *filter = NULL;
 
 		if (cda[CTA_MARK] && cda[CTA_MARK_MASK]) {
-			struct ctnetlink_filter *filter;
-
 			filter = ctnetlink_alloc_filter(cda);
 			if (IS_ERR(filter))
 				return PTR_ERR(filter);
 
 			c.data = filter;
 		}
-		return netlink_dump_start(ctnl, skb, nlh, &c);
+		err = netlink_dump_start(ctnl, skb, nlh, &c);
+		if (err != -EINTR && filter)
+			kfree(filter);
+		return err;
 	}
 
 	err = ctnetlink_parse_zone(cda[CTA_ZONE], &zone);

net/netfilter/nf_tables_api.c +30 ~~-11~~

96 lines not on this page · raw message

net/netfilter/nfnetlink_acct.c +5 -3

index a0e5adf0b3b6..aecdb9cc1c5d 100644
@@ -277,17 +277,19 @@ static int nfnl_acct_get(struct net *net, struct sock *nfnl,
 			.dump = nfnl_acct_dump,
 			.done = nfnl_acct_done,
 		};
+		struct nfacct_filter *filter = NULL;
 
 		if (tb[NFACCT_FILTER]) {
-			struct nfacct_filter *filter;
-
 			filter = nfacct_filter_alloc(tb[NFACCT_FILTER]);
 			if (IS_ERR(filter))
 				return PTR_ERR(filter);
 
 			c.data = filter;
 		}
-		return netlink_dump_start(nfnl, skb, nlh, &c);
+		ret = netlink_dump_start(nfnl, skb, nlh, &c);
+		if (ret != -EINTR && filter)
+			kfree(filter);
+		return ret;
 	}
 
 	if (!tb[NFACCT_NAME])

net/netlink/af_netlink.c +1 -4

index 393573a99a5a..61450461378c 100644
@@ -2320,13 +2320,10 @@ int __netlink_dump_start(struct sock *ssk, struct sk_buff *skb,
 
 	mutex_unlock(nlk->cb_mutex);
 
-	ret = netlink_dump(sk);
+	netlink_dump(sk);
 
 	sock_put(sk);
 
-	if (ret)
-		return ret;
-
 	/* We successfully started a dump, by returning -EINTR we
 	 * signal not to send ACK even if it was requested.
 	 */

Signature · 1 line

2.17.1

Florian Westphal

maintainer

2018-07-22 16:39 UTC #1 lore raw

Shaochun Chen <[email protected]> wrote:

[ CC ing Tom, as he added ->start() ]

Quoted 6 lines

1) if netlink_dump_start start fail, the memory of c->data will leak. so free manually after netlink_dump_start return error.
2) In netlink_dump_start, ignore the return of netlink_dump. Because if cb_running is set to true, cb->dump will be call in anyway. so if netlink_dump_start start successfully, just return -EINTR.

This now requires ->start() to not return -EINVAL, else we won't release resources either, this seems fragile to me.

I can only think of three ways to address this:

1. Kill ->start() and force all users to defer state allocation to first ->dump() invocation, so existing ->done() can be used to release resources.

OR
2. add ->stop() and have core always pair it with ->start(), no matter if dump() got called or not, then convert all places that provide .start to use .stop, not .done.

OR
3. change meaning of ->done() so its always called once ->start() was invoked (and returned 0), this requires audit of all places that provide .done to make sure they won't trip.

3) seems to be what Tom intended when he added .start, so probably best to investigate that first.

David Miller

maintainer

2018-07-22 17:07 UTC #2 lore raw

From: Florian Westphal <[email protected]>
Date: Sun, 22 Jul 2018 18:39:25 +0200

Quoted 6 lines

3. change meaning of ->done() so its always called once ->start() was invoked (and returned 0), this requires audit of all places that provide .done to make sure they won't trip.
3) seems to be what Tom intended when he added .start, so probably best to investigate that first.

Hmmm...

Any time ->start() succeeds, we set cb_running to true.

Quoted 1 line

From that point forward, ->done() will be called at some point at all

of the locations that check if cb_running is true and set it to false.

This may be deferred all the way to socket destruction, but it will eventually happens.

Nothing sets cb_running to false without invoking the ->done() callback.

Just because the individual dump invocations don't call ->done() does not mean it will not eventually happen. The dump callbacks, and thus the state data, is live across multiple rounds of recvmsg() calls by the user and that is as-designed.

Florian Westphal

maintainer

2018-07-22 18:09 UTC #3 lore raw

Quoted 7 lines · David Miller

From: Florian Westphal <[email protected]>
Date: Sun, 22 Jul 2018 18:39:25 +0200
Quoted 4 lines
3. change meaning of ->done() so its always called once ->start() was invoked (and returned 0), this requires audit of all places that provide .done to make sure they won't trip.

Quoted 6 lines

Quoted 2 lines
3) seems to be what Tom intended when he added .start, so probably best to investigate that first.
Hmmm...
Any time ->start() succeeds, we set cb_running to true.

Right.

Quoted 2 lines

From that point forward, ->done() will be called at some point at all of the locations that check if cb_running is true and set it to false.

Also right, thanks for pointing this out, I missed fact that netlink core restarts a dump after this.

So 3) is already true which means we should try to see if we can move all dump-related extra magic into ->start().

Shaochun, can you see if this is possible?

Something along these lines (totally untested), which makes this a netfilter fix:

net/netfilter/nf_tables_api.c +18 ~~-10~~

@@ -5010,6 +5013,22 @@ nft_obj_filter_alloc(const struct nlattr * const nla[])
 	return filter;
 }
 
+static int nf_tables_dump_obj_start(struct netlink_callback *cb)
+{
+	const struct nlattr * const *nla = cb->data;
+	struct nft_obj_filter *filter = NULL;
+
+	if (nla[NFTA_OBJ_TABLE] ||
+	    nla[NFTA_OBJ_TYPE]) {
+		filter = nft_obj_filter_alloc(nla);
+		if (IS_ERR(filter))
+			return -ENOMEM;
+	}
+
+	cb->data = filter;
+	return 0;
+}
+
 /* called with rcu_read_lock held */
 static int nf_tables_getobj(struct net *net, struct sock *nlsk,
 			    struct sk_buff *skb, const struct nlmsghdr *nlh,

@@ -5028,21 +5047,13 @@ static int nf_tables_getobj(struct net *net, struct sock *nlsk,
 
 	if (nlh->nlmsg_flags & NLM_F_DUMP) {
 		struct netlink_dump_control c = {
+			.start = nf_tables_dump_obj_start,
 			.dump = nf_tables_dump_obj,
 			.done = nf_tables_dump_obj_done,
 			.module = THIS_MODULE,
+			.data = (void *)nla,
 		};
 
-		if (nla[NFTA_OBJ_TABLE] ||
-		    nla[NFTA_OBJ_TYPE]) {
-			struct nft_obj_filter *filter;
-
-			filter = nft_obj_filter_alloc(nla);
-			if (IS_ERR(filter))
-				return -ENOMEM;
-
-			c.data = filter;
-		}
 		return nft_netlink_dump_start_rcu(nlsk, skb, nlh, &c);
 	}

shaochun chen

author

2018-07-23 02:05 UTC #4 lore raw

allocate memory in cb->start(), which means passing 'static' variable through control->data,
then allocate memory in cb->start() according to cb->data (cb->data is equal to control->data now),
and set the memory back to cb->data which will be used in cb->dump(). It's a bit complicated， please see nf_tables_getset.

2018-07-23 2:09 GMT+08:00 Florian Westphal <[email protected]>:

Quoted 85 lines

Quoted 7 lines · David Miller
From: Florian Westphal <[email protected]>
Date: Sun, 22 Jul 2018 18:39:25 +0200
Quoted 4 lines
3. change meaning of ->done() so its always called once ->start() was invoked (and returned 0), this requires audit of all places that provide .done to make sure they won't trip.
Quoted 6 lines
Quoted 2 lines
3) seems to be what Tom intended when he added .start, so probably best to investigate that first.
Hmmm...
Any time ->start() succeeds, we set cb_running to true.
Right.
Quoted 2 lines
From that point forward, ->done() will be called at some point at all of the locations that check if cb_running is true and set it to false.
Also right, thanks for pointing this out, I missed fact that netlink core restarts a dump after this.
So 3) is already true which means we should try to see if we can move all dump-related extra magic into ->start().
Shaochun, can you see if this is possible?
Something along these lines (totally untested), which makes this a netfilter fix:
net/netfilter/nf_tables_api.c +16 -0
@@ -5010,6 +5013,22 @@ nft_obj_filter_alloc(const struct nlattr * const
 nla[])
        return filter;
 }
 
+static int nf_tables_dump_obj_start(struct netlink_callback *cb)
+{
+       const struct nlattr * const *nla = cb->data;
+       struct nft_obj_filter *filter = NULL;
+
+       if (nla[NFTA_OBJ_TABLE] ||
+           nla[NFTA_OBJ_TYPE]) {
+               filter = nft_obj_filter_alloc(nla);
+               if (IS_ERR(filter))
+                       return -ENOMEM;
+       }
+
+       cb->data = filter;
+       return 0;
+}
+
 /* called with rcu_read_lock held */
 static int nf_tables_getobj(struct net *net, struct sock *nlsk,
struct sk_buff *skb, const struct nlmsghdr
*nlh,
net/netfilter/nf_tables_api.c +2 ~~-10~~
@@ -5028,21 +5047,13 @@ static int nf_tables_getobj(struct net *net,
 struct sock *nlsk,
 
        if (nlh->nlmsg_flags & NLM_F_DUMP) {
                struct netlink_dump_control c = {
+                       .start = nf_tables_dump_obj_start,
                        .dump = nf_tables_dump_obj,
                        .done = nf_tables_dump_obj_done,
                        .module = THIS_MODULE,
+                       .data = (void *)nla,
                };
 
-               if (nla[NFTA_OBJ_TABLE] ||
-                   nla[NFTA_OBJ_TYPE]) {
-                       struct nft_obj_filter *filter;
-
-                       filter = nft_obj_filter_alloc(nla);
-                       if (IS_ERR(filter))
-                               return -ENOMEM;
-
-                       c.data = filter;
-               }
                return nft_netlink_dump_start_rcu(nlsk, skb, nlh, &c);
        }

Pablo Neira Ayuso

maintainer

2018-07-23 09:15 UTC #5 lore raw

In reply to Florian Westphal

Quoted 58 lines · Florian Westphal

Quoted 7 lines · David Miller
From: Florian Westphal <[email protected]>
Date: Sun, 22 Jul 2018 18:39:25 +0200
Quoted 4 lines
3. change meaning of ->done() so its always called once ->start() was invoked (and returned 0), this requires audit of all places that provide .done to make sure they won't trip.
Quoted 6 lines
Quoted 2 lines
3) seems to be what Tom intended when he added .start, so probably best to investigate that first.
Hmmm...
Any time ->start() succeeds, we set cb_running to true.
Right.
Quoted 2 lines
From that point forward, ->done() will be called at some point at all of the locations that check if cb_running is true and set it to false.
Also right, thanks for pointing this out, I missed fact that netlink core restarts a dump after this.
So 3) is already true which means we should try to see if we can move all dump-related extra magic into ->start().
Shaochun, can you see if this is possible?
Something along these lines (totally untested), which makes this a netfilter fix:
net/netfilter/nf_tables_api.c +16 -0
@@ -5010,6 +5013,22 @@ nft_obj_filter_alloc(const struct nlattr * const nla[])
 	return filter;
 }
 
+static int nf_tables_dump_obj_start(struct netlink_callback *cb)
+{
+	const struct nlattr * const *nla = cb->data;
+	struct nft_obj_filter *filter = NULL;
+
+	if (nla[NFTA_OBJ_TABLE] ||
+	    nla[NFTA_OBJ_TYPE]) {
+		filter = nft_obj_filter_alloc(nla);
+		if (IS_ERR(filter))
+			return -ENOMEM;
+	}
+
+	cb->data = filter;
+	return 0;
+}
+
 /* called with rcu_read_lock held */
 static int nf_tables_getobj(struct net *net, struct sock *nlsk,
 			    struct sk_buff *skb, const struct nlmsghdr *nlh,

net/netfilter/nf_tables_api.c static int nf_tables_getobj(struct net *net, struct sock *nlsk, · line 5054

Quoted 8 lines

net/netfilter/nf_tables_api.c +2 -0

@@ -5028,21 +5047,13 @@ static int nf_tables_getobj(struct net *net, struct sock *nlsk,
 
 	if (nlh->nlmsg_flags & NLM_F_DUMP) {
 		struct netlink_dump_control c = {
+			.start = nf_tables_dump_obj_start,
 			.dump = nf_tables_dump_obj,
 			.done = nf_tables_dump_obj_done,
 			.module = THIS_MODULE,
+			.data = (void *)nla,

You cannot do this.

nla is allocated in this stack.

the nla will not be available in the second recv(), it won't be there.

Florian Westphal

maintainer

2018-07-23 09:28 UTC #6 lore raw

Quoted 10 lines · Pablo Neira Ayuso

net/netfilter/nf_tables_api.c nft_obj_filter_alloc(const struct nlattr * const nla[]) · line 5018
Quoted 10 lines
net/netfilter/nf_tables_api.c +3 -0
@@ -5010,6 +5013,22 @@ nft_obj_filter_alloc(const struct nlattr * const nla[])
 	return filter;
 }
 
+static int nf_tables_dump_obj_start(struct netlink_callback *cb)
+{
+	const struct nlattr * const *nla = cb->data;

On-Stack input.
I can't see how its wrong, ->start() happens from same context as netlink_dump_start so its valid.

Quoted 10 lines

net/netfilter/nf_tables_api.c nft_obj_filter_alloc(const struct nlattr * const nla[]) · line 5028
Quoted 10 lines
net/netfilter/nf_tables_api.c +10 -0
+	struct nft_obj_filter *filter = NULL;
+
+	if (nla[NFTA_OBJ_TABLE] ||
+	    nla[NFTA_OBJ_TYPE]) {
+		filter = nft_obj_filter_alloc(nla);
+		if (IS_ERR(filter))
+			return -ENOMEM;
+	}
+
+	cb->data = filter;

And this replaced the on-stack input with dynamically allocated one, which will be free'd via ->done().

Quoted 15 lines

net/netfilter/nf_tables_api.c static int nf_tables_getobj(struct net *net, struct sock *nlsk, · line 5054

Quoted 12 lines

net/netfilter/nf_tables_api.c +2 -0

 /* called with rcu_read_lock held */
 static int nf_tables_getobj(struct net *net, struct sock *nlsk,
 			    struct sk_buff *skb, const struct nlmsghdr *nlh,

@@ -5028,21 +5047,13 @@ static int nf_tables_getobj(struct net *net, struct sock *nlsk,
 
 	if (nlh->nlmsg_flags & NLM_F_DUMP) {
 		struct netlink_dump_control c = {
+			.start = nf_tables_dump_obj_start,
 			.dump = nf_tables_dump_obj,
 			.done = nf_tables_dump_obj_done,
 			.module = THIS_MODULE,
+			.data = (void *)nla,

You cannot do this.

Quoted 1 line

nla is allocated in this stack.

Yes.

Quoted 1 line

the nla will not be available in the second recv(), it won't be there.

Its replaced in ->start().

As David pointed out, once ->start() returns 0 we set cb_running, i.e. only after successful ->start() netlink core will call ->dump() again.

So I see no problem setting ->data to onstack cookie and then duplicating it to heap via kmemdup in ->start().

As far as I can see netlink core offers all functionality already, so we only need to switch netfilter to make use of it.

If you disagree please let me know, otherwise I will cook up a patch along this pattern for net/netfilter/*.

Thanks.

Pablo Neira Ayuso

maintainer

2018-07-23 09:38 UTC #7 lore raw

Quoted 56 lines · Florian Westphal

Quoted 10 lines · Pablo Neira Ayuso
net/netfilter/nf_tables_api.c nft_obj_filter_alloc(const struct nlattr * const nla[]) · line 5018
Quoted 10 lines
net/netfilter/nf_tables_api.c +3 -0
@@ -5010,6 +5013,22 @@ nft_obj_filter_alloc(const struct nlattr * const nla[])
 	return filter;
 }
 
+static int nf_tables_dump_obj_start(struct netlink_callback *cb)
+{
+	const struct nlattr * const *nla = cb->data;
On-Stack input.
I can't see how its wrong, ->start() happens from same context as netlink_dump_start so its valid.
Quoted 10 lines
net/netfilter/nf_tables_api.c nft_obj_filter_alloc(const struct nlattr * const nla[]) · line 5028
Quoted 10 lines
net/netfilter/nf_tables_api.c +10 -0
+	struct nft_obj_filter *filter = NULL;
+
+	if (nla[NFTA_OBJ_TABLE] ||
+	    nla[NFTA_OBJ_TYPE]) {
+		filter = nft_obj_filter_alloc(nla);
+		if (IS_ERR(filter))
+			return -ENOMEM;
+	}
+
+	cb->data = filter;
And this replaced the on-stack input with dynamically allocated one, which will be free'd via ->done().
Quoted 15 lines
net/netfilter/nf_tables_api.c static int nf_tables_getobj(struct net *net, struct sock *nlsk, · line 5054
Quoted 12 lines
net/netfilter/nf_tables_api.c +2 -0
 /* called with rcu_read_lock held */
 static int nf_tables_getobj(struct net *net, struct sock *nlsk,
 			    struct sk_buff *skb, const struct nlmsghdr *nlh,
@@ -5028,21 +5047,13 @@ static int nf_tables_getobj(struct net *net, struct sock *nlsk,
 
 	if (nlh->nlmsg_flags & NLM_F_DUMP) {
 		struct netlink_dump_control c = {
+			.start = nf_tables_dump_obj_start,
 			.dump = nf_tables_dump_obj,
 			.done = nf_tables_dump_obj_done,
 			.module = THIS_MODULE,
+			.data = (void *)nla,
You cannot do this.
Quoted 1 line
nla is allocated in this stack.
Yes.
Quoted 1 line
the nla will not be available in the second recv(), it won't be there.
Its replaced in ->start().
As David pointed out, once ->start() returns 0 we set cb_running, i.e. only after successful ->start() netlink core will call ->dump() again.

Quoted 8 lines

So I see no problem setting ->data to onstack cookie and then duplicating it to heap via kmemdup in ->start().
As far as I can see netlink core offers all functionality already, so we only need to switch netfilter to make use of it.
If you disagree please let me know, otherwise I will cook up a patch along this pattern for net/netfilter/*.

Why not just call ->done from netlink_dump_start() when it fails?

Florian Westphal

maintainer

2018-07-23 09:42 UTC #8 lore raw

Quoted 6 lines · Pablo Neira Ayuso

Quoted 6 lines
As David pointed out, once ->start() returns 0 we set cb_running, i.e. only after successful ->start() netlink core will call ->dump() again.
So I see no problem setting ->data to onstack cookie and then duplicating it to heap via kmemdup in ->start().

Quoted 7 lines

Quoted 5 lines
As far as I can see netlink core offers all functionality already, so we only need to switch netfilter to make use of it.
If you disagree please let me know, otherwise I will cook up a patch along this pattern for net/netfilter/*.
Why not just call ->done from netlink_dump_start() when it fails?

Not sure thats safe for all users, we will also still need to call it in nft_netlink_dump_start and we need to play guess game wrt EINTR (which can mean 'dump was now started, do not send ack').

Pablo Neira Ayuso

maintainer

2018-07-23 09:47 UTC #9 lore raw

Quoted 10 lines · Florian Westphal

Quoted 9 lines · Pablo Neira Ayuso
Quoted 9 lines
As David pointed out, once ->start() returns 0 we set cb_running, i.e. only after successful ->start() netlink core will call ->dump() again.
So I see no problem setting ->data to onstack cookie and then duplicating it to heap via kmemdup in ->start().
As far as I can see netlink core offers all functionality already, so we only need to switch netfilter to make use of it.

Quoted 8 lines

Quoted 4 lines
Quoted 2 lines
If you disagree please let me know, otherwise I will cook up a patch along this pattern for net/netfilter/*.
Why not just call ->done from netlink_dump_start() when it fails?
Not sure thats safe for all users, we will also still need to call it in nft_netlink_dump_start and we need to play guess game wrt EINTR (which can mean 'dump was now started, do not send ack').

We can also add another scratchpad area, similar to the ->cb[x] area that can be initialized before netlink_dump_start()? So we don't need the data pointer.

By passing the array of attributes, we'll need to do attribute parsing over and over again from each netlink_dump() call.

Florian Westphal

maintainer

2018-07-23 10:51 UTC #10 lore raw

Quoted 7 lines · Pablo Neira Ayuso

On Mon, Jul 23, 2018 at 11:42:28AM +0200, Florian Westphal wrote:
We can also add another scratchpad area, similar to the ->cb[x] area that can be initialized before netlink_dump_start()? So we don't need the data pointer.
By passing the array of attributes, we'll need to do attribute parsing over and over again from each netlink_dump() call.

Its still done once, parsing is only delayed/moved to ->start().

Now:

nla_parse
control->data = kmalloc() + init
netlink_dump_start()
->start() /* if it returns 0, done() will be called eventually netlink_dump()
->done()
free(cb->data);

Proposed fix:
control->data = &on_stack;
netlink_dump_start()
->start()

     nla_parse
     cb->data = kmalloc() + init

netlink_dump()
->done()
free(cb->data);

I've submitted a patch for nf_tables, let me know if you have a better idea or see a problem with it.

shaochun chen

author

2018-07-23 09:52 UTC #11 lore raw

In reply to Florian Westphal

allocate memory in ->start(), it's not convenient for users. if call ->done() isn't ok for clean memory when netlink_dump_start() fail, maybe we should have another function ->clean() to clean memory.

2018-07-23 17:28 GMT+08:00 Florian Westphal <[email protected]>:

Quoted 71 lines

Quoted 1 line · Pablo Neira Ayuso
Review
Quoted 1 line
(quoted diff) +0 -0
b/net/netfilter/nf_tables_api.c
Quoted 3 lines
net/netfilter/nf_tables_api.c nft_obj_filter_alloc(const struct nlattr *
Quoted 3 lines
net/netfilter/nf_tables_api.c +0 -0
@@ -5010,6 +5013,22 @@ nft_obj_filter_alloc(const struct nlattr *
const nla[])
Quoted 6 lines
net/netfilter/nf_tables_api.c nft_obj_filter_alloc(const struct nlattr * const nla[]) · line 5018
Quoted 6 lines
net/netfilter/nf_tables_api.c +3 -0
    return filter;
 }
 
+static int nf_tables_dump_obj_start(struct netlink_callback *cb)
+{
+   const struct nlattr * const *nla = cb->data;
On-Stack input.
I can't see how its wrong, ->start() happens from same context as netlink_dump_start so its valid.
Quoted 10 lines
net/netfilter/nf_tables_api.c nft_obj_filter_alloc(const struct nlattr * const nla[]) · line 5028
Quoted 10 lines
net/netfilter/nf_tables_api.c +10 -0
+   struct nft_obj_filter *filter = NULL;
+
+   if (nla[NFTA_OBJ_TABLE] ||
+       nla[NFTA_OBJ_TYPE]) {
+           filter = nft_obj_filter_alloc(nla);
+           if (IS_ERR(filter))
+                   return -ENOMEM;
+   }
+
+   cb->data = filter;
And this replaced the on-stack input with dynamically allocated one, which will be free'd via ->done().
Quoted 3 lines
net/netfilter/nf_tables_api.c nft_obj_filter_alloc(const struct nlattr * const nla[])
Quoted 3 lines
net/netfilter/nf_tables_api.c +0 -0
 /* called with rcu_read_lock held */
 static int nf_tables_getobj(struct net *net, struct sock *nlsk,
                        struct sk_buff *skb, const struct nlmsghdr
*nlh,
Quoted 1 line
net/netfilter/nf_tables_api.c static int nf_tables_getobj(struct net *net,
Quoted 1 line
net/netfilter/nf_tables_api.c +0 -0
@@ -5028,21 +5047,13 @@ static int nf_tables_getobj(struct net *net,
struct sock *nlsk,
Quoted 9 lines
net/netfilter/nf_tables_api.c static int nf_tables_getobj(struct net *net, struct sock *nlsk, · line 5054
Quoted 8 lines
net/netfilter/nf_tables_api.c +2 -0
    if (nlh->nlmsg_flags & NLM_F_DUMP) {
            struct netlink_dump_control c = {
+                   .start = nf_tables_dump_obj_start,
                    .dump = nf_tables_dump_obj,
                    .done = nf_tables_dump_obj_done,
                    .module = THIS_MODULE,
+                   .data = (void *)nla,
Quoted 3 lines
You cannot do this.
nla is allocated in this stack.
Yes.
Quoted 1 line
the nla will not be available in the second recv(), it won't be there.
Its replaced in ->start().
As David pointed out, once ->start() returns 0 we set cb_running, i.e. only after successful ->start() netlink core will call ->dump() again.
So I see no problem setting ->data to onstack cookie and then duplicating it to heap via kmemdup in ->start().
As far as I can see netlink core offers all functionality already, so we only need to switch netfilter to make use of it.
If you disagree please let me know, otherwise I will cook up a patch along this pattern for net/netfilter/*.
Thanks.

shaochun chen

author

2018-07-23 10:34 UTC #12 lore raw

I have a question: we will try_module_get in __netlink_dump_start(), but why we need to call try_module_get again in nft_netlink_dump_start ??

2018-07-23 17:52 GMT+08:00 shaochun chen <[email protected]>:

Quoted 79 lines

allocate memory in ->start(), it's not convenient for users. if call ->done() isn't ok for clean memory when netlink_dump_start() fail, maybe we should have another function ->clean() to clean memory.
2018-07-23 17:28 GMT+08:00 Florian Westphal <[email protected]>:
Quoted 71 lines
Quoted 1 line · Pablo Neira Ayuso
Review
Quoted 1 line
(quoted diff) +0 -0
b/net/netfilter/nf_tables_api.c
Quoted 3 lines
net/netfilter/nf_tables_api.c nft_obj_filter_alloc(const struct nlattr *
Quoted 3 lines
net/netfilter/nf_tables_api.c +0 -0
@@ -5010,6 +5013,22 @@ nft_obj_filter_alloc(const struct nlattr *
const nla[])
Quoted 6 lines
net/netfilter/nf_tables_api.c nft_obj_filter_alloc(const struct nlattr * const nla[]) · line 5018
Quoted 6 lines
net/netfilter/nf_tables_api.c +3 -0
    return filter;
 }
 
+static int nf_tables_dump_obj_start(struct netlink_callback *cb)
+{
+   const struct nlattr * const *nla = cb->data;
On-Stack input.
I can't see how its wrong, ->start() happens from same context as netlink_dump_start so its valid.
Quoted 10 lines
net/netfilter/nf_tables_api.c nft_obj_filter_alloc(const struct nlattr * const nla[]) · line 5028
Quoted 10 lines
net/netfilter/nf_tables_api.c +10 -0
+   struct nft_obj_filter *filter = NULL;
+
+   if (nla[NFTA_OBJ_TABLE] ||
+       nla[NFTA_OBJ_TYPE]) {
+           filter = nft_obj_filter_alloc(nla);
+           if (IS_ERR(filter))
+                   return -ENOMEM;
+   }
+
+   cb->data = filter;
And this replaced the on-stack input with dynamically allocated one, which will be free'd via ->done().
Quoted 3 lines
net/netfilter/nf_tables_api.c nft_obj_filter_alloc(const struct nlattr * const nla[])
Quoted 3 lines
net/netfilter/nf_tables_api.c +0 -0
 /* called with rcu_read_lock held */
 static int nf_tables_getobj(struct net *net, struct sock *nlsk,
                        struct sk_buff *skb, const struct nlmsghdr
*nlh,
Quoted 1 line
net/netfilter/nf_tables_api.c static int nf_tables_getobj(struct net *net,
Quoted 1 line
net/netfilter/nf_tables_api.c +0 -0
@@ -5028,21 +5047,13 @@ static int nf_tables_getobj(struct net *net,
struct sock *nlsk,
Quoted 9 lines
net/netfilter/nf_tables_api.c static int nf_tables_getobj(struct net *net, struct sock *nlsk, · line 5054
Quoted 8 lines
net/netfilter/nf_tables_api.c +2 -0
    if (nlh->nlmsg_flags & NLM_F_DUMP) {
            struct netlink_dump_control c = {
+                   .start = nf_tables_dump_obj_start,
                    .dump = nf_tables_dump_obj,
                    .done = nf_tables_dump_obj_done,
                    .module = THIS_MODULE,
+                   .data = (void *)nla,
Quoted 3 lines
You cannot do this.
nla is allocated in this stack.
Yes.
Quoted 1 line
the nla will not be available in the second recv(), it won't be there.
Its replaced in ->start().
As David pointed out, once ->start() returns 0 we set cb_running, i.e. only after successful ->start() netlink core will call ->dump() again.
So I see no problem setting ->data to onstack cookie and then duplicating it to heap via kmemdup in ->start().
As far as I can see netlink core offers all functionality already, so we only need to switch netfilter to make use of it.
If you disagree please let me know, otherwise I will cook up a patch along this pattern for net/netfilter/*.
Thanks.

Pablo Neira Ayuso

maintainer

2018-07-23 10:43 UTC #13 lore raw

Quoted 2 lines · shaochun chen

I have a question: we will try_module_get in __netlink_dump_start(), but why we need to call try_module_get again in nft_netlink_dump_start ??

Because they refer to two different modules. nfnetlink is multiplexing all netfilter subsystem through one single netlink bus.

At the time that decision was made, there were concerns about netlink running out of busses.

Florian Westphal

maintainer

2018-07-23 10:59 UTC #14 lore raw

In reply to shaochun chen

Quoted 1 line · shaochun chen

I have a question: we will try_module_get in __netlink_dump_start(),

Thats too late, we release rcu read lock before this, so the module implementing ->dump might have been removed already.

Quoted 1 line

but why we need to call try_module_get again in nft_netlink_dump_start ??

Its the other way around.
This is the first try_module_get; at this point we still hold rcu read lock.

If nf_tables module is being removed, try_module_get will fail and we can error out.

If it succeeds, its safe to drop the rcu read lock.