[PATCH] xen-netback: don't de-reference vif pointer after having called xenvif_put()

[PATCH] xen-netback: don't de-reference vif pointer after having called xenvif_put()

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 1643 bytes --]

When putting vif-s on the rx notify list, calling xenvif_put() must be
deferred until after the removal from the list and the issuing of the
notification, as both operations dereference the pointer.

Changing this got me to notice that the "irq" variable was effectively
unused (and was of too narrow type anyway).

Signed-off-by: Jan Beulich <jbeulich@suse.com>

---
 drivers/net/xen-netback/netback.c |   11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

--- 3.10-rc4/drivers/net/xen-netback/netback.c
+++ 3.10-rc4-xen-netback-vif-use-after-free/drivers/net/xen-netback/netback.c
@@ -662,7 +662,7 @@ static void xen_netbk_rx_action(struct x
 {
 	struct xenvif *vif = NULL, *tmp;
 	s8 status;
-	u16 irq, flags;
+	u16 flags;
 	struct xen_netif_rx_response *resp;
 	struct sk_buff_head rxq;
 	struct sk_buff *skb;
@@ -771,13 +771,13 @@ static void xen_netbk_rx_action(struct x
 					 sco->meta_slots_used);
 
 		RING_PUSH_RESPONSES_AND_CHECK_NOTIFY(&vif->rx, ret);
-		irq = vif->irq;
-		if (ret && list_empty(&vif->notify_list))
-			list_add_tail(&vif->notify_list, &notify);
 
 		xenvif_notify_tx_completion(vif);
 
-		xenvif_put(vif);
+		if (ret && list_empty(&vif->notify_list))
+			list_add_tail(&vif->notify_list, &notify);
+		else
+			xenvif_put(vif);
 		npo.meta_cons += sco->meta_slots_used;
 		dev_kfree_skb(skb);
 	}
@@ -785,6 +785,7 @@ static void xen_netbk_rx_action(struct x
 	list_for_each_entry_safe(vif, tmp, &notify, notify_list) {
 		notify_remote_via_irq(vif->irq);
 		list_del_init(&vif->notify_list);
+		xenvif_put(vif);
 	}
 
 	/* More work to do? */




[-- Attachment #2: linux-3.10-rc4-xen-netback-vif-use-after-free.patch --]
[-- Type: text/plain, Size: 1717 bytes --]

xen-netback: don't de-reference vif pointer after having called xenvif_put()

When putting vif-s on the rx notify list, calling xenvif_put() must be
deferred until after the removal from the list and the issuing of the
notification, as both operations dereference the pointer.

Changing this got me to notice that the "irq" variable was effectively
unused (and was of too narrow type anyway).

Signed-off-by: Jan Beulich <jbeulich@suse.com>

---
 drivers/net/xen-netback/netback.c |   11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

--- 3.10-rc4/drivers/net/xen-netback/netback.c
+++ 3.10-rc4-xen-netback-vif-use-after-free/drivers/net/xen-netback/netback.c
@@ -662,7 +662,7 @@ static void xen_netbk_rx_action(struct x
 {
 	struct xenvif *vif = NULL, *tmp;
 	s8 status;
-	u16 irq, flags;
+	u16 flags;
 	struct xen_netif_rx_response *resp;
 	struct sk_buff_head rxq;
 	struct sk_buff *skb;
@@ -771,13 +771,13 @@ static void xen_netbk_rx_action(struct x
 					 sco->meta_slots_used);
 
 		RING_PUSH_RESPONSES_AND_CHECK_NOTIFY(&vif->rx, ret);
-		irq = vif->irq;
-		if (ret && list_empty(&vif->notify_list))
-			list_add_tail(&vif->notify_list, &notify);
 
 		xenvif_notify_tx_completion(vif);
 
-		xenvif_put(vif);
+		if (ret && list_empty(&vif->notify_list))
+			list_add_tail(&vif->notify_list, &notify);
+		else
+			xenvif_put(vif);
 		npo.meta_cons += sco->meta_slots_used;
 		dev_kfree_skb(skb);
 	}
@@ -785,6 +785,7 @@ static void xen_netbk_rx_action(struct x
 	list_for_each_entry_safe(vif, tmp, &notify, notify_list) {
 		notify_remote_via_irq(vif->irq);
 		list_del_init(&vif->notify_list);
+		xenvif_put(vif);
 	}
 
 	/* More work to do? */

Re: [PATCH] xen-netback: don't de-reference vif pointer after having called xenvif_put()

[Ian Campbell]

On Wed, 2013-06-05 at 15:03 +0100, Jan Beulich wrote:
> When putting vif-s on the rx notify list, calling xenvif_put() must be
> deferred until after the removal from the list and the issuing of the
> notification, as both operations dereference the pointer.
> 
> Changing this got me to notice that the "irq" variable was effectively
> unused (and was of too narrow type anyway).
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

Re: [PATCH] xen-netback: don't de-reference vif pointer after having called xenvif_put()

[David Miller]

From: "Jan Beulich" <JBeulich@suse.com>
Date: Wed, 05 Jun 2013 15:03:11 +0100

> When putting vif-s on the rx notify list, calling xenvif_put() must be
> deferred until after the removal from the list and the issuing of the
> notification, as both operations dereference the pointer.
> 
> Changing this got me to notice that the "irq" variable was effectively
> unused (and was of too narrow type anyway).
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

You've included the commit message, and the patch, twice.  Once inline
and once as an attachment, do not do this.  It makes the patch impossible
to apply when I fetch it out of patchwork.

Please submit it properly.

Re: [PATCH] xen-netback: don't de-reference vif pointer after having called xenvif_put()

[Jan Beulich]

>>> On 11.06.13 at 11:01, David Miller <davem@davemloft.net> wrote:
> From: "Jan Beulich" <JBeulich@suse.com>
> Date: Wed, 05 Jun 2013 15:03:11 +0100
> 
>> When putting vif-s on the rx notify list, calling xenvif_put() must be
>> deferred until after the removal from the list and the issuing of the
>> notification, as both operations dereference the pointer.
>> 
>> Changing this got me to notice that the "irq" variable was effectively
>> unused (and was of too narrow type anyway).
>> 
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> You've included the commit message, and the patch, twice.  Once inline
> and once as an attachment, do not do this.  It makes the patch impossible
> to apply when I fetch it out of patchwork.
> 
> Please submit it properly.

Sending to multiple lists makes it impossible to match everyone's
preferences/requirements. xen-devel generally wants patches
inline and attached (for the case where the inline variant gets
mangled by mail programs).

I'll resend inline only, in the hope that it'll reach your inbox intact.

Jan

Re: [PATCH] xen-netback: don't de-reference vif pointer after having called xenvif_put()

[David Miller]

From: "Jan Beulich" <JBeulich@suse.com>
Date: Tue, 11 Jun 2013 10:59:39 +0100

>>>> On 11.06.13 at 11:01, David Miller <davem@davemloft.net> wrote:
>> From: "Jan Beulich" <JBeulich@suse.com>
>> Date: Wed, 05 Jun 2013 15:03:11 +0100
>> 
>>> When putting vif-s on the rx notify list, calling xenvif_put() must be
>>> deferred until after the removal from the list and the issuing of the
>>> notification, as both operations dereference the pointer.
>>> 
>>> Changing this got me to notice that the "irq" variable was effectively
>>> unused (and was of too narrow type anyway).
>>> 
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> 
>> You've included the commit message, and the patch, twice.  Once inline
>> and once as an attachment, do not do this.  It makes the patch impossible
>> to apply when I fetch it out of patchwork.
>> 
>> Please submit it properly.
> 
> Sending to multiple lists makes it impossible to match everyone's
> preferences/requirements. xen-devel generally wants patches
> inline and attached (for the case where the inline variant gets
> mangled by mail programs).

That's rediculous.  Fix your mail client setup, and send it inline
only.

Otherwise people can't reply and comment inline to the content of
your patch properly.

[PATCH, resend] xen-netback: don't de-reference vif pointer after having called xenvif_put()

[Jan Beulich]

When putting vif-s on the rx notify list, calling xenvif_put() must be
deferred until after the removal from the list and the issuing of the
notification, as both operations dereference the pointer.

Changing this got me to notice that the "irq" variable was effectively
unused (and was of too narrow type anyway).

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>

---
 drivers/net/xen-netback/netback.c |   11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

--- 3.10-rc4/drivers/net/xen-netback/netback.c
+++ 3.10-rc4-xen-netback-vif-use-after-free/drivers/net/xen-netback/netback.c
@@ -662,7 +662,7 @@ static void xen_netbk_rx_action(struct x
 {
 	struct xenvif *vif = NULL, *tmp;
 	s8 status;
-	u16 irq, flags;
+	u16 flags;
 	struct xen_netif_rx_response *resp;
 	struct sk_buff_head rxq;
 	struct sk_buff *skb;
@@ -771,13 +771,13 @@ static void xen_netbk_rx_action(struct x
 					 sco->meta_slots_used);
 
 		RING_PUSH_RESPONSES_AND_CHECK_NOTIFY(&vif->rx, ret);
-		irq = vif->irq;
-		if (ret && list_empty(&vif->notify_list))
-			list_add_tail(&vif->notify_list, &notify);
 
 		xenvif_notify_tx_completion(vif);
 
-		xenvif_put(vif);
+		if (ret && list_empty(&vif->notify_list))
+			list_add_tail(&vif->notify_list, &notify);
+		else
+			xenvif_put(vif);
 		npo.meta_cons += sco->meta_slots_used;
 		dev_kfree_skb(skb);
 	}
@@ -785,6 +785,7 @@ static void xen_netbk_rx_action(struct x
 	list_for_each_entry_safe(vif, tmp, &notify, notify_list) {
 		notify_remote_via_irq(vif->irq);
 		list_del_init(&vif->notify_list);
+		xenvif_put(vif);
 	}
 
 	/* More work to do? */

Re: [PATCH, resend] xen-netback: don't de-reference vif pointer after having called xenvif_put()

[David Miller]

From: "Jan Beulich" <JBeulich@suse.com>
Date: Tue, 11 Jun 2013 11:00:34 +0100

> When putting vif-s on the rx notify list, calling xenvif_put() must be
> deferred until after the removal from the list and the issuing of the
> notification, as both operations dereference the pointer.
> 
> Changing this got me to notice that the "irq" variable was effectively
> unused (and was of too narrow type anyway).
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Acked-by: Ian Campbell <ian.campbell@citrix.com>

Applied.