netdev - Re: [PATCH] xen/netback: Wake dealloc thread after completing zerocopy work

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]

Date:	Tue, 4 Aug 2015 14:11:49 +0100
From:	Wei Liu <wei.liu2@...rix.com>
To:	Ross Lagerwall <ross.lagerwall@...rix.com>
CC:	<netdev@...r.kernel.org>, <xen-devel@...ts.xenproject.org>,
	Wei Liu <wei.liu2@...rix.com>,
	Ian Campbell <ian.campbell@...rix.com>
Subject: Re: [PATCH] xen/netback: Wake dealloc thread after completing
 zerocopy work

On Tue, Aug 04, 2015 at 01:50:58PM +0100, Ross Lagerwall wrote:
> Waking the dealloc thread before decrementing inflight_packets is racy
> because it means the thread may go to sleep before inflight_packets is
> decremented. If kthread_stop() has already been called, the dealloc
> thread may wait forever with nothing to wake it. Instead, wake the
> thread only after decrementing inflight_packets.
> 
> Signed-off-by: Ross Lagerwall <ross.lagerwall@...rix.com>
> ---
>  drivers/net/xen-netback/netback.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
> index 7d50711..e95ee20 100644
> --- a/drivers/net/xen-netback/netback.c
> +++ b/drivers/net/xen-netback/netback.c
> @@ -1536,7 +1536,6 @@ void xenvif_zerocopy_callback(struct ubuf_info *ubuf, bool zerocopy_success)
>  		smp_wmb();
>  		queue->dealloc_prod++;
>  	} while (ubuf);
> -	wake_up(&queue->dealloc_wq);
>  	spin_unlock_irqrestore(&queue->callback_lock, flags);
>  
>  	if (likely(zerocopy_success))
> @@ -1544,6 +1543,7 @@ void xenvif_zerocopy_callback(struct ubuf_info *ubuf, bool zerocopy_success)
>  	else
>  		queue->stats.tx_zerocopy_fail++;
>  	xenvif_skb_zerocopy_complete(queue);
> +	wake_up(&queue->dealloc_wq);

Can you move this wake_up into xenvif_skb_zerocopy_complete and have a
comment there saying wake_up must be called after decrementing inflight
counters (possibly with some of the commit message)? That way we don't
trip over this in the future if we're to refactor the callback function.

Wei.

>  }
>  
>  static inline void xenvif_tx_dealloc_action(struct xenvif_queue *queue)
> -- 
> 2.1.0
--
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