[PATCH] powerpc/spufs: Add rcu_read_lock() around fcheck()

[PATCH] powerpc/spufs: Add rcu_read_lock() around fcheck()

[Michael Ellerman]

Currently the spu coredump code triggers an RCU warning:

  =============================
  WARNING: suspicious RCU usage
  5.7.0-rc3-01755-g7cd49f0b7ec7 #1 Not tainted
  -----------------------------
  include/linux/fdtable.h:95 suspicious rcu_dereference_check() usage!

  other info that might help us debug this:

  rcu_scheduler_active = 2, debug_locks = 1
  1 lock held by spu-coredump/1343:
   #0: c0000007fa22f430 (sb_writers#2){.+.+}-{0:0}, at: .do_coredump+0x1010/0x13c8

  stack backtrace:
  CPU: 0 PID: 1343 Comm: spu-coredump Not tainted 5.7.0-rc3-01755-g7cd49f0b7ec7 #1
  Call Trace:
    .dump_stack+0xec/0x15c (unreliable)
    .lockdep_rcu_suspicious+0x120/0x144
    .coredump_next_context+0x148/0x158
    .spufs_coredump_extra_notes_size+0x54/0x190
    .elf_coredump_extra_notes_size+0x34/0x50
    .elf_core_dump+0xe48/0x19d0
    .do_coredump+0xe50/0x13c8
    .get_signal+0x864/0xd88
    .do_notify_resume+0x158/0x3c8
    .interrupt_exit_user_prepare+0x19c/0x208
    interrupt_return+0x14/0x1c0

This comes from fcheck_files() via fcheck().

It's pretty clearly documented that fcheck() must be wrapped with
rcu_read_lock(), so fix it.

Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
 arch/powerpc/platforms/cell/spufs/coredump.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/platforms/cell/spufs/coredump.c b/arch/powerpc/platforms/cell/spufs/coredump.c
index 8b3296b62f65..0fc52cbaa552 100644
--- a/arch/powerpc/platforms/cell/spufs/coredump.c
+++ b/arch/powerpc/platforms/cell/spufs/coredump.c
@@ -82,13 +82,19 @@ static int match_context(const void *v, struct file *file, unsigned fd)
  */
 static struct spu_context *coredump_next_context(int *fd)
 {
+	struct spu_context *ctx;
 	struct file *file;
 	int n = iterate_fd(current->files, *fd, match_context, NULL);
 	if (!n)
 		return NULL;
 	*fd = n - 1;
+
+	rcu_read_lock();
 	file = fcheck(*fd);
-	return SPUFS_I(file_inode(file))->i_ctx;
+	ctx = SPUFS_I(file_inode(file))->i_ctx;
+	rcu_read_unlock();
+
+	return ctx;
 }
 
 int spufs_coredump_extra_notes_size(void)
-- 
2.25.1

Re: [PATCH] powerpc/spufs: Add rcu_read_lock() around fcheck()

[Christoph Hellwig]

On Tue, Apr 28, 2020 at 09:48:11PM +1000, Michael Ellerman wrote:
> 
> This comes from fcheck_files() via fcheck().
> 
> It's pretty clearly documented that fcheck() must be wrapped with
> rcu_read_lock(), so fix it.

But for this to actually be useful you'd need the rcu read lock until
your are done with the file (or got a reference).

Re: [PATCH] powerpc/spufs: Add rcu_read_lock() around fcheck()

[Michael Ellerman]

Christoph Hellwig <hch@lst.de> writes:
> On Tue, Apr 28, 2020 at 09:48:11PM +1000, Michael Ellerman wrote:
>> 
>> This comes from fcheck_files() via fcheck().
>> 
>> It's pretty clearly documented that fcheck() must be wrapped with
>> rcu_read_lock(), so fix it.
>
> But for this to actually be useful you'd need the rcu read lock until
> your are done with the file (or got a reference).

Hmm OK. My reasoning was that we were done with the struct file, because
we return the ctx that's hanging off the inode.

+	ctx = SPUFS_I(file_inode(file))->i_ctx;

But I guess the lifetime of the ctx is not guaranteed if the file goes
away.

It looks like the only long lived reference on the ctx is the one
taken in spufs_new_file() and dropped in spufs_evict_inode().

So if we take a reference to the ctx with the RCU lock held we should be
safe, I think. But I've definitely exhausted my spufs/vfs knowledge at
this point.

Something like below.

cheers


diff --git a/arch/powerpc/platforms/cell/spufs/coredump.c b/arch/powerpc/platforms/cell/spufs/coredump.c
index 8b3296b62f65..37c155254cd5 100644
--- a/arch/powerpc/platforms/cell/spufs/coredump.c
+++ b/arch/powerpc/platforms/cell/spufs/coredump.c
@@ -82,13 +82,20 @@ static int match_context(const void *v, struct file *file, unsigned fd)
  */
 static struct spu_context *coredump_next_context(int *fd)
 {
+       struct spu_context *ctx;
        struct file *file;
        int n = iterate_fd(current->files, *fd, match_context, NULL);
        if (!n)
                return NULL;
        *fd = n - 1;
+
+       rcu_read_lock();
        file = fcheck(*fd);
-       return SPUFS_I(file_inode(file))->i_ctx;
+       ctx = SPUFS_I(file_inode(file))->i_ctx;
+       get_spu_context(ctx);
+       rcu_read_unlock();
+
+       return ctx;
 }

 int spufs_coredump_extra_notes_size(void)
@@ -99,17 +106,23 @@ int spufs_coredump_extra_notes_size(void)
        fd = 0;
        while ((ctx = coredump_next_context(&fd)) != NULL) {
                rc = spu_acquire_saved(ctx);
-               if (rc)
+               if (rc) {
+                       put_spu_context(ctx);
                        break;
+               }
+
                rc = spufs_ctx_note_size(ctx, fd);
                spu_release_saved(ctx);
-               if (rc < 0)
+               if (rc < 0) {
+                       put_spu_context(ctx);
                        break;
+               }

                size += rc;

                /* start searching the next fd next time */
                fd++;
+               put_spu_context(ctx);
        }

        return size;

Re: [PATCH] powerpc/spufs: Add rcu_read_lock() around fcheck()

[Christoph Hellwig]

On Wed, Apr 29, 2020 at 09:42:39PM +1000, Michael Ellerman wrote:
> Christoph Hellwig <hch@lst.de> writes:
> > On Tue, Apr 28, 2020 at 09:48:11PM +1000, Michael Ellerman wrote:
> >> 
> >> This comes from fcheck_files() via fcheck().
> >> 
> >> It's pretty clearly documented that fcheck() must be wrapped with
> >> rcu_read_lock(), so fix it.
> >
> > But for this to actually be useful you'd need the rcu read lock until
> > your are done with the file (or got a reference).
> 
> Hmm OK. My reasoning was that we were done with the struct file, because
> we return the ctx that's hanging off the inode.
> 
> +	ctx = SPUFS_I(file_inode(file))->i_ctx;
> 
> But I guess the lifetime of the ctx is not guaranteed if the file goes
> away.
> 
> It looks like the only long lived reference on the ctx is the one
> taken in spufs_new_file() and dropped in spufs_evict_inode().
> 
> So if we take a reference to the ctx with the RCU lock held we should be
> safe, I think. But I've definitely exhausted my spufs/vfs knowledge at
> this point.
> 
> Something like below.

Looks reasonable.