From: Alexander Duyck <alexander.h.duyck@intel•com>
To: Eric Dumazet <eric.dumazet@gmail•com>
Cc: netdev@vger•kernel.org, richardcochran@gmail•com, davem@davemloft•net
Subject: Re: [PATCH net-next] skb: Add documentation for skb_clone_sk
Date: Mon, 08 Sep 2014 11:44:46 -0700 [thread overview]
Message-ID: <540DF91E.7040005@intel.com> (raw)
In-Reply-To: <1410196297.11872.119.camel@edumazet-glaptop2.roam.corp.google.com>
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
next prev parent reply other threads:[~2014-09-08 18:44 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-09-08 16:18 [PATCH net-next] skb: Add documentation for skb_clone_sk Alexander Duyck
2014-09-08 17:11 ` Eric Dumazet
2014-09-08 18:44 ` Alexander Duyck [this message]
2014-09-08 20:14 ` Eric Dumazet
2014-09-10 16:43 ` Alexander Duyck
2014-09-10 0:20 ` David Miller
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=540DF91E.7040005@intel.com \
--to=alexander.h.duyck@intel$(echo .)com \
--cc=davem@davemloft$(echo .)net \
--cc=eric.dumazet@gmail$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
--cc=richardcochran@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox