Re: [PATCH net-next] skb: Add documentation for skb_clone_sk

[Eric Dumazet <eric.dumazet@gmail.com>]

On Mon, 2014-09-08 at 12:18 -0400, Alexander Duyck wrote:
> This change adds some documentation to the call skb_clone_sk.  This is
> meant to help clarify the purpose of the function for other developers.
> 
> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
> ---
>  net/core/skbuff.c |   11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index a18dfb0..3f83a8a 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -3511,6 +3511,17 @@ struct sk_buff *sock_dequeue_err_skb(struct sock *sk)
>  }
>  EXPORT_SYMBOL(sock_dequeue_err_skb);
>  
> +/**
> + * skb_clone_sk - create clone of skb, and take reference to socket
> + * @skb: the skb to clone
> + *
> + * For functions such as timestamping it is necessary to clone an skb and
> + * hold a reference to the socket for it until the hardware delivers the
> + * actual timestamp or the timestamp is timed out.  As as such this
> + * function is useful for creating a clone to later be handed off to
> + * skb_complete_tx_timestamp or kfree_skb to take care of cleaning up
> + * the reference handling for the socket.
> + */
>  struct sk_buff *skb_clone_sk(struct sk_buff *skb)
>  {
>  	struct sock *sk = skb->sk;

Note that I have serious doubts about the atomic_inc_not_zero() here.

At this point, we need to have consistent refcounting on the socket.

If we decide the reference is against sk_refcnt, then current sk_refcnt
cannot be 0 at this point.

This might hide a very serious bug.

In TCP tx path for example, we do not take reference on sk_refcnt for
each packet, but a reference on sk_wmem_alloc

If skb destructor is sock_wfree() or tcp_wfree(), then we should take an
extra reference on sk_wmem_alloc instead of sk_refcnt.