Re: [PATCH 3.17 122/122] netfilter: conntrack: fix race in __nf_conntrack_confirm against get_next_corpse

From: Pablo Neira Ayuso
Date: Sun Dec 07 2014 - 13:13:23 EST


Please, withdraw this patch. It has been reverted.

Thanks Greg.

On Fri, Dec 05, 2014 at 02:44:56PM -0800, Greg Kroah-Hartman wrote:
> 3.17-stable review patch. If anyone has any objections, please let me know.
>
> ------------------
>
> From: bill bonaparte <programme110@xxxxxxxxx>
>
> commit 5195c14c8b27cc0b18220ddbf0e5ad3328a04187 upstream.
>
> After removal of the central spinlock nf_conntrack_lock, in
> commit 93bb0ceb75be2 ("netfilter: conntrack: remove central
> spinlock nf_conntrack_lock"), it is possible to race against
> get_next_corpse().
>
> The race is against the get_next_corpse() cleanup on
> the "unconfirmed" list (a per-cpu list with seperate locking),
> which set the DYING bit.
>
> Fix this race, in __nf_conntrack_confirm(), by removing the CT
> from unconfirmed list before checking the DYING bit. In case
> race occured, re-add the CT to the dying list.
>
> While at this, fix coding style of the comment that has been
> updated.
>
> Fixes: 93bb0ceb75be2 ("netfilter: conntrack: remove central spinlock nf_conntrack_lock")
> Reported-by: bill bonaparte <programme110@xxxxxxxxx>
> Signed-off-by: bill bonaparte <programme110@xxxxxxxxx>
> Signed-off-by: Jesper Dangaard Brouer <brouer@xxxxxxxxxx>
> Signed-off-by: Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx>
> Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
>
> ---
> net/netfilter/nf_conntrack_core.c | 14 ++++++++------
> 1 file changed, 8 insertions(+), 6 deletions(-)
>
> --- a/net/netfilter/nf_conntrack_core.c
> +++ b/net/netfilter/nf_conntrack_core.c
> @@ -611,12 +611,16 @@ __nf_conntrack_confirm(struct sk_buff *s
> */
> NF_CT_ASSERT(!nf_ct_is_confirmed(ct));
> pr_debug("Confirming conntrack %p\n", ct);
> - /* We have to check the DYING flag inside the lock to prevent
> - a race against nf_ct_get_next_corpse() possibly called from
> - user context, else we insert an already 'dead' hash, blocking
> - further use of that particular connection -JM */
> +
> + /* We have to check the DYING flag after unlink to prevent
> + * a race against nf_ct_get_next_corpse() possibly called from
> + * user context, else we insert an already 'dead' hash, blocking
> + * further use of that particular connection -JM.
> + */
> + nf_ct_del_from_dying_or_unconfirmed_list(ct);
>
> if (unlikely(nf_ct_is_dying(ct))) {
> + nf_ct_add_to_dying_list(ct);
> nf_conntrack_double_unlock(hash, reply_hash);
> local_bh_enable();
> return NF_ACCEPT;
> @@ -636,8 +640,6 @@ __nf_conntrack_confirm(struct sk_buff *s
> zone == nf_ct_zone(nf_ct_tuplehash_to_ctrack(h)))
> goto out;
>
> - nf_ct_del_from_dying_or_unconfirmed_list(ct);
> -
> /* Timer relative to confirmation time, not original
> setting time, otherwise we'd get timer wrap in
> weird delay cases. */
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/