Commit 38b407be authored by Michael Ellerman's avatar Michael Ellerman

powerpc/spufs: Rework fcheck() usage

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(), adding that fixes the RCU warning.

hch points out that once we've released the RCU read lock the file may
be closed and freed, which would leave us with a pointer to a freed
spu_context.

To avoid that, take a reference to the spu_context while we hold the
RCU read lock, and drop that reference later once we're done with the
context.
Signed-off-by: default avatarMichael Ellerman <mpe@ellerman.id.au>
Link: https://lore.kernel.org/r/20200508130633.2532759-1-mpe@ellerman.id.au
parent 9d1ebe9a
...@@ -66,13 +66,20 @@ static int match_context(const void *v, struct file *file, unsigned fd) ...@@ -66,13 +66,20 @@ static int match_context(const void *v, struct file *file, unsigned fd)
*/ */
static struct spu_context *coredump_next_context(int *fd) static struct spu_context *coredump_next_context(int *fd)
{ {
struct spu_context *ctx;
struct file *file; struct file *file;
int n = iterate_fd(current->files, *fd, match_context, NULL); int n = iterate_fd(current->files, *fd, match_context, NULL);
if (!n) if (!n)
return NULL; return NULL;
*fd = n - 1; *fd = n - 1;
rcu_read_lock();
file = fcheck(*fd); 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) int spufs_coredump_extra_notes_size(void)
...@@ -83,17 +90,23 @@ int spufs_coredump_extra_notes_size(void) ...@@ -83,17 +90,23 @@ int spufs_coredump_extra_notes_size(void)
fd = 0; fd = 0;
while ((ctx = coredump_next_context(&fd)) != NULL) { while ((ctx = coredump_next_context(&fd)) != NULL) {
rc = spu_acquire_saved(ctx); rc = spu_acquire_saved(ctx);
if (rc) if (rc) {
put_spu_context(ctx);
break; break;
}
rc = spufs_ctx_note_size(ctx, fd); rc = spufs_ctx_note_size(ctx, fd);
spu_release_saved(ctx); spu_release_saved(ctx);
if (rc < 0) if (rc < 0) {
put_spu_context(ctx);
break; break;
}
size += rc; size += rc;
/* start searching the next fd next time */ /* start searching the next fd next time */
fd++; fd++;
put_spu_context(ctx);
} }
return size; return size;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment