Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1409153 > unrolled thread

[PATCH 0/2] PCI: hv: fix a couple of issues in hv_pci_onchannelcallback()

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/2] PCI: hv: fix a couple of issues in hv_pci_onchannelcallback() Vitaly Kuznetsov <vkuznets@redhat.com> - 2016-05-30 16:20 +0200
    [PATCH 2/2] PCI: hv: handle all pending messages in hv_pci_onchannelcallback() Vitaly Kuznetsov <vkuznets@redhat.com> - 2016-05-30 16:20 +0200
      RE: [PATCH 2/2] PCI: hv: handle all pending messages in  hv_pci_onchannelcallback() Jake Oshins <jakeo@microsoft.com> - 2016-05-31 19:40 +0200
    [PATCH 1/2] PCI: hv: don't leak buffer in hv_pci_onchannelcallback() Vitaly Kuznetsov <vkuznets@redhat.com> - 2016-05-30 16:20 +0200
      RE: [PATCH 1/2] PCI: hv: don't leak buffer in  hv_pci_onchannelcallback() Jake Oshins <jakeo@microsoft.com> - 2016-05-31 20:10 +0200
    Re: [PATCH 0/2] PCI: hv: fix a couple of issues in hv_pci_onchannelcallback() Vitaly Kuznetsov <vkuznets@redhat.com> - 2016-06-10 14:10 +0200
      Re: [PATCH 0/2] PCI: hv: fix a couple of issues in  hv_pci_onchannelcallback() Bjorn Helgaas <helgaas@kernel.org> - 2016-06-10 17:40 +0200
    Re: [PATCH 0/2] PCI: hv: fix a couple of issues in  hv_pci_onchannelcallback() Bjorn Helgaas <helgaas@kernel.org> - 2016-06-11 02:00 +0200

#1409153 — [PATCH 0/2] PCI: hv: fix a couple of issues in hv_pci_onchannelcallback()

kmemleak helped me to identify a memory leak in hv_pci_onchannelcallback()
and while fixing it I stumbled upon an unrelated issue(s) there.

Vitaly Kuznetsov (2):
  PCI: hv: don't leak buffer in hv_pci_onchannelcallback()
  PCI: hv: handle all pending messages in hv_pci_onchannelcallback()

 drivers/pci/host/pci-hyperv.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

-- 
2.5.5

[toc] | [next] | [standalone]

#1409154 — [PATCH 2/2] PCI: hv: handle all pending messages in hv_pci_onchannelcallback()

When we have an interrupt from host we have a bit set in event page
indicating there are messages for the particular channel. We need to read
them all as we won't get signaled for what was on the queue before we
cleared the bit in vmbus_on_event(). This applies to all Hyper-V drivers
and the pass-through driver should do the same.
I did non meet any bugs, the issue was found by code inspection. We don't
have many events going through hv_pci_onchannelcallback(), this explains
why nobody reported the issue before.

While on it, fix handling non-zero vmbus_recvpacket_raw() return values by
dropping out. If the return value is not zero it is wrong to inspect
buffer or bytes_recvd as these may contain invalid data.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 drivers/pci/host/pci-hyperv.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
index a68ec49..7de341d 100644
--- a/drivers/pci/host/pci-hyperv.c
+++ b/drivers/pci/host/pci-hyperv.c
@@ -1657,12 +1657,16 @@ static void hv_pci_onchannelcallback(void *context)
 			continue;
 		}
 
+		/* Zero length indicates there are no more packets. */
+		if (ret || !bytes_recvd)
+			break;
+
 		/*
 		 * All incoming packets must be at least as large as a
 		 * response.
 		 */
 		if (bytes_recvd <= sizeof(struct pci_response))
-			break;
+			continue;
 		desc = (struct vmpacket_descriptor *)buffer;
 
 		switch (desc->type) {
@@ -1724,7 +1728,6 @@ static void hv_pci_onchannelcallback(void *context)
 				desc->type, req_id, bytes_recvd);
 			break;
 		}
-		break;
 	}
 
 	kfree(buffer);
-- 
2.5.5

[toc] | [prev] | [next] | [standalone]

#1410484 — RE: [PATCH 2/2] PCI: hv: handle all pending messages in hv_pci_onchannelcallback()

