diff mbox series

xen/netback: avoid race in xenvif_rx_ring_slots_available()

Message ID 20210202070938.7863-1-jgross@suse.com
State Accepted
Commit ec7d8e7dd3a59528e305a18e93f1cb98f7faf83b
Headers show
Series xen/netback: avoid race in xenvif_rx_ring_slots_available() | expand

Commit Message

Juergen Gross Feb. 2, 2021, 7:09 a.m. UTC
Since commit 23025393dbeb3b8b3 ("xen/netback: use lateeoi irq binding")
xenvif_rx_ring_slots_available() is no longer called only from the rx
queue kernel thread, so it needs to access the rx queue with the
associated queue held.

Reported-by: Igor Druzhinin <igor.druzhinin@citrix.com>
Fixes: 23025393dbeb3b8b3 ("xen/netback: use lateeoi irq binding")
Cc: stable@vger.kernel.org
Signed-off-by: Juergen Gross <jgross@suse.com>
---
 drivers/net/xen-netback/rx.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

Comments

Igor Druzhinin Feb. 2, 2021, 3:26 p.m. UTC | #1
On 02/02/2021 07:09, Juergen Gross wrote:
> Since commit 23025393dbeb3b8b3 ("xen/netback: use lateeoi irq binding")
> xenvif_rx_ring_slots_available() is no longer called only from the rx
> queue kernel thread, so it needs to access the rx queue with the
> associated queue held.
> 
> Reported-by: Igor Druzhinin <igor.druzhinin@citrix.com>
> Fixes: 23025393dbeb3b8b3 ("xen/netback: use lateeoi irq binding")
> Cc: stable@vger.kernel.org
> Signed-off-by: Juergen Gross <jgross@suse.com>

Appreciate a quick fix! Is this the only place that sort of race could
happen now?

Igor
Juergen Gross Feb. 2, 2021, 4:12 p.m. UTC | #2
On 02.02.21 16:26, Igor Druzhinin wrote:
> On 02/02/2021 07:09, Juergen Gross wrote:
>> Since commit 23025393dbeb3b8b3 ("xen/netback: use lateeoi irq binding")
>> xenvif_rx_ring_slots_available() is no longer called only from the rx
>> queue kernel thread, so it needs to access the rx queue with the
>> associated queue held.
>>
>> Reported-by: Igor Druzhinin <igor.druzhinin@citrix.com>
>> Fixes: 23025393dbeb3b8b3 ("xen/netback: use lateeoi irq binding")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Juergen Gross <jgross@suse.com>
> 
> Appreciate a quick fix! Is this the only place that sort of race could
> happen now?

I checked and didn't find any other similar problem.


Juergen
Wei Liu Feb. 2, 2021, 4:22 p.m. UTC | #3
On Tue, Feb 02, 2021 at 08:09:38AM +0100, Juergen Gross wrote:
> Since commit 23025393dbeb3b8b3 ("xen/netback: use lateeoi irq binding")
> xenvif_rx_ring_slots_available() is no longer called only from the rx
> queue kernel thread, so it needs to access the rx queue with the
> associated queue held.
> 
> Reported-by: Igor Druzhinin <igor.druzhinin@citrix.com>
> Fixes: 23025393dbeb3b8b3 ("xen/netback: use lateeoi irq binding")
> Cc: stable@vger.kernel.org
> Signed-off-by: Juergen Gross <jgross@suse.com>

Acked-by: Wei Liu <wl@xen.org>
Jakub Kicinski Feb. 3, 2021, 11:48 p.m. UTC | #4
On Tue,  2 Feb 2021 08:09:38 +0100 Juergen Gross wrote:
> Since commit 23025393dbeb3b8b3 ("xen/netback: use lateeoi irq binding")

> xenvif_rx_ring_slots_available() is no longer called only from the rx

> queue kernel thread, so it needs to access the rx queue with the

> associated queue held.

> 

> Reported-by: Igor Druzhinin <igor.druzhinin@citrix.com>

