From: Florian Westphal <fw@strlen.de>
To: Sean Tranchetti <stranche@codeaurora.org>
Cc: pablo@netfilter.org, netfilter-devel@vger.kernel.org,
lucien.xin@gmail.com
Subject: Re: [PATCH nf] Revert "netfilter: unlock xt_table earlier in __do_replace"
Date: Thu, 23 Jan 2020 11:29:21 +0100 [thread overview]
Message-ID: <20200123102921.GU795@breakpoint.cc> (raw)
In-Reply-To: <1579740455-17249-1-git-send-email-stranche@codeaurora.org>
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.
next prev parent reply other threads:[~2020-01-23 10:29 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-01-23 0:47 [PATCH nf] Revert "netfilter: unlock xt_table earlier in __do_replace" Sean Tranchetti
2020-01-23 10:29 ` Florian Westphal [this message]
2020-01-23 20:08 ` stranche
2020-02-03 10:51 ` Xin Long
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200123102921.GU795@breakpoint.cc \
--to=fw@strlen.de \
--cc=lucien.xin@gmail.com \
--cc=netfilter-devel@vger.kernel.org \
--cc=pablo@netfilter.org \
--cc=stranche@codeaurora.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).