diff mbox series

netfilter: fix percpu counter block leak on error path when creating new netns

Message ID 20230213042505.334898-1-ptikhomirov@virtuozzo.com
State New
Headers
Series netfilter: fix percpu counter block leak on error path when creating new netns |

Commit Message

Pavel Tikhomirov Feb. 13, 2023, 4:25 a.m. UTC 
  Here is the stack where we allocate percpu counter block:

  +-< __alloc_percpu
    +-< xt_percpu_counter_alloc
      +-< find_check_entry # {arp,ip,ip6}_tables.c
        +-< translate_table

And it can be leaked on this code path:

  +-> ip6t_register_table
    +-> translate_table # allocates percpu counter block
    +-> xt_register_table # fails

there is no freeing of the counter block on xt_register_table fail.
Note: xt_percpu_counter_free should be called to free it like we do in
do_replace through cleanup_entry helper (or in __ip6t_unregister_table).

Probability of hitting this error path is low AFAICS (xt_register_table
can only return ENOMEM here, as it is not replacing anything, as we are
creating new netns, and it is hard to imagine that all previous
allocations succeeded and after that one in xt_register_table failed).
But it's worth fixing even the rare leak.

Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
---
 net/ipv4/netfilter/arp_tables.c | 4 ++++
 net/ipv4/netfilter/ip_tables.c  | 4 ++++
 net/ipv6/netfilter/ip6_tables.c | 4 ++++
 3 files changed, 12 insertions(+)

Comments

Pablo Neira Ayuso Feb. 21, 2023, 11:25 p.m. UTC | #1 
Hi,

On Mon, Feb 13, 2023 at 12:25:05PM +0800, Pavel Tikhomirov wrote:
> Here is the stack where we allocate percpu counter block:
> 
>   +-< __alloc_percpu
>     +-< xt_percpu_counter_alloc
>       +-< find_check_entry # {arp,ip,ip6}_tables.c
>         +-< translate_table
> 
> And it can be leaked on this code path:
> 
>   +-> ip6t_register_table
>     +-> translate_table # allocates percpu counter block
>     +-> xt_register_table # fails
> 
> there is no freeing of the counter block on xt_register_table fail.
> Note: xt_percpu_counter_free should be called to free it like we do in
> do_replace through cleanup_entry helper (or in __ip6t_unregister_table).
> 
> Probability of hitting this error path is low AFAICS (xt_register_table
> can only return ENOMEM here, as it is not replacing anything, as we are
> creating new netns, and it is hard to imagine that all previous
> allocations succeeded and after that one in xt_register_table failed).
> But it's worth fixing even the rare leak.

Any suggestion as Fixes: tag here? This issue seems to be rather old?
Pavel Tikhomirov Feb. 22, 2023, 2:11 a.m. UTC | #2 
On 22.02.2023 07:25, Pablo Neira Ayuso wrote:
> Hi,
> 
> On Mon, Feb 13, 2023 at 12:25:05PM +0800, Pavel Tikhomirov wrote:
>> Here is the stack where we allocate percpu counter block:
>>
>>    +-< __alloc_percpu
>>      +-< xt_percpu_counter_alloc
>>        +-< find_check_entry # {arp,ip,ip6}_tables.c
>>          +-< translate_table
>>
>> And it can be leaked on this code path:
>>
>>    +-> ip6t_register_table
>>      +-> translate_table # allocates percpu counter block
>>      +-> xt_register_table # fails
>>
>> there is no freeing of the counter block on xt_register_table fail.
>> Note: xt_percpu_counter_free should be called to free it like we do in
>> do_replace through cleanup_entry helper (or in __ip6t_unregister_table).
>>
>> Probability of hitting this error path is low AFAICS (xt_register_table
>> can only return ENOMEM here, as it is not replacing anything, as we are
>> creating new netns, and it is hard to imagine that all previous
>> allocations succeeded and after that one in xt_register_table failed).
>> But it's worth fixing even the rare leak.
> 
> Any suggestion as Fixes: tag here? This issue seems to be rather old?


If I'm correct:

1) we have this exact percpu leak since commit 71ae0dff02d7 ("netfilter: 
xtables: use percpu rule counters") which introduced the percpu allocation.

2) but we don't call cleanup_entry on this path at least since commit 
1da177e4c3f4 ("Linux-2.6.12-rc2") which is really old.

3) I also see the same thing here 
https://github.com/mpe/linux-fullhistory/blame/1ab7e5ccf454483fb78998854dddd0bab398c3de/net/ipv4/netfilter/arp_tables.c#L1169 
which is probably the initiall commit which introduced 
net/ipv4/netfilter/arp_tables.c file.

