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

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] [day] [month] [year] [list]

Message-ID: <540CEBCB.1010600@gmail.com>
Date:	Sun, 07 Sep 2014 16:35:39 -0700
From:	Alexander Duyck <alexander.duyck@...il.com>
To:	Richard Cochran <richardcochran@...il.com>,
	Alexander Duyck <alexander.h.duyck@...el.com>
CC:	netdev@...r.kernel.org, davem@...emloft.net, willemb@...gle.com
Subject: Re: [PATCH net-next v4 2/3] net-timestamp: Make the clone operation
 stand-alone from phy timestamping

On 09/07/2014 02:50 PM, Richard Cochran wrote:
> Just saw this now, was away on vacation, so sorry for the delay...
>
> On Thu, Sep 04, 2014 at 01:31:35PM -0400, Alexander Duyck wrote:
>> v2: Renamed function to skb_clone_sk.
>>     Added destructor to call sock_put instead of doing it ourselves.
>>     Dropped freeing functionality from skb_complete_tx_timestamp.
> ...
>
>> diff --git a/drivers/net/phy/dp83640.c b/drivers/net/phy/dp83640.c
>> index d5991ac..87648b3 100644
>> --- a/drivers/net/phy/dp83640.c
>> +++ b/drivers/net/phy/dp83640.c
>> @@ -1148,7 +1148,7 @@ static void dp83640_remove(struct phy_device *phydev)
>>  		kfree_skb(skb);
>>  
>>  	while ((skb = skb_dequeue(&dp83640->tx_queue)) != NULL)
>> -		skb_complete_tx_timestamp(skb, NULL);
>> +		kfree_skb(skb);
> The way the code was before, there was a clear usage pattern for
> phy_driver.txtstamp() and skb_complete_tx_timestamp() which was also
> documented in the comment to the latter.
>
> Now, we have drivers freeing buffers allocated by the stack.  I
> thought it was cleaner to have the same layer allocate and free the
> clone. Even if you say that this new way is just fine, still you
> should correct the comment to reflect the new pattern.

The "new" pattern is how we have done it for all Tx skbs handed down by
the stack, so why should we treat Tx timestamp SKBs any different?  If
anything this change eliminates a risk since now they don't have to
remember specifically to use a special "destructor included" callback to
free the buffer and the socket.  Instead all of the standard
kfree/consume_skb calls can be used to free the buffer.

It  just occurred to me when I was looking at this code is that it can
now use standard calls such as __skb_queue_purge instead of having to
implement its own version of the call.  I will try to remember to submit
a patch for that tomorrow.

Thanks,

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