Re: [PATCH nf 1/3] netfilter: conntrack: fix race between nf_conntrack proc read and hash resize

[Florian Westphal <fw@strlen.de>]

Liping Zhang <zlpnobody@163.com> wrote:
> From: Liping Zhang <liping.zhang@spreadtrum.com>
> 
> When we do "cat /proc/net/nf_conntrack", and meanwhile resize the conntrack
> hash table via /sys/module/nf_conntrack/parameters/hashsize, race will
> happen, because reader can observe a newly allocated hash but the old size
> (or vice versa). So oops will happen like follows：
> 
>   BUG: unable to handle kernel NULL pointer dereference at 0000000000000017
>   IP: [<ffffffffa0418e21>] seq_print_acct+0x11/0x50 [nf_conntrack]
>   Call Trace:
>   [<ffffffffa0412f4e>] ? ct_seq_show+0x14e/0x340 [nf_conntrack]
>   [<ffffffff81261a1c>] seq_read+0x2cc/0x390
>   [<ffffffff812a8d62>] proc_reg_read+0x42/0x70
>   [<ffffffff8123bee7>] __vfs_read+0x37/0x130
>   [<ffffffff81347980>] ? security_file_permission+0xa0/0xc0
>   [<ffffffff8123cf75>] vfs_read+0x95/0x140
>   [<ffffffff8123e475>] SyS_read+0x55/0xc0
>   [<ffffffff817c2572>] entry_SYSCALL_64_fastpath+0x1a/0xa4
> 
> It is very easy to reproduce this kernel crash.
> 1. open one shell and input the following cmds:
>   while : ; do
>     echo $RANDOM > hashsize
>   done
> 2. open more shells and input the following cmds:
>   while : ; do
>     cat /proc/net/nf_conntrack
>   done
> 3. just wait a monent, oops will happen soon.

Good catch, but ...

> diff --git a/include/net/netfilter/nf_conntrack_core.h b/include/net/netfilter/nf_conntrack_core.h
> index 3e2f332..4f6453a 100644
> --- a/include/net/netfilter/nf_conntrack_core.h
> +++ b/include/net/netfilter/nf_conntrack_core.h
> @@ -82,6 +82,7 @@ print_tuple(struct seq_file *s, const struct nf_conntrack_tuple *tuple,
>  #define CONNTRACK_LOCKS 1024
>  
>  extern struct hlist_nulls_head *nf_conntrack_hash;
> +extern seqcount_t nf_conntrack_generation;

instead of this and the proliferation of this:

> +	do {
> +		sequence = read_seqcount_begin(&nf_conntrack_generation);
> +		st->htable_size = nf_conntrack_htable_size;
> +		st->hash = nf_conntrack_hash;
> +	} while (read_seqcount_retry(&nf_conntrack_generation, sequence));
> +
>  	return ct_get_idx(seq, *pos);
>  }

I think it might be better to do something like

/* must be called with rcu read lock held */
unsigned int nf_conntrack_get_ht(struct hlist_nulls_head *h,
			         unsigned int *buckets)
{
	do {
		s = read_seq ...
		size = nf_conntrack_htable_size;
		ptr = nf_conntrack_hash;
	} while ...

	*h = ptr;
	*buckets = size;

	return s;
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html