Message ID | 20200428114811.68436-1-mpe@ellerman.id.au (mailing list archive) |
---|---|
State | Changes Requested |
Headers | show |
Series | powerpc/spufs: Add rcu_read_lock() around fcheck() | expand |
Context | Check | Description |
---|---|---|
snowpatch_ozlabs/apply_patch | success | Successfully applied on branch powerpc/merge (54dc28ff5e0b3585224d49a31b53e030342ca5c3) |
snowpatch_ozlabs/build-ppc64le | success | Build succeeded |
snowpatch_ozlabs/build-ppc64be | success | Build succeeded |
snowpatch_ozlabs/build-ppc64e | success | Build succeeded |
snowpatch_ozlabs/build-pmac32 | warning | Upstream build failed, couldn't test patch |
snowpatch_ozlabs/checkpatch | warning | total: 0 errors, 1 warnings, 0 checks, 20 lines checked |
snowpatch_ozlabs/needsstable | success | Patch has no Fixes tags |
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).
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;
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.
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)
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(-)