linux-kernel - Re: [PATCH] powerpc/spufs: Add rcu_read

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <875zdifrgw.fsf@mpe.ellerman.id.au>
Date:   Wed, 29 Apr 2020 21:42:39 +1000
From:   Michael Ellerman <mpe@...erman.id.au>
To:     Christoph Hellwig <hch@....de>
Cc:     linuxppc-dev@...abs.org, hch@....de, jk@...abs.org,
        viro@...iv.linux.org.uk, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] powerpc/spufs: Add rcu_read_lock() around fcheck()

Christoph Hellwig <hch@....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;