[PATCH 1/2] pci: host: pci-hyperv: Replace GFP_ATOMIC with GFP_KERNEL in hv_pci_onchannelcallback

[PATCH 1/2] pci: host: pci-hyperv: Replace GFP_ATOMIC with GFP_KERNEL in hv_pci_onchannelcallback

[Jia-Ju Bai]

hv_pci_onchannelcallback() is not called in atomic context.

The call chain ending up at hv_pci_onchannelcallback() is:
[1] hv_pci_onchannelcallback() <- hv_pci_probe()
hv_pci_probe() is only set as ".probe" in hv_driver 
structure "hv_pci_drv".

Despite never getting called from atomic context, 
hv_pci_onchannelcallback() calls kmalloc with GFP_ATOMIC, 
which waits busily for allocation.
GFP_ATOMIC is not necessary and can be replaced with GFP_KERNEL 
to avoid busy waiting.

This is found by a static analysis tool named DCNS written by myself.

Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com>
---
 drivers/pci/host/pci-hyperv.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
index 0fe3ea1..c5c8a99 100644
--- a/drivers/pci/host/pci-hyperv.c
+++ b/drivers/pci/host/pci-hyperv.c
@@ -1887,7 +1887,7 @@ static void hv_pci_onchannelcallback(void *context)
 	struct pci_dev_incoming *dev_message;
 	struct hv_pci_dev *hpdev;
 
-	buffer = kmalloc(bufferlen, GFP_ATOMIC);
+	buffer = kmalloc(bufferlen, GFP_KERNEL);
 	if (!buffer)
 		return;
 
@@ -1899,7 +1899,7 @@ static void hv_pci_onchannelcallback(void *context)
 			kfree(buffer);
 			/* Handle large packet */
 			bufferlen = bytes_recvd;
-			buffer = kmalloc(bytes_recvd, GFP_ATOMIC);
+			buffer = kmalloc(bytes_recvd, GFP_KERNEL);
 			if (!buffer)
 				return;
 			continue;
-- 
1.9.1

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH 1/2] pci: host: pci-hyperv: Replace GFP_ATOMIC with GFP_KERNEL in hv_pci_onchannelcallback

[Michael Kelley (EOSG)]

> -----Original Message-----
> From: linux-kernel-owner@vger.kernel.org <linux-kernel-owner@vger.kernel.org> On Behalf
> Of Jia-Ju Bai
> Sent: Sunday, March 18, 2018 7:53 AM
> To: KY Srinivasan <kys@microsoft.com>; Haiyang Zhang <haiyangz@microsoft.com>; Stephen
> Hemminger <sthemmin@microsoft.com>; bhelgaas@google.com
> Cc: devel@linuxdriverproject.org; linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org;
> Jia-Ju Bai <baijiaju1990@gmail.com>
> Subject: [PATCH 1/2] pci: host: pci-hyperv: Replace GFP_ATOMIC with GFP_KERNEL in
> hv_pci_onchannelcallback
> 
> hv_pci_onchannelcallback() is not called in atomic context.
> 
> The call chain ending up at hv_pci_onchannelcallback() is:
> [1] hv_pci_onchannelcallback() <- hv_pci_probe()
> hv_pci_probe() is only set as ".probe" in hv_driver
> structure "hv_pci_drv".
> 
> Despite never getting called from atomic context,

Not true.   hv_pci_probe() registers hv_pci_onchannelcallback() as
a callback function that is invoked from the VMbus interrupt handler.
So GFP_ATOMIC is appropriate.

Michael

