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

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

[Vitaly Kuznetsov]

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

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

[Vitaly Kuznetsov]

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

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

[Jake Oshins]

> -----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

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

[Vitaly Kuznetsov]

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

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

[Jake Oshins]

> -----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

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

[Vitaly Kuznetsov]

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

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

[Bjorn Helgaas]

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

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

[Bjorn Helgaas]

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.

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

[Bjorn Helgaas]

On Fri, Jun 10, 2016 at 06:53:36PM -0500, Bjorn Helgaas wrote:
> 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.

Somehow I must have been thinking these were fixes for things we
merged or broke during the v4.7 merge window, but that doesn't look
like the case.  So I'm going to merge these for v4.8 instead, on the
theory that the v4.7-rc cycles are primarily for stabilization.

Bjorn

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

[Vitaly Kuznetsov]

Bjorn Helgaas <helgaas@kernel.org> writes:

> On Fri, Jun 10, 2016 at 06:53:36PM -0500, Bjorn Helgaas wrote:
>> 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.
>
> Somehow I must have been thinking these were fixes for things we
> merged or broke during the v4.7 merge window, but that doesn't look
> like the case.  So I'm going to merge these for v4.8 instead, on the
> theory that the v4.7-rc cycles are primarily for stabilization.

Right, this is not a 4.7 regression.

Thanks!

-- 
  Vitaly