[PATCH] staging: zcache: fix possible sleep under lock

[PATCH] staging: zcache: fix possible sleep under lock

[Seth Jennings]

zcache_new_pool() calls kmalloc() with GFP_KERNEL which has
__GFP_WAIT set.  However, zcache_new_pool() gets called on
a stack that holds the swap_lock spinlock, leading to a
possible sleep-with-lock situation. The lock is obtained
in enable_swap_info().

The patch replaces GFP_KERNEL with GFP_IOFS, which is
GFP_KERNEL & ~__GFP_WAIT.

Signed-off-by: Seth Jennings <sjenning@linux.vnet.ibm.com>
---
 drivers/staging/zcache/zcache-main.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/staging/zcache/zcache-main.c b/drivers/staging/zcache/zcache-main.c
index 855a5bb..96ca0ee 100644
--- a/drivers/staging/zcache/zcache-main.c
+++ b/drivers/staging/zcache/zcache-main.c
@@ -1668,7 +1668,7 @@ static int zcache_new_pool(uint16_t cli_id, uint32_t flags)
 	if (cli == NULL)
 		goto out;
 	atomic_inc(&cli->refcount);
-	pool = kmalloc(sizeof(struct tmem_pool), GFP_KERNEL);
+	pool = kmalloc(sizeof(struct tmem_pool), GFP_IOFS);
 	if (pool == NULL) {
 		pr_info("zcache: pool creation failed: out of memory\n");
 		goto out;
-- 
1.7.4.1

Re: [PATCH] staging: zcache: fix possible sleep under lock

[Dan Carpenter]

On Mon, Aug 22, 2011 at 01:47:49PM -0500, Seth Jennings wrote:
> zcache_new_pool() calls kmalloc() with GFP_KERNEL which has
> __GFP_WAIT set.  However, zcache_new_pool() gets called on
> a stack that holds the swap_lock spinlock, leading to a
> possible sleep-with-lock situation. The lock is obtained
> in enable_swap_info().
> 
> The patch replaces GFP_KERNEL with GFP_IOFS, which is
> GFP_KERNEL & ~__GFP_WAIT.
> 

You should use GFP_ATOMIC.  We don't want to do IO with the locks
held.  The only reason GFP_IOFS exists is so that we can turn off
io during suspend and resume.

regards,
dan carpenter

Re: [PATCH] staging: zcache: fix possible sleep under lock

[Seth Jennings]

On 08/22/2011 02:09 PM, Dan Carpenter wrote:
> On Mon, Aug 22, 2011 at 01:47:49PM -0500, Seth Jennings wrote:
>> zcache_new_pool() calls kmalloc() with GFP_KERNEL which has
>> __GFP_WAIT set.  However, zcache_new_pool() gets called on
>> a stack that holds the swap_lock spinlock, leading to a
>> possible sleep-with-lock situation. The lock is obtained
>> in enable_swap_info().
>>
>> The patch replaces GFP_KERNEL with GFP_IOFS, which is
>> GFP_KERNEL & ~__GFP_WAIT.
>>
> 
> You should use GFP_ATOMIC.  We don't want to do IO with the locks
> held.  The only reason GFP_IOFS exists is so that we can turn off
> io during suspend and resume.
> 
> regards,
> dan carpenter
> 
I guess I was looking to change it as little as possible and didn't 
know what allocations should be allowed to use the "emergency pool" of
pages.

I'll update and resend.

Thanks
--
Seth

Re: [PATCH] staging: zcache: fix possible sleep under lock

[Seth Jennings]

On 08/22/2011 02:09 PM, Dan Carpenter wrote:
> On Mon, Aug 22, 2011 at 01:47:49PM -0500, Seth Jennings wrote:
>> zcache_new_pool() calls kmalloc() with GFP_KERNEL which has
>> __GFP_WAIT set.  However, zcache_new_pool() gets called on
>> a stack that holds the swap_lock spinlock, leading to a
>> possible sleep-with-lock situation. The lock is obtained
>> in enable_swap_info().
>>
>> The patch replaces GFP_KERNEL with GFP_IOFS, which is
>> GFP_KERNEL & ~__GFP_WAIT.
>>
> 
> You should use GFP_ATOMIC.  We don't want to do IO with the locks
> held.  The only reason GFP_IOFS exists is so that we can turn off
> io during suspend and resume.
> 
> regards,
> dan carpenter
> 
Actually, should this be GFP_ATOMIC or GFP_NOWAIT?

--
Seth

Re: [PATCH] staging: zcache: fix possible sleep under lock

[Dan Carpenter]

On Mon, Aug 22, 2011 at 02:49:45PM -0500, Seth Jennings wrote:
> Actually, should this be GFP_ATOMIC or GFP_NOWAIT?
> 

GFP_ATOMIC is sort of a good default answer.

GFP_NOWAIT is normally used when you want to do something really
fast and if the allocation fails, you don't want to wait for it.
So if memory is short, and you drop a packet?  Who cares!  TCP has
error handling built in.  Other than that, GFP_NOWAIT is used a lot
in the core kernel.

You could be right that GFP_NOWAIT is fine here.  I don't know zcache
well enough to say.  How bad is it if the allocation fails?

regards,
dan carpenter

Re: [PATCH] staging: zcache: fix possible sleep under lock

[Seth Jennings]

On 08/22/2011 04:10 PM, Dan Carpenter wrote:
> On Mon, Aug 22, 2011 at 02:49:45PM -0500, Seth Jennings wrote:
>> Actually, should this be GFP_ATOMIC or GFP_NOWAIT?
>>
> 
> GFP_ATOMIC is sort of a good default answer.
> 
> GFP_NOWAIT is normally used when you want to do something really
> fast and if the allocation fails, you don't want to wait for it.
> So if memory is short, and you drop a packet?  Who cares!  TCP has
> error handling built in.  Other than that, GFP_NOWAIT is used a lot
> in the core kernel.
> 
> You could be right that GFP_NOWAIT is fine here.  I don't know zcache
> well enough to say.  How bad is it if the allocation fails?
> 
> regards,
> dan carpenter

Meh... I think GFP_ATOMIC is fine.  If the allocation fails, then zcache
fails to initialise and the page cache and swaps just go down their normal
non-zache/frontswap/cleancache paths.  The only time there is a difference
between GFP_ATOMIC and GFP_NOWAIT, AFAIK, is if there are no non-emergency pages
left, which is unlikely to be the case.

Plus, I don't want to have to send out v3 of a one line patch :-/

Thanks,
Seth