> hv_pci_onchannelcallback() calls kmalloc with GFP_ATOMIC,
> which waits busily for allocation.
> GFP_ATOMIC is not necessary and can be replaced with GFP_KERNEL
> to avoid busy waiting.
> 
> This is found by a static analysis tool named DCNS written by myself.
> 
> Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com>
> ---
>  drivers/pci/host/pci-hyperv.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
> index 0fe3ea1..c5c8a99 100644
> --- a/drivers/pci/host/pci-hyperv.c
> +++ b/drivers/pci/host/pci-hyperv.c
> @@ -1887,7 +1887,7 @@ static void hv_pci_onchannelcallback(void *context)
>  	struct pci_dev_incoming *dev_message;
>  	struct hv_pci_dev *hpdev;
> 
> -	buffer = kmalloc(bufferlen, GFP_ATOMIC);
> +	buffer = kmalloc(bufferlen, GFP_KERNEL);
>  	if (!buffer)
>  		return;
> 
> @@ -1899,7 +1899,7 @@ static void hv_pci_onchannelcallback(void *context)
>  			kfree(buffer);
>  			/* Handle large packet */
>  			bufferlen = bytes_recvd;
> -			buffer = kmalloc(bytes_recvd, GFP_ATOMIC);
> +			buffer = kmalloc(bytes_recvd, GFP_KERNEL);
>  			if (!buffer)
>  				return;
>  			continue;
> --
> 1.9.1

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH 1/2] pci: host: pci-hyperv: Replace GFP_ATOMIC with GFP_KERNEL in hv_pci_onchannelcallback

[KY Srinivasan]

> -----Original Message-----
> From: Jia-Ju Bai <baijiaju1990@gmail.com>
> Sent: Sunday, March 18, 2018 7:53 AM
> To: KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
> <haiyangz@microsoft.com>; Stephen Hemminger
> <sthemmin@microsoft.com>; bhelgaas@google.com
> Cc: devel@linuxdriverproject.org; linux-pci@vger.kernel.org; linux-
> kernel@vger.kernel.org; Jia-Ju Bai <baijiaju1990@gmail.com>
> Subject: [PATCH 1/2] pci: host: pci-hyperv: Replace GFP_ATOMIC with
> GFP_KERNEL in hv_pci_onchannelcallback
> 
> hv_pci_onchannelcallback() is not called in atomic context.
> 
> The call chain ending up at hv_pci_onchannelcallback() is:
> [1] hv_pci_onchannelcallback() <- hv_pci_probe()
> hv_pci_probe() is only set as ".probe" in hv_driver
> structure "hv_pci_drv".

This function is setup as the function to handle interrupts on the channel. Hence the
need for GFP_ATOMIC.

K. Y
> 
> Despite never getting called from atomic context,
> hv_pci_onchannelcallback() calls kmalloc with GFP_ATOMIC,
> which waits busily for allocation.
> GFP_ATOMIC is not necessary and can be replaced with GFP_KERNEL
> to avoid busy waiting.
> 
> This is found by a static analysis tool named DCNS written by myself.
> 
> Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com>
> ---
>  drivers/pci/host/pci-hyperv.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
> index 0fe3ea1..c5c8a99 100644
> --- a/drivers/pci/host/pci-hyperv.c
> +++ b/drivers/pci/host/pci-hyperv.c
> @@ -1887,7 +1887,7 @@ static void hv_pci_onchannelcallback(void
> *context)
>  	struct pci_dev_incoming *dev_message;
>  	struct hv_pci_dev *hpdev;
> 
> -	buffer = kmalloc(bufferlen, GFP_ATOMIC);
> +	buffer = kmalloc(bufferlen, GFP_KERNEL);
>  	if (!buffer)
>  		return;
> 
> @@ -1899,7 +1899,7 @@ static void hv_pci_onchannelcallback(void
> *context)
>  			kfree(buffer);
>  			/* Handle large packet */
>  			bufferlen = bytes_recvd;
> -			buffer = kmalloc(bytes_recvd, GFP_ATOMIC);
> +			buffer = kmalloc(bytes_recvd, GFP_KERNEL);
>  			if (!buffer)
>  				return;
>  			continue;
> --
> 1.9.1

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/2] pci: host: pci-hyperv: Replace GFP_ATOMIC with GFP_KERNEL in hv_pci_onchannelcallback

[Jia-Ju Bai]

On 2018/3/19 10:52, KY Srinivasan wrote:
>
>> -----Original Message-----
>> From: Jia-Ju Bai <baijiaju1990@gmail.com>
>> Sent: Sunday, March 18, 2018 7:53 AM
>> To: KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
>> <haiyangz@microsoft.com>; Stephen Hemminger
>> <sthemmin@microsoft.com>; bhelgaas@google.com
>> Cc: devel@linuxdriverproject.org; linux-pci@vger.kernel.org; linux-
>> kernel@vger.kernel.org; Jia-Ju Bai <baijiaju1990@gmail.com>
>> Subject: [PATCH 1/2] pci: host: pci-hyperv: Replace GFP_ATOMIC with
>> GFP_KERNEL in hv_pci_onchannelcallback
>>
>> hv_pci_onchannelcallback() is not called in atomic context.
>>
>> The call chain ending up at hv_pci_onchannelcallback() is:
>> [1] hv_pci_onchannelcallback() <- hv_pci_probe()
>> hv_pci_probe() is only set as ".probe" in hv_driver
>> structure "hv_pci_drv".
> This function is setup as the function to handle interrupts on the channel. Hence the
> need for GFP_ATOMIC.
>

