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

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

[Alexander Duyck]

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;

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

[Eric Dumazet]

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.

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

[Alexander Duyck]

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@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.

Isn't that what is guaranteed by using the atomic_inc_not_zero?  If it
is zero we abort and just return NULL.

> 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?

Thanks,

Alex

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

[Eric Dumazet]

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@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.
> 
> 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.

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

[David Miller]

From: Alexander Duyck <alexander.h.duyck@intel.com>
Date: Mon, 08 Sep 2014 12:18:22 -0400

> 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>

Since there is still active discussion about whether the socket
refcounting is even handled correctly by these mechanisms, I
am deferring on this patch for now.

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

[Alexander Duyck]

On 09/08/2014 01:14 PM, Eric Dumazet wrote:
> 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@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.
>>
>> 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.

Are you sure about that?  It seems like what sock_put does is decrement
the sk_refcnt, then the sk_wmem_alloc.  At that point we are just
waiting on the remaining outstanding Tx frames before the socket closes.
 So wouldn't it be possible to have frames sitting in a Qdisc after
sock_put is called such that the only reference still keeping the socket
open is sk_wmem_alloc?

In that case us returning the timestamp would be pointless since an
sk_refcnt of 0 would indicate that there is nobody on the other end to
receive it anyway.  So we don't perform the clone and return a NULL pointer.

>>> This might hide a very serious bug.

Actually how is this code any different from the early demux code?  From
what I can tell that code goes through and calls atomic_inc_not_zero as
a part of __inet_lookup_established and uses a similar destructor that
eventually frees the sk_refcnt value.  The only real difference I see is
that we know the socket we want before-hand and we don't have to search
for it.

In our case we could be best described as a "running timer" as we are
waiting for an acknowledgment for the frame from the timestamping device
or planning to time it out.  The comments just above sock_put state that
such an entity should be holding a reference to sk_refcnt.

>>> 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.

Actually I think this extends to sock_queue_err_skb in general.  In the
case of skb_complete_tx_timestamp I think we avoid any issues due to the
fact that we are holding the reference created by skb_clone_sk.

However for skb_complete_wifi_ack it seems like there is a potential for
issues as the original Tx skb is what is returned and the clone is
transmitted.  The skb_orphan call could potentially call __sk_free on
the socket before trying to enqueue the skb on the sk_error_queue.  I'm
not exactly sure what the result would be, but I suspect it would
probably be a memory leak.

Thanks,

Alex