Re: [netfilter-core] [Q] The usage of xt_recseq.

[Florian Westphal <fw@strlen.de>]

Hi Sebastian!

Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:
> xt_recseq is per-CPU sequence counter which is not entirely using the
> seqcount API.
> The writer side of the sequence counter is updating the packet and byte
> counter (64bit) while processing a packet. The reader simply retrieves
> the two counter.
> Based on the code, the writer side can be recursive which is probably
> why the "regular" write side isn't used or maybe because there is no
> "lock".

Yes, recursive entry is possible even with local_bh_disable(), as
some of the xt_FOO extensions can send a packet (REJECT and TEE come
to mind), which can re-enter into ip_tables' traverser (*_do_table).

> The seqcount is per-CPU and disabling BH is used as the "lock". On
> PREEMPT_RT code in local_bh_disable()ed section is preemptible and this
> means that a seqcount reader with higher priority can preempt the writer
> which leads to a deadlock. 
> 
> While trying to trigger the writer side, I managed only to trigger a
> single reader and only while using iptables-legacy/ arptables-legacy
> commands. The nft did not trigger it. So it is legacy code only.

Yes, this is legacy only.

> Would it work to convert the counters to u64_stats_sync? On 32bit
> there would be a seqcount_t with preemption disabling during the
> update which means the xt_write_recseq_begin()/ xt_write_recseq_end()
> has to be limited the counter update only. On 64bit architectures there
> would be just the update. This means that number of packets and bytes
> might be "off" (the one got updated, the other not "yet") but I don't
> think that this is a problem here.

Unfortunately its not only about counters; local_bh_disable() is also
used to prevent messing up the chain jump stack.

For local hooks, this is called from process context, so in order
to avoid timers kicking in and then re-using the jumpstack, this
local_bh_disable avoids that.

The chain stack is percpu in -legacy, and on-stack in nf_tables.

Then, there is also recursion via xt_TEE.c, hence this strange
        if (static_key_false(&xt_tee_enabled))

in ipt_do_table() (We'll switch to a shadow-stack for that case).