All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/2] PCI: hv: fix a couple of issues in hv_pci_onchannelcallback()
@ 2016-05-30 14:17 Vitaly Kuznetsov
  2016-05-30 14:17 ` [PATCH 1/2] PCI: hv: don't leak buffer " Vitaly Kuznetsov
                   ` (3 more replies)
  0 siblings, 4 replies; 10+ messages in thread
From: Vitaly Kuznetsov @ 2016-05-30 14:17 UTC (permalink / raw)
  To: linux-pci
  Cc: linux-kernel, devel, Bjorn Helgaas, Haiyang Zhang,
	K. Y. Srinivasan, Jake Oshins

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 1/2] PCI: hv: don't leak buffer in hv_pci_onchannelcallback()
  2016-05-30 14:17 [PATCH 0/2] PCI: hv: fix a couple of issues in hv_pci_onchannelcallback() Vitaly Kuznetsov
@ 2016-05-30 14:17 ` Vitaly Kuznetsov
  2016-05-31 17:27   ` Jake Oshins
  2016-05-30 14:17 ` [PATCH 2/2] PCI: hv: handle all pending messages " Vitaly Kuznetsov
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 10+ messages in thread
From: Vitaly Kuznetsov @ 2016-05-30 14:17 UTC (permalink / raw)
  To: linux-pci
  Cc: linux-kernel, devel, Bjorn Helgaas, Haiyang Zhang,
	K. Y. Srinivasan, Jake Oshins

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

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 2/2] PCI: hv: handle all pending messages in hv_pci_onchannelcallback()
  2016-05-30 14:17 [PATCH 0/2] PCI: hv: fix a couple of issues in hv_pci_onchannelcallback() Vitaly Kuznetsov
  2016-05-30 14:17 ` [PATCH 1/2] PCI: hv: don't leak buffer " Vitaly Kuznetsov
@ 2016-05-30 14:17 ` Vitaly Kuznetsov
  2016-05-31 17:36   ` Jake Oshins
  2016-06-10 12:05 ` [PATCH 0/2] PCI: hv: fix a couple of issues " Vitaly Kuznetsov
  2016-06-10 23:53 ` Bjorn Helgaas
  3 siblings, 1 reply; 10+ messages in thread
From: Vitaly Kuznetsov @ 2016-05-30 14:17 UTC (permalink / raw)
  To: linux-pci
  Cc: linux-kernel, devel, Bjorn Helgaas, Haiyang Zhang,
	K. Y. Srinivasan, Jake Oshins

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

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* RE: [PATCH 1/2] PCI: hv: don't leak buffer in hv_pci_onchannelcallback()
  2016-05-30 14:17 ` [PATCH 1/2] PCI: hv: don't leak buffer " Vitaly Kuznetsov
@ 2016-05-31 17:27   ` Jake Oshins
  0 siblings, 0 replies; 10+ messages in thread
From: Jake Oshins @ 2016-05-31 17:27 UTC (permalink / raw)
  To: Vitaly Kuznetsov, linux-pci
  Cc: linux-kernel, devel, Bjorn Helgaas, Haiyang Zhang, KY Srinivasan

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH 2/2] PCI: hv: handle all pending messages in hv_pci_onchannelcallback()
  2016-05-30 14:17 ` [PATCH 2/2] PCI: hv: handle all pending messages " Vitaly Kuznetsov
@ 2016-05-31 17:36   ` Jake Oshins
  0 siblings, 0 replies; 10+ messages in thread
From: Jake Oshins @ 2016-05-31 17:36 UTC (permalink / raw)
  To: Vitaly Kuznetsov, linux-pci
  Cc: linux-kernel, devel, Bjorn Helgaas, Haiyang Zhang, KY Srinivasan

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 0/2] PCI: hv: fix a couple of issues in hv_pci_onchannelcallback()
  2016-05-30 14:17 [PATCH 0/2] PCI: hv: fix a couple of issues in hv_pci_onchannelcallback() Vitaly Kuznetsov
  2016-05-30 14:17 ` [PATCH 1/2] PCI: hv: don't leak buffer " Vitaly Kuznetsov
  2016-05-30 14:17 ` [PATCH 2/2] PCI: hv: handle all pending messages " Vitaly Kuznetsov