So I'm not sure about Fixes: tag, probably one of those three commits.
Pablo Neira Ayuso Feb. 22, 2023, 9:02 a.m. UTC | #3 
On Wed, Feb 22, 2023 at 10:11:03AM +0800, Pavel Tikhomirov wrote:
> On 22.02.2023 07:25, Pablo Neira Ayuso wrote:
> > Hi,
> > 
> > On Mon, Feb 13, 2023 at 12:25:05PM +0800, Pavel Tikhomirov wrote:
> > > Here is the stack where we allocate percpu counter block:
> > > 
> > >    +-< __alloc_percpu
> > >      +-< xt_percpu_counter_alloc
> > >        +-< find_check_entry # {arp,ip,ip6}_tables.c
> > >          +-< translate_table
> > > 
> > > And it can be leaked on this code path:
> > > 
> > >    +-> ip6t_register_table
> > >      +-> translate_table # allocates percpu counter block
> > >      +-> xt_register_table # fails
> > > 
> > > there is no freeing of the counter block on xt_register_table fail.
> > > Note: xt_percpu_counter_free should be called to free it like we do in
> > > do_replace through cleanup_entry helper (or in __ip6t_unregister_table).
> > > 
> > > Probability of hitting this error path is low AFAICS (xt_register_table
> > > can only return ENOMEM here, as it is not replacing anything, as we are
> > > creating new netns, and it is hard to imagine that all previous
> > > allocations succeeded and after that one in xt_register_table failed).
> > > But it's worth fixing even the rare leak.
> > 
> > Any suggestion as Fixes: tag here? This issue seems to be rather old?
> 
> 
> If I'm correct:
> 
> 1) we have this exact percpu leak since commit 71ae0dff02d7
> ("netfilter: xtables: use percpu rule counters") which introduced
> the percpu allocation.
> 
> 2) but we don't call cleanup_entry on this path at least since
> commit 1da177e4c3f4 ("Linux-2.6.12-rc2") which is really old.
> 
> 3) I also see the same thing here https://github.com/mpe/linux-fullhistory/blame/1ab7e5ccf454483fb78998854dddd0bab398c3de/net/ipv4/netfilter/arp_tables.c#L1169
> which is probably the initiall commit which introduced
> net/ipv4/netfilter/arp_tables.c file.
> 
> So I'm not sure about Fixes: tag, probably one of those three commits.

Thanks, I will pick #1 as Fixes: tag.
diff mbox series

Patch

diff --git a/net/ipv4/netfilter/arp_tables.c b/net/ipv4/netfilter/arp_tables.c
index ffc0cab7cf18..2407066b0fec 100644
--- a/net/ipv4/netfilter/arp_tables.c
+++ b/net/ipv4/netfilter/arp_tables.c
@@ -1525,6 +1525,10 @@  int arpt_register_table(struct net *net,
 
 	new_table = xt_register_table(net, table, &bootstrap, newinfo);
 	if (IS_ERR(new_table)) {
+		struct arpt_entry *iter;
+
+		xt_entry_foreach(iter, loc_cpu_entry, newinfo->size)
+			cleanup_entry(iter, net);
 		xt_free_table_info(newinfo);
 		return PTR_ERR(new_table);
 	}
diff --git a/net/ipv4/netfilter/ip_tables.c b/net/ipv4/netfilter/ip_tables.c
index 2ed7c58b471a..2eb0852ad2b1 100644
--- a/net/ipv4/netfilter/ip_tables.c
+++ b/net/ipv4/netfilter/ip_tables.c
@@ -1742,6 +1742,10 @@  int ipt_register_table(struct net *net, const struct xt_table *table,
 
 	new_table = xt_register_table(net, table, &bootstrap, newinfo);
 	if (IS_ERR(new_table)) {
+		struct ipt_entry *iter;
+
+		xt_entry_foreach(iter, loc_cpu_entry, newinfo->size)
+			cleanup_entry(iter, net);
 		xt_free_table_info(newinfo);
 		return PTR_ERR(new_table);
 	}
diff --git a/net/ipv6/netfilter/ip6_tables.c b/net/ipv6/netfilter/ip6_tables.c
index 2d816277f2c5..34eeb53bc9fb 100644
--- a/net/ipv6/netfilter/ip6_tables.c
+++ b/net/ipv6/netfilter/ip6_tables.c
@@ -1751,6 +1751,10 @@  int ip6t_register_table(struct net *net, const struct xt_table *table,
 
 	new_table = xt_register_table(net, table, &bootstrap, newinfo);
 	if (IS_ERR(new_table)) {
+		struct ip6t_entry *iter;
+
+		xt_entry_foreach(iter, loc_cpu_entry, newinfo->size)
+			cleanup_entry(iter, net);
 		xt_free_table_info(newinfo);
 		return PTR_ERR(new_table);
 	}