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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1410207244.11872.127.camel@edumazet-glaptop2.roam.corp.google.com>
Date:	Mon, 08 Sep 2014 13:14:04 -0700
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	Alexander Duyck <alexander.h.duyck@...el.com>
Cc:	netdev@...r.kernel.org, richardcochran@...il.com,
	davem@...emloft.net
Subject: Re: [PATCH net-next] skb: Add documentation for skb_clone_sk

On Mon, 2014-09-08 at 11:44 -0700, Alexander Duyck wrote:
> On 09/08/2014 10:11 AM, Eric Dumazet wrote:
> > 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@...el.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.
> 
> Isn't that what is guaranteed by using the atomic_inc_not_zero?  If it
> is zero we abort and just return NULL.
> 

Point is : the skb we clone here must have a reference on the socket.

How sk_refcnt could be 0 here ?

If it was 0, then something was broken before skb_clone_sk() call.



> > 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.
> 
> My concern then would be what I should do about skb_tx_complete as I am
> currently using sock_hold/sock_put to prevent the socket from being
> freed due to the skb_orphan call in sock_queue_err_skb.
> 
> Would I need to change the logic there as well in order to prevent us
> from using the wrong reference to keep the socket valid?

We certainly have to think again and clean this.


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