Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Michael Ellerman <mpe@ellerman.id.au>
To:     Christoph Hellwig <hch@lst.de>
Cc:     linuxppc-dev@ozlabs.org, hch@lst.de, jk@ozlabs.org,
        viro@zeniv.linux.org.uk, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] powerpc/spufs: Add rcu_read_lock() around fcheck()
In-Reply-To: <20200428135245.GA2827@lst.de>
References: <20200428114811.68436-1-mpe@ellerman.id.au> <20200428135245.GA2827@lst.de>
Date:   Wed, 29 Apr 2020 21:42:39 +1000
Message-ID: <875zdifrgw.fsf@mpe.ellerman.id.au>
MIME-Version: 1.0
Content-Type: text/plain
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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;