Re: [PATCH nf] Revert "netfilter: unlock xt_table earlier in __do_replace"

[Florian Westphal <fw@strlen.de>]

Sean Tranchetti <stranche@codeaurora.org> wrote:

[ CC Xin Long ]

> A recently reported crash in the x_tables framework seems to stem from
> a potential race condition between adding rules to a table and having a
> packet traversing the table at the same time.
> 
> In the crash, the jumpstack being used by the table traversal was freed
> by the table replace code. After performing some bisection, it seems that
> commit f31e5f1a891f ("netfilter: unlock xt_table earlier in __do_replace")
> exposed this race condition by unlocking the table before the
> get_old_counters() routine was called to perform the synchronization.

But the packet path doesn't grab the table mutex.

> Call Stack:
> 	Unable to handle kernel paging request at virtual address
> 	006b6b6b6b6b6bc5
> 
> 	pc : ipt_do_table+0x3b8/0x660
> 	lr : ipt_do_table+0x31c/0x660
> 	Call trace:
> 	ipt_do_table+0x3b8/0x660
> 	iptable_mangle_hook+0x58/0xf8
> 	nf_hook_slow+0x48/0xd8
> 	__ip_local_out+0xf4/0x138
> 	__ip_queue_xmit+0x348/0x3a0
> 	ip_queue_xmit+0x10/0x18
> 
> Signed-off-by: Sean Tranchetti <stranche@codeaurora.org>
> ---
> @@ -921,8 +921,6 @@ static int __do_replace(struct net *net, const char *name,
>  	    (newinfo->number <= oldinfo->initial_entries))
>  		module_put(t->me);
>  
> -	xt_table_unlock(t);
> -
>  	get_old_counters(oldinfo, counters);
>  
>  	/* Decrease module usage counts and free resource */
> @@ -937,6 +935,7 @@ static int __do_replace(struct net *net, const char *name,
>  		net_warn_ratelimited("arptables: counters copy to user failed while replacing table\n");
>  	}
>  	vfree(counters);
> +	xt_table_unlock(t);

I don't see how this changes anything wrt. packet path.
This disallows another instance of iptables(-restore) to come in
before the counters have been copied/freed and the destructors have run.

But as those have nothing to do with the jumpstack I don't see how this
helps.