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
| ||
|
Message-ID: <540DF91E.7040005@intel.com> Date: Mon, 08 Sep 2014 11:44:46 -0700 From: Alexander Duyck <alexander.h.duyck@...el.com> To: Eric Dumazet <eric.dumazet@...il.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 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. > 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 -- 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
Powered by blists - more mailing lists