On Thu, Jan 23, 2020 at 6:29 PM Florian Westphal <fw@xxxxxxxxx> wrote:
>
> Sean Tranchetti <stranche@xxxxxxxxxxxxxx> 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
I don't see how this happens either.
Hi Sean,
do you have a script to reproduce this issue?
Thanks.
> >
> > Signed-off-by: Sean Tranchetti <stranche@xxxxxxxxxxxxxx>
> > ---
> > @@ -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.