> -----Original Message-----
> From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
> Sent: Monday, May 30, 2016 7:18 AM
> To: linux-pci@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org; devel@linuxdriverproject.org; Bjorn
> Helgaas <bhelgaas@google.com>; Haiyang Zhang
> <haiyangz@microsoft.com>; KY Srinivasan <kys@microsoft.com>; Jake
> Oshins <jakeo@microsoft.com>
> Subject: [PATCH 2/2] PCI: hv: handle all pending messages in
> hv_pci_onchannelcallback()
> 
> When we have an interrupt from host we have a bit set in event page
> indicating there are messages for the particular channel. We need to read
> them all as we won't get signaled for what was on the queue before we
> cleared the bit in vmbus_on_event(). This applies to all Hyper-V drivers
> and the pass-through driver should do the same.
> I did non meet any bugs, the issue was found by code inspection. We don't
> have many events going through hv_pci_onchannelcallback(), this explains
> why nobody reported the issue before.
> 
> While on it, fix handling non-zero vmbus_recvpacket_raw() return values by
> dropping out. If the return value is not zero it is wrong to inspect
> buffer or bytes_recvd as these may contain invalid data.
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
Acked-by: Jake Oshins <jakeo@microsoft.com>

> ---
>  drivers/pci/host/pci-hyperv.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
> index a68ec49..7de341d 100644
> --- a/drivers/pci/host/pci-hyperv.c
> +++ b/drivers/pci/host/pci-hyperv.c
> @@ -1657,12 +1657,16 @@ static void hv_pci_onchannelcallback(void
> *context)
>  			continue;
>  		}
> 
> +		/* Zero length indicates there are no more packets. */
> +		if (ret || !bytes_recvd)
> +			break;
> +
>  		/*
>  		 * All incoming packets must be at least as large as a
>  		 * response.
>  		 */
>  		if (bytes_recvd <= sizeof(struct pci_response))
> -			break;
> +			continue;
>  		desc = (struct vmpacket_descriptor *)buffer;
> 
>  		switch (desc->type) {
> @@ -1724,7 +1728,6 @@ static void hv_pci_onchannelcallback(void
> *context)
>  				desc->type, req_id, bytes_recvd);
>  			break;
>  		}
> -		break;
>  	}
> 
>  	kfree(buffer);
> --
> 2.5.5

This is good, too.

Thanks,
Jake Oshins

[toc] | [prev] | [next] | [standalone]

#1409158 — [PATCH 1/2] PCI: hv: don't leak buffer in hv_pci_onchannelcallback()

We don't free buffer on several code paths in hv_pci_onchannelcallback(),
put kfree() to the end of the function to fix the issue. Direct { kfree();
return; } can now be replaced with a simple 'break';

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 drivers/pci/host/pci-hyperv.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
index 7e9b2de..a68ec49 100644
--- a/drivers/pci/host/pci-hyperv.c
+++ b/drivers/pci/host/pci-hyperv.c
@@ -1661,10 +1661,8 @@ static void hv_pci_onchannelcallback(void *context)
 		 * All incoming packets must be at least as large as a
 		 * response.
 		 */
-		if (bytes_recvd <= sizeof(struct pci_response)) {
-			kfree(buffer);
-			return;
-		}
+		if (bytes_recvd <= sizeof(struct pci_response))
+			break;
 		desc = (struct vmpacket_descriptor *)buffer;
 
 		switch (desc->type) {
@@ -1679,8 +1677,7 @@ static void hv_pci_onchannelcallback(void *context)
 			comp_packet->completion_func(comp_packet->compl_ctxt,
 						     response,
 						     bytes_recvd);
-			kfree(buffer);
-			return;
+			break;
 
 		case VM_PKT_DATA_INBAND:
 
@@ -1729,6 +1726,8 @@ static void hv_pci_onchannelcallback(void *context)
 		}
 		break;
 	}