Oh, sorry for my incorrect patch.
Thanks for your reply :)


Best wishes,
Jia-Ju Bai
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/2] pci: host: pci-hyperv: Replace GFP_ATOMIC with GFP_KERNEL in hv_pci_onchannelcallback

[Dan Carpenter]

On Sun, Mar 18, 2018 at 10:53:02PM +0800, Jia-Ju Bai wrote:
> hv_pci_onchannelcallback() is not called in atomic context.
> 
> The call chain ending up at hv_pci_onchannelcallback() is:
> [1] hv_pci_onchannelcallback() <- hv_pci_probe()
> hv_pci_probe() is only set as ".probe" in hv_driver 
> structure "hv_pci_drv".
> 

Your static analysis tool is faulty and apparently so is Smatch.

$ smdb function_ptrs hv_pci_onchannelcallback

Says it can't find a caller.  When I look for function pointers I get:

$ smdb function_ptr hv_pci_onchannelcallback
hv_pci_onchannelcallback =  'hv_pci_onchannelcallback' , 'vmbus_open param 5' , '(struct vmbus_channel)->onchannel_callback' , '__read_once_size param 0'

Anyway the call tree is:

vmbus_chan_sched() <-- takes rcu_read_lock();
-> vmbus_channel_isr()
   -> channel->onchannel_callback() -> which is hv_pci_onchannelcallback()

regards,
dan carpenter

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/2] pci: host: pci-hyperv: Replace GFP_ATOMIC with GFP_KERNEL in hv_pci_onchannelcallback

[Jia-Ju Bai]

On 2018/3/19 16:38, Dan Carpenter wrote:
> On Sun, Mar 18, 2018 at 10:53:02PM +0800, Jia-Ju Bai wrote:
>> hv_pci_onchannelcallback() is not called in atomic context.
>>
>> The call chain ending up at hv_pci_onchannelcallback() is:
>> [1] hv_pci_onchannelcallback() <- hv_pci_probe()
>> hv_pci_probe() is only set as ".probe" in hv_driver
>> structure "hv_pci_drv".
>>
> Your static analysis tool is faulty and apparently so is Smatch.
>
> $ smdb function_ptrs hv_pci_onchannelcallback
>
> Says it can't find a caller.  When I look for function pointers I get:
>
> $ smdb function_ptr hv_pci_onchannelcallback
> hv_pci_onchannelcallback =  'hv_pci_onchannelcallback' , 'vmbus_open param 5' , '(struct vmbus_channel)->onchannel_callback' , '__read_once_size param 0'
>
> Anyway the call tree is:
>
> vmbus_chan_sched() <-- takes rcu_read_lock();
> -> vmbus_channel_isr()
>     -> channel->onchannel_callback() -> which is hv_pci_onchannelcallback(

Thanks for your reply :)
I admit my tool produces a false positive for this code...
Sorry for my incorrect patch.

Anyway, I find that function pointers are quite hard to analyze in the 
Linux kernel code, because their usages are often flexible.
Have you found a good way to handle function pointers? Or can you 
recommend some good tools to handle them?


Best wishes,
Jia-Ju Bai
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/2] pci: host: pci-hyperv: Replace GFP_ATOMIC with GFP_KERNEL in hv_pci_onchannelcallback

[Dan Carpenter]

Smatch tracks information about every function call.  When a function
pointer is called it maybe looks something like this:

     kernel/module.c |   SYSC_delete_module | (struct module)->exit |    INTERNAL |  -1 |  | void(*)()

So then we just have to know what functions are assigned to
module->exit.

I also filter based on the function signature "void(*)()" because
there are a couple places where we cut and pasted so the structs can
have the same name and function pointer name as a member but take
different arguments.

regards,
dan carpenter

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel