Re: [PATCH 2/2] net-timestamp: Make the clone operation stand-alone from phy timestamping

[Alexander Duyck <alexander.h.duyck@intel.com>]

On 09/03/2014 03:05 PM, Eric Dumazet wrote:
> On Wed, 2014-09-03 at 14:07 -0700, Eric Dumazet wrote:
> 
>>
>> Normally, if one skb holds a reference to a socket, it should have
>> skb->destructor set to a cleanup function.
>>
>> Otherwise, we rely on callers following a convention, to release sk
>> reference.
>>
>> If you believe its needed, it should be dully documented.
> 
> 
> BTW, skb_orphan() would BUG() on this case (skb->destrutor == NULL and
> skb->sk != NULL)

That was why we were setting skb->sk to null before passing it to
sock_queue_err_skb.  I think I found the reason why things were done the
way they were.  It looks like skb_orphan is called in
sock_queue_err_skb.  So any destructor added would be fired there before
before being replaced.

The only part I am not sure about is if that would actually be any sort
of issue.  Do we really need to hold the extra reference to the socket
all the way through the sock_queue_err_skb call or can we just let it
get dropped with the call to skb_orphan and let the fact that the
message is being queued be good enough?

I'm not sure due to the comment about "before exiting rcu section, make
sure dst is refcounted".  It kind of implies I should be able to hand
off the reference counts without worrying about the socket being freed
out from under me.

Thanks,

Alex