[PATCH] fs/afs/flock and fs/locks: Fix possible sleep-in-atomic bugs in posix_lock_file

[PATCH] fs/afs/flock and fs/locks: Fix possible sleep-in-atomic bugs in posix_lock_file

[Jia-Ju Bai]

The kernel may sleep under a spinlock, and the function call paths are:
afs_do_unlk (acquire the spinlock)
  posix_lock_file
    posix_lock_inode (fs/locks.c)
      locks_get_lock_context
        kmem_cache_alloc(GFP_KERNEL) --> may sleep

afs_do_setlk (acquire the spinlock)
  posix_lock_file
    posix_lock_inode (fs/locks.c)
      locks_get_lock_context
        kmem_cache_alloc(GFP_KERNEL) --> may sleep

To fix them, GFP_KERNEL is replaced with GFP_ATOMIC.
These bugs are found by my static analysis tool and my code review.

Signed-off-by: Jia-Ju Bai <baijiaju1990@163.com>
---
 fs/locks.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/locks.c b/fs/locks.c
index 1bd71c4..975cc62 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -222,7 +222,7 @@ struct file_lock_list_struct {
 	if (likely(ctx) || type == F_UNLCK)
 		goto out;
 
-	ctx = kmem_cache_alloc(flctx_cache, GFP_KERNEL);
+	ctx = kmem_cache_alloc(flctx_cache, GFP_ATOMIC);
 	if (!ctx)
 		goto out;
 
-- 
1.7.9.5

Re: [PATCH] fs/afs/flock and fs/locks: Fix possible sleep-in-atomic bugs in posix_lock_file

[Jeff Layton]

On Sat, 2017-10-07 at 17:55 +0800, Jia-Ju Bai wrote:
> The kernel may sleep under a spinlock, and the function call paths are:
> afs_do_unlk (acquire the spinlock)
>   posix_lock_file
>     posix_lock_inode (fs/locks.c)
>       locks_get_lock_context
>         kmem_cache_alloc(GFP_KERNEL) --> may sleep
> 
> afs_do_setlk (acquire the spinlock)
>   posix_lock_file
>     posix_lock_inode (fs/locks.c)
>       locks_get_lock_context
>         kmem_cache_alloc(GFP_KERNEL) --> may sleep
> 
> To fix them, GFP_KERNEL is replaced with GFP_ATOMIC.
> These bugs are found by my static analysis tool and my code review.
> 
> Signed-off-by: Jia-Ju Bai <baijiaju1990@163.com>
> ---
>  fs/locks.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/locks.c b/fs/locks.c
> index 1bd71c4..975cc62 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -222,7 +222,7 @@ struct file_lock_list_struct {
>  	if (likely(ctx) || type == F_UNLCK)
>  		goto out;
>  
> -	ctx = kmem_cache_alloc(flctx_cache, GFP_KERNEL);
> +	ctx = kmem_cache_alloc(flctx_cache, GFP_ATOMIC);
>  	if (!ctx)
>  		goto out;
>  

NAK

This needs to be fixed in the AFS code. It should not be calling these
functions with a spinlock held.

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH] fs/afs/flock and fs/locks: Fix possible sleep-in-atomic bugs in posix_lock_file

[J. Bruce Fields]

On Sat, Oct 07, 2017 at 06:36:57AM -0400, Jeff Layton wrote:
> On Sat, 2017-10-07 at 17:55 +0800, Jia-Ju Bai wrote:
> > The kernel may sleep under a spinlock, and the function call paths are:
> > afs_do_unlk (acquire the spinlock)
> >   posix_lock_file
> >     posix_lock_inode (fs/locks.c)
> >       locks_get_lock_context
> >         kmem_cache_alloc(GFP_KERNEL) --> may sleep
> > 
> > afs_do_setlk (acquire the spinlock)
> >   posix_lock_file
> >     posix_lock_inode (fs/locks.c)
> >       locks_get_lock_context
> >         kmem_cache_alloc(GFP_KERNEL) --> may sleep
> > 
> > To fix them, GFP_KERNEL is replaced with GFP_ATOMIC.
> > These bugs are found by my static analysis tool and my code review.
> > 
> > Signed-off-by: Jia-Ju Bai <baijiaju1990@163.com>
> > ---
> >  fs/locks.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/fs/locks.c b/fs/locks.c
> > index 1bd71c4..975cc62 100644
> > --- a/fs/locks.c
> > +++ b/fs/locks.c
> > @@ -222,7 +222,7 @@ struct file_lock_list_struct {
> >  	if (likely(ctx) || type == F_UNLCK)
> >  		goto out;
> >  
> > -	ctx = kmem_cache_alloc(flctx_cache, GFP_KERNEL);
> > +	ctx = kmem_cache_alloc(flctx_cache, GFP_ATOMIC);
> >  	if (!ctx)
> >  		goto out;
> >  
> 
> NAK
> 
> This needs to be fixed in the AFS code. It should not be calling these
> functions with a spinlock held.

Agreed.

>From a quick look at afs_do_setlk: am I misreading something, or is it
actually trying to do an rpc call to the server while holding i_lock?

I wonder if this is the fault of the BKL conversion: 72f98e72551f
"locks: turn lock_flocks into a spinlock" claims "nothing depends on
lock_flocks using the BKL any more, so we can do the switch over to a
private spinlock."  But this code, with lots of blockers, was under
lock_flocks().  Does that mean nobody's tested fcntl locking over afs
since that change in 2010?

--b.

Re: [PATCH] fs/afs/flock and fs/locks: Fix possible sleep-in-atomic bugs in posix_lock_file

[David Howells]

J. Bruce Fields <bfields@fieldses.org> wrote:

> Does that mean nobody's tested fcntl locking over afs since that change in
> 2010?

Quite feasibly not.  I've been beating kAFS into shape and I'm aware of the
lock thing.  It's on the list after getting the core server rotation working
properly since that will affect a whole bunch of code, including the locking
code.

I've more or less finished the server rotation bit.  See:

	https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/log/?h=afs-2-experimental

David

Re: [PATCH] fs/afs/flock and fs/locks: Fix possible sleep-in-atomic bugs in posix_lock_file

[J. Bruce Fields]

On Wed, Oct 11, 2017 at 10:47:43AM +0100, David Howells wrote:
> J. Bruce Fields <bfields@fieldses.org> wrote:
> 
> > Does that mean nobody's tested fcntl locking over afs since that change in
> > 2010?
> 
> Quite feasibly not.  I've been beating kAFS into shape and I'm aware of the
> lock thing.  It's on the list after getting the core server rotation working
> properly since that will affect a whole bunch of code, including the locking
> code.
> 
> I've more or less finished the server rotation bit.  See:
> 
> 	https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/log/?h=afs-2-experimental

Neat!

Well, pending a real fix I suppose it's probably clearest to leave the
locking code in an obviously broken state.  Might be worth a comment
though.

--b.