Re: [PATCH] net: sk_prot_alloc() should not blindly overwrite memory

["Paul E. McKenney" <paulmck@linux.vnet.ibm.com>]

On Thu, Jul 09, 2009 at 07:36:05AM +0200, Eric Dumazet wrote:
> Eric Dumazet a écrit :
> > David Miller a écrit :
> >> From: Eric Dumazet <eric.dumazet@gmail.com>
> >> Date: Wed, 08 Jul 2009 00:33:29 +0200
> >>
> >>> [PATCH] net: sk_prot_alloc() should not blindly overwrite memory
> >>>
> >>> Some sockets use SLAB_DESTROY_BY_RCU, and our RCU code rely that some
> >>> fields should not be blindly overwritten, even with null.
> >>>
> >>> These fields are sk->sk_refcnt and sk->sk_nulls_node.next
> >>>
> >>> Current sk_prot_alloc() implementation doesnt respect this hypothesis,
> >>> calling kmem_cache_alloc() with __GFP_ZERO and setting sk_refcnt to 1
> >>> instead of atomically increment it.
> >>>
> >>> Reported-by: Emil S Tantilov <emils.tantilov@gmail.com>
> >>> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> >> I've applied this but will wait for some more testing before
> >> I push it out for real to kernel.org
> > 
> > Thanks David
> > 
> > I forgot to CC Paul and Patrick, so I'll ask them to look at this patch.
> > 
> > Patrick, a similar fix is needed in conntrack as well, we currently
> > uses "ct = kmem_cache_zalloc(nf_conntrack_cachep, gfp);" and thus
> >  overwrite struct hlist_nulls_node hnnode; contained
> > in "struct nf_conntrack_tuple_hash", while lockless readers still
> > potentialy need them. Setting hnnode.next to NULL is dangerous
> > since last bit is not set (not a nulls value), a reader could
> > try to dereference this NULL pointer and trap.
> > 
> > 
> > Here is the patch again so that Paul & Patrick can comment on it.
> > 
> > I am not sure about the refcnt thing (blindly setting it to 0 again
> > should be OK in fact, since no reader should/can to the 
> > atomic_inc_if_not_zero on it), but the nulls.next thing is problematic.
> 
> Here is an updated and much simpler patch, taking care of sk_node.next being not set to 0
> 
> This patch applies to >= 2.6.29 kernels

Does this one also need the rearrangement of struct elements in the
earlier patch?  (And apologies about being slow to get to that one.)

						Thanx, Paul

> [PATCH] net: sk_prot_alloc() should not blindly overwrite memory
> 
> Some sockets use SLAB_DESTROY_BY_RCU, and our RCU code correctness
> depends on sk->sk_nulls_node.next being always valid. A NULL
> value is not allowed as it might fault a lockless reader.
> 
> Current sk_prot_alloc() implementation doesnt respect this hypothesis,
> calling kmem_cache_alloc() with __GFP_ZERO. Just call memset() around
> the forbidden field.
> 
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> ---
> diff --git a/net/core/sock.c b/net/core/sock.c
> index b0ba569..7b87ec0 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -939,8 +939,23 @@ static struct sock *sk_prot_alloc(struct proto *prot, gfp_t priority,
>  	struct kmem_cache *slab;
> 
>  	slab = prot->slab;
> -	if (slab != NULL)
> -		sk = kmem_cache_alloc(slab, priority);
> +	if (slab != NULL) {
> +		sk = kmem_cache_alloc(slab, priority & ~__GFP_ZERO);
> +		if (!sk)
> +			return sk;
> +		if (priority & __GFP_ZERO) {
> +			/*
> +			 * caches using SLAB_DESTROY_BY_RCU should let
> +			 * sk_node.next un-modified. Special care is taken
> +			 * when initializing object to zero.
> +			 */
> +			if (offsetof(struct sock, sk_node.next) != 0)
> +				memset(sk, 0, offsetof(struct sock, sk_node.next));
> +			memset(&sk->sk_node.pprev, 0,
> +			       prot->obj_size - offsetof(struct sock,
> +							 sk_node.pprev));
> +		}
> +	}
>  	else
>  		sk = kmalloc(prot->obj_size, priority);
>