[v4,0/4] btrfs: fix reading from userspace in btrfs_uring_encoded_read()

Message

Mark Harmstone Jan. 3, 2025, 3:02 p.m. UTC

Version 4 of mine and Jens' patches, to make sure that when our io_uring
function gets called a second time, it doesn't accidentally read
something from userspace that's gone out of scope or otherwise gotten
corrupted.

I sent a version 3 on December 17, but it looks like that got forgotten
about over Christmas (unsurprisingly). Version 4 fixes a problem that I
noticed, namely that we weren't taking a copy of the iovs, which also
necessitated creating a struct to store these things in. This does
simplify things by removing the need for the kmemdup, however.

I also have a patch for io_uring encoded writes ready to go, but it's
waiting on some of the stuff introduced here.

Jens Axboe (2):
  io_uring/cmd: rename struct uring_cache to io_uring_cmd_data
  io_uring/cmd: add per-op data to struct io_uring_cmd_data

Mark Harmstone (2):
  io_uring: add io_uring_cmd_get_async_data helper
  btrfs: don't read from userspace twice in btrfs_uring_encoded_read()

 fs/btrfs/ioctl.c             | 125 +++++++++++++++++++----------------
 include/linux/io_uring/cmd.h |  10 +++
 io_uring/io_uring.c          |   2 +-
 io_uring/opdef.c             |   3 +-
 io_uring/uring_cmd.c         |  23 +++++--
 io_uring/uring_cmd.h         |   4 --
 6 files changed, 97 insertions(+), 70 deletions(-)

Comments

Jens Axboe Jan. 3, 2025, 5:55 p.m. UTC | #1

On 1/3/25 8:02 AM, Mark Harmstone wrote:
> Version 4 of mine and Jens' patches, to make sure that when our io_uring
> function gets called a second time, it doesn't accidentally read
> something from userspace that's gone out of scope or otherwise gotten
> corrupted.
> 
> I sent a version 3 on December 17, but it looks like that got forgotten
> about over Christmas (unsurprisingly). Version 4 fixes a problem that I
> noticed, namely that we weren't taking a copy of the iovs, which also
> necessitated creating a struct to store these things in. This does
> simplify things by removing the need for the kmemdup, however.
> 
> I also have a patch for io_uring encoded writes ready to go, but it's
> waiting on some of the stuff introduced here.

Looks fine to me, and we really should get this into 6.13. The encoded
reads are somewhat broken without it, violating the usual expectations
on how persistent passed in data should be.

David Sterba Jan. 6, 2025, 11:58 a.m. UTC | #2

On Fri, Jan 03, 2025 at 10:55:42AM -0700, Jens Axboe wrote:
> On 1/3/25 8:02 AM, Mark Harmstone wrote:
> > Version 4 of mine and Jens' patches, to make sure that when our io_uring
> > function gets called a second time, it doesn't accidentally read
> > something from userspace that's gone out of scope or otherwise gotten
> > corrupted.
> > 
> > I sent a version 3 on December 17, but it looks like that got forgotten
> > about over Christmas (unsurprisingly). Version 4 fixes a problem that I
> > noticed, namely that we weren't taking a copy of the iovs, which also
> > necessitated creating a struct to store these things in. This does
> > simplify things by removing the need for the kmemdup, however.
> > 
> > I also have a patch for io_uring encoded writes ready to go, but it's
> > waiting on some of the stuff introduced here.
> 
> Looks fine to me, and we really should get this into 6.13. The encoded
> reads are somewhat broken without it, violating the usual expectations
> on how persistent passed in data should be.

Ok, I'll add the to the queue for the next RC.

David Sterba Jan. 6, 2025, 2:24 p.m. UTC | #3

On Fri, Jan 03, 2025 at 03:02:22PM +0000, Mark Harmstone wrote:
> Version 4 of mine and Jens' patches, to make sure that when our io_uring
> function gets called a second time, it doesn't accidentally read
> something from userspace that's gone out of scope or otherwise gotten
> corrupted.
> 
> I sent a version 3 on December 17, but it looks like that got forgotten
> about over Christmas (unsurprisingly).

V3 lacked the cover letter and it was not obvious if it's urgent fix,
new devlopemnent or a regular fix. Also it touched code outside of
btrfs, did not have any acks or word agreement that it would be ok to
take the fixes via btrfs tree.