> Fixes: 23025393dbeb3b8b3 ("xen/netback: use lateeoi irq binding")

> Cc: stable@vger.kernel.org

> Signed-off-by: Juergen Gross <jgross@suse.com>


Should we route this change via networking trees? I see the bug did not
go through networking :)
Juergen Gross Feb. 4, 2021, 5:32 a.m. UTC | #5
On 04.02.21 00:48, Jakub Kicinski wrote:
> On Tue,  2 Feb 2021 08:09:38 +0100 Juergen Gross wrote:

>> Since commit 23025393dbeb3b8b3 ("xen/netback: use lateeoi irq binding")

>> xenvif_rx_ring_slots_available() is no longer called only from the rx

>> queue kernel thread, so it needs to access the rx queue with the

>> associated queue held.

>>

>> Reported-by: Igor Druzhinin <igor.druzhinin@citrix.com>

>> Fixes: 23025393dbeb3b8b3 ("xen/netback: use lateeoi irq binding")

>> Cc: stable@vger.kernel.org

>> Signed-off-by: Juergen Gross <jgross@suse.com>

> 

> Should we route this change via networking trees? I see the bug did not

> go through networking :)

> 


I'm fine with either networking or the Xen tree. It should be included
in 5.11, though. So if you are willing to take it, please do so.


Juergen
Jakub Kicinski Feb. 5, 2021, 1:56 a.m. UTC | #6
On Thu, 4 Feb 2021 06:32:32 +0100 Jürgen Groß wrote:
> On 04.02.21 00:48, Jakub Kicinski wrote:

> > On Tue,  2 Feb 2021 08:09:38 +0100 Juergen Gross wrote:  

> >> Since commit 23025393dbeb3b8b3 ("xen/netback: use lateeoi irq binding")

> >> xenvif_rx_ring_slots_available() is no longer called only from the rx

> >> queue kernel thread, so it needs to access the rx queue with the

> >> associated queue held.

> >>

> >> Reported-by: Igor Druzhinin <igor.druzhinin@citrix.com>

> >> Fixes: 23025393dbeb3b8b3 ("xen/netback: use lateeoi irq binding")

> >> Cc: stable@vger.kernel.org

> >> Signed-off-by: Juergen Gross <jgross@suse.com>  

> > 

> > Should we route this change via networking trees? I see the bug did not

> > go through networking :)

> 

> I'm fine with either networking or the Xen tree. It should be included

> in 5.11, though. So if you are willing to take it, please do so.


All right, applied to net, it'll most likely hit Linus's tree on Tue.

Thanks!
diff mbox series

Patch

diff --git a/drivers/net/xen-netback/rx.c b/drivers/net/xen-netback/rx.c
index b8febe1d1bfd..accc991d153f 100644
--- a/drivers/net/xen-netback/rx.c
+++ b/drivers/net/xen-netback/rx.c
@@ -38,10 +38,15 @@  static bool xenvif_rx_ring_slots_available(struct xenvif_queue *queue)
 	RING_IDX prod, cons;
 	struct sk_buff *skb;
 	int needed;
+	unsigned long flags;
+
+	spin_lock_irqsave(&queue->rx_queue.lock, flags);
 
 	skb = skb_peek(&queue->rx_queue);
-	if (!skb)
+	if (!skb) {
+		spin_unlock_irqrestore(&queue->rx_queue.lock, flags);
 		return false;
+	}
 
 	needed = DIV_ROUND_UP(skb->len, XEN_PAGE_SIZE);
 	if (skb_is_gso(skb))
@@ -49,6 +54,8 @@  static bool xenvif_rx_ring_slots_available(struct xenvif_queue *queue)
 	if (skb->sw_hash)
 		needed++;
 
+	spin_unlock_irqrestore(&queue->rx_queue.lock, flags);
+
 	do {
 		prod = queue->rx.sring->req_prod;
 		cons = queue->rx.req_cons;