+
+	kfree(buffer);
 }
 
 /**
-- 
2.5.5

[toc] | [prev] | [next] | [standalone]

#1410509 — RE: [PATCH 1/2] PCI: hv: don't leak buffer in hv_pci_onchannelcallback()

> -----Original Message-----
> From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
> Sent: Monday, May 30, 2016 7:18 AM
> To: linux-pci@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org; devel@linuxdriverproject.org; Bjorn
> Helgaas <bhelgaas@google.com>; Haiyang Zhang
> <haiyangz@microsoft.com>; KY Srinivasan <kys@microsoft.com>; Jake
> Oshins <jakeo@microsoft.com>
> Subject: [PATCH 1/2] PCI: hv: don't leak buffer in hv_pci_onchannelcallback()
> 
> We don't free buffer on several code paths in hv_pci_onchannelcallback(),
> put kfree() to the end of the function to fix the issue. Direct { kfree();
> return; } can now be replaced with a simple 'break';
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
Acked-by: Jake Oshins <jakeo@microsoft.com>

> ---
>  drivers/pci/host/pci-hyperv.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
> index 7e9b2de..a68ec49 100644
> --- a/drivers/pci/host/pci-hyperv.c
> +++ b/drivers/pci/host/pci-hyperv.c
> @@ -1661,10 +1661,8 @@ static void hv_pci_onchannelcallback(void
> *context)
>  		 * All incoming packets must be at least as large as a
>  		 * response.
>  		 */
> -		if (bytes_recvd <= sizeof(struct pci_response)) {
> -			kfree(buffer);
> -			return;
> -		}
> +		if (bytes_recvd <= sizeof(struct pci_response))
> +			break;
>  		desc = (struct vmpacket_descriptor *)buffer;
> 
>  		switch (desc->type) {
> @@ -1679,8 +1677,7 @@ static void hv_pci_onchannelcallback(void
> *context)
>  			comp_packet->completion_func(comp_packet-
> >compl_ctxt,
>  						     response,
>  						     bytes_recvd);
> -			kfree(buffer);
> -			return;
> +			break;
> 
>  		case VM_PKT_DATA_INBAND:
> 
> @@ -1729,6 +1726,8 @@ static void hv_pci_onchannelcallback(void
> *context)
>  		}
>  		break;
>  	}
> +
> +	kfree(buffer);
>  }
> 
>  /**
> --
> 2.5.5

This is a good fix.  Thanks.

-- Jake Oshins

[toc] | [prev] | [next] | [standalone]

#1419312

Vitaly Kuznetsov <vkuznets@redhat.com> writes:

> kmemleak helped me to identify a memory leak in hv_pci_onchannelcallback()
> and while fixing it I stumbled upon an unrelated issue(s) there.
>
> Vitaly Kuznetsov (2):
>   PCI: hv: don't leak buffer in hv_pci_onchannelcallback()
>   PCI: hv: handle all pending messages in hv_pci_onchannelcallback()
>

Bjorn,

sorry for the ping but with both patches acked by Jake is there anything
else required for this series to get merged? It would be nice to have
these fixes in 4.7 but even knowing that they're queued for 4.8 is OK.

Thanks,

-- 
  Vitaly

[toc] | [prev] | [next] | [standalone]

#1419526 — Re: [PATCH 0/2] PCI: hv: fix a couple of issues in hv_pci_onchannelcallback()

On Fri, Jun 10, 2016 at 02:05:33PM +0200, Vitaly Kuznetsov wrote:
> Vitaly Kuznetsov <vkuznets@redhat.com> writes:
> 
> > kmemleak helped me to identify a memory leak in hv_pci_onchannelcallback()
> > and while fixing it I stumbled upon an unrelated issue(s) there.
> >
> > Vitaly Kuznetsov (2):
> >   PCI: hv: don't leak buffer in hv_pci_onchannelcallback()
> >   PCI: hv: handle all pending messages in hv_pci_onchannelcallback()
> >
> 
> Bjorn,
> 
> sorry for the ping but with both patches acked by Jake is there anything
> else required for this series to get merged? It would be nice to have
> these fixes in 4.7 but even knowing that they're queued for 4.8 is OK.

Nothing else required, but I'm glad you mentioned that these should go
in v4.7.  By default I merge things to -next, which would be for v4.8
right now.

Bjorn

[toc] | [prev] | [next] | [standalone]

#1419840 — Re: [PATCH 0/2] PCI: hv: fix a couple of issues in hv_pci_onchannelcallback()

On Mon, May 30, 2016 at 04:17:57PM +0200, Vitaly Kuznetsov wrote:
> kmemleak helped me to identify a memory leak in hv_pci_onchannelcallback()
> and while fixing it I stumbled upon an unrelated issue(s) there.
> 
> Vitaly Kuznetsov (2):
>   PCI: hv: don't leak buffer in hv_pci_onchannelcallback()
>   PCI: hv: handle all pending messages in hv_pci_onchannelcallback()

I applied both to for-linus for v4.7 with Jake's acks, thanks, Vitaly.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web