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 <[email protected]>
---
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
> -----Original Message-----
> From: [email protected] <[email protected]> On Behalf
> Of Jia-Ju Bai
> Sent: Sunday, March 18, 2018 7:53 AM
> To: KY Srinivasan <[email protected]>; Haiyang Zhang <[email protected]>; Stephen
> Hemminger <[email protected]>; [email protected]
> Cc: [email protected]; [email protected]; [email protected];
> Jia-Ju Bai <[email protected]>
> 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 <[email protected]>
> ---
> 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
> -----Original Message-----
> From: Jia-Ju Bai <[email protected]>
> Sent: Sunday, March 18, 2018 7:53 AM
> To: KY Srinivasan <[email protected]>; Haiyang Zhang
> <[email protected]>; Stephen Hemminger
> <[email protected]>; [email protected]
> Cc: [email protected]; [email protected]; linux-
> [email protected]; Jia-Ju Bai <[email protected]>
> 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 <[email protected]>
> ---
> 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
On 2018/3/19 10:52, KY Srinivasan wrote:
>
>> -----Original Message-----
>> From: Jia-Ju Bai <[email protected]>
>> Sent: Sunday, March 18, 2018 7:53 AM
>> To: KY Srinivasan <[email protected]>; Haiyang Zhang
>> <[email protected]>; Stephen Hemminger
>> <[email protected]>; [email protected]
>> Cc: [email protected]; [email protected]; linux-
>> [email protected]; Jia-Ju Bai <[email protected]>
>> 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
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
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
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