On 11/11/19 12:49 PM, stranche@xxxxxxxxxxxxxx wrote:
> Hi all,
>
> We recently had a crash reported to us on the 4.19 kernel where ip6_do_table() appeared to be referencing a jumpstack that had already been freed.
> Based on the dump, it appears that the scenario was a concurrent use of iptables-restore and active data transfer. The kernel has Florian's commit
> to wait in xt_replace_table instead of get_counters(), 80055dab5de0 ("netfilter: x_tables: make xt_replace_table wait until old rules are not used
> anymore"), so it appears that xt_replace_table is somehow returning prematurely, allowing __do_replace() to free the table while it is still in use.
>
> After reviewing the code, we had a question about the following section:
> /* ... so wait for even xt_recseq on all cpus */
> for_each_possible_cpu(cpu) {
> seqcount_t *s = &per_cpu(xt_recseq, cpu);
> u32 seq = raw_read_seqcount(s);
>
> if (seq & 1) {
> do {
> cond_resched();
> cpu_relax();
> } while (seq == raw_read_seqcount(s));
> }
> }
The intent of this code is to check that each cpu went through a phase where the seq was even at least once.
>
> Based on the other uses of seqcount locks, there should be a paired read_seqcount_retry() to mark the end of the read section like below:
> for_each_possible_cpu(cpu) {
> seqcount_t *s = &per_cpu(xt_recseq, cpu);
> u32 seq;
>
> do {
> seq = raw_read_seqcount(s);
> if (seq & 1) {
> cond_resched();
> cpu_relax();
> }
> } while (read_seqcount_retry(s, seq);
This would loop possibly more times, since you exit if the count is _currently_ even.
If we are unlucky this could loop for a very long time.
> }
>
> These two snippets are very similar, as the original seems like it attempted to open-code this retry() helper, but there is a slight difference in
> the smp_rmb() placement relative to the "retry" read of the sequence value.
> Original:
> READ_ONCE(s->sequence);
> smp_rmb();
> ... //check and resched
> READ_ONCE(s->sequence);
> smp_rmb();
> ... //compare the two sequence values
>
> Modified using read_seqcount_retry():
> READ_ONCE(s->sequence);
> smp_rmb();
> ... //check and and resched
> smp_rmb();
> READ_ONCE(s->sequence);
> ... //compare the two sequence values
>
> Is it possible that this difference in ordering could lead to an incorrect read of the sequence in certain neurotic scenarios? Alternatively, is there
> some other place that this jumpstack could be freed while still in use?
>
4.19 has many bugs really. Please upgrade to v4.19.83