@ 2016-06-10 12:05 ` Vitaly Kuznetsov
  2016-06-10 15:35   ` Bjorn Helgaas
  2016-06-10 23:53 ` Bjorn Helgaas
  3 siblings, 1 reply; 10+ messages in thread
From: Vitaly Kuznetsov @ 2016-06-10 12:05 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: linux-kernel, devel, Haiyang Zhang, K. Y. Srinivasan,
	Jake Oshins, linux-pci

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 0/2] PCI: hv: fix a couple of issues in hv_pci_onchannelcallback()
  2016-06-10 12:05 ` [PATCH 0/2] PCI: hv: fix a couple of issues " Vitaly Kuznetsov
@ 2016-06-10 15:35   ` Bjorn Helgaas
  0 siblings, 0 replies; 10+ messages in thread
From: Bjorn Helgaas @ 2016-06-10 15:35 UTC (permalink / raw)
  To: Vitaly Kuznetsov
  Cc: Bjorn Helgaas, linux-kernel, devel, Haiyang Zhang,
	K. Y. Srinivasan, Jake Oshins, linux-pci

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 0/2] PCI: hv: fix a couple of issues in hv_pci_onchannelcallback()
  2016-05-30 14:17 [PATCH 0/2] PCI: hv: fix a couple of issues in hv_pci_onchannelcallback() Vitaly Kuznetsov
                   ` (2 preceding siblings ...)
  2016-06-10 12:05 ` [PATCH 0/2] PCI: hv: fix a couple of issues " Vitaly Kuznetsov
@ 2016-06-10 23:53 ` Bjorn Helgaas
  2016-06-17 17:44   ` Bjorn Helgaas
  3 siblings, 1 reply; 10+ messages in thread
From: Bjorn Helgaas @ 2016-06-10 23:53 UTC (permalink / raw)
  To: Vitaly Kuznetsov
  Cc: linux-pci, linux-kernel, devel, Bjorn Helgaas, Haiyang Zhang,
	K. Y. Srinivasan, Jake Oshins

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.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 0/2] PCI: hv: fix a couple of issues in hv_pci_onchannelcallback()
  2016-06-10 23:53 ` Bjorn Helgaas
@ 2016-06-17 17:44   ` Bjorn Helgaas
  2016-06-19  8:37     ` Vitaly Kuznetsov
  0 siblings, 1 reply; 10+ messages in thread
From: Bjorn Helgaas @ 2016-06-17 17:44 UTC (permalink / raw)
  To: Vitaly Kuznetsov
  Cc: linux-pci, linux-kernel, devel, Bjorn Helgaas, Haiyang Zhang,
	K. Y. Srinivasan, Jake Oshins

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 0/2] PCI: hv: fix a couple of issues in hv_pci_onchannelcallback()
  2016-06-17 17:44   ` Bjorn Helgaas
@ 2016-06-19  8:37     ` Vitaly Kuznetsov
  0 siblings, 0 replies; 10+ messages in thread
From: Vitaly Kuznetsov @ 2016-06-19  8:37 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: linux-pci, linux-kernel, devel, Bjorn Helgaas, Haiyang Zhang,
	K. Y. Srinivasan, Jake Oshins

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2016-06-19  8:37 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-05-30 14:17 [PATCH 0/2] PCI: hv: fix a couple of issues in hv_pci_onchannelcallback() Vitaly Kuznetsov
2016-05-30 14:17 ` [PATCH 1/2] PCI: hv: don't leak buffer " Vitaly Kuznetsov
2016-05-31 17:27   ` Jake Oshins
2016-05-30 14:17 ` [PATCH 2/2] PCI: hv: handle all pending messages " Vitaly Kuznetsov
2016-05-31 17:36   ` Jake Oshins
2016-06-10 12:05 ` [PATCH 0/2] PCI: hv: fix a couple of issues " Vitaly Kuznetsov
2016-06-10 15:35   ` Bjorn Helgaas
2016-06-10 23:53 ` Bjorn Helgaas
2016-06-17 17:44   ` Bjorn Helgaas
2016-06-19  8:37     ` Vitaly Kuznetsov

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.