Wrong offset for incremental buffers after non-pollable files

[mgjm]

When a buffer gets committed early during buffer selection in io_ring_buffer_select (for non-pollable files), the amount of committed data is based on the requested length, but the length reported to the userspace is the actual length read.

Steps to reproduce:

• Have a small file (e.g. 12 bytes: Hello World\n)
• Issue multiple reads at offset 0 with a requested length of 16 bytes
• The CQEs contain res=12 and flags=BUFFER | BUF_MORE but the next data will be placed at offset 16

This is only a problem if an explicit length is requested (length in request != 0). Otherwise the remaining length of buffer the will be requested and the buffer is fully committed anyways.

Solutions:

1. Introduce a new CQE flag: indicating that the committed length of the buffer was the requested length and not the actual length of the operation. (This does not work, because of the second bug)
2. Deny explicitly requested lengths for incremental buffers and force non-pollable files to always commit all of the remaining buffer (length in request == 0).
3. Allow explicitly requested lengths but commit the full remaining buffer anyways.
4. Find a way to tell the userspace the offset (or address) within the buffer

The first solution might still be problematic if the following can happen:
Is it possible that two non-pollable operations commit a chunk of the same buffer (committed during buffer selection) but complete in a different order (e.g. the first disk io was slower).
Then the userspace would see the CQE for the second chunk first and calculate the wrong offset in the buffer.
This would be a problem in itself even without the actual length being shorter.

Edit: The out-of-order completion of read operations can actually happen, see reproducers below.

[mgjm]

I have written reproducers for both bugs. Here is the output:

$ ./short_read < lorem.txt
 res | flags   | buffer | bid | buf_more | data
-----+---------+--------+-----+----------+-----
  16 | 0x00011 |      1 |   0 |        1 | "Lorem ipsum dolo"
  16 | 0x00011 |      1 |   0 |        1 | "Lorem ipsum dolo"
Done.

$ ./short_read < short.txt
 res | flags   | buffer | bid | buf_more | data
-----+---------+--------+-----+----------+-----
  12 | 0x00011 |      1 |   0 |        1 | "Hello World\n"
  12 | 0x00011 |      1 |   0 |        1 | "\x00\x00\x00\x00Hello Wo"   <== BUG 1
Done.

$ ./out-of-order lorem.txt
 user_data | res | flags   | buffer | bid | buf_more | data
-----------+-----+---------+--------+-----+----------+-----
         1 |   16 | 0x00011 |      1 |   0 |        1 | "it amet, consete"
         2 |   12 | 0x00011 |      1 |   0 |        1 | "Hello World\n"
Done.

$ dd if=/dev/urandom of=random.bin bs=1M count=10240
$ ./out-of-order random.bin
 user_data | res | flags   | buffer | bid | buf_more | data
-----------+-----+---------+--------+-----+----------+-----
         2 |   12 | 0x00011 |      1 |   0 |        1 | "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"   <== BUG 2
         1 |   16 | 0x00011 |      1 |   0 |        1 | "\xae\xda\xb6\xa0Hello World\n"    <== BUG 2
Done.

Bug 1

The program tries to read 16 bytes from a 12 byte file. Both CQEs report 12 bytes read, but 16 bytes were consumed during buffer selection.

Bug 2

The program submits two read operations. First a read of 16 bytes from a random location of a file (first argument). Then a read from pipe that we already filled with Hello World\n. In the first case the file is already in the page cache and the operations complete in the order they were submitted. In the second case we use a 10 GB file and the requested data is not in the page cache. Therefore the second operation completes first and we see the not yet filled buffer of the first operation as the result buffer of the second operation. Once the first operation completes, we se the output of the second operation in the buffer of the first one.

Demo code

short_read.c

#include <errno.h>
#include <liburing.h>
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>

int main() {
  struct io_uring ring;

  const unsigned flags = IORING_SETUP_COOP_TASKRUN |
                         IORING_SETUP_SINGLE_ISSUER |
                         IORING_SETUP_DEFER_TASKRUN;

  if ((errno = -io_uring_queue_init(64, &ring, flags)) != 0) {
    perror("io_uring_queue_init");
    return EXIT_FAILURE;
  }

  struct io_uring_buf_ring *br;
  br = io_uring_setup_buf_ring(&ring, 2, 42, IOU_PBUF_RING_INC, &errno);
  if (!br) {
    errno = -errno;
    perror("io_uring_setup_buf_ring");
    return EXIT_FAILURE;
  }

  void *bufs[2] = {};
  int offsets[2] = {};
  for (int bid = 0; bid < 2; bid++) {
    const size_t size = 64;
    void *buf = calloc(size, 1);
    if (!buf) {
      perror("calloc");
      return EXIT_FAILURE;
    }

    bufs[bid] = buf;

    io_uring_buf_ring_add(br, buf, size, bid, 1, 0);
    io_uring_buf_ring_advance(br, 1);
  }

  int fd = STDIN_FILENO;

  printf(" res | flags   | buffer | bid | buf_more | data\n");
  printf("-----+---------+--------+-----+----------+-----\n");

  for (int i = 0; i < 2; i++) {
    struct io_uring_sqe *sqe = io_uring_get_sqe(&ring);
    io_uring_prep_read(sqe, fd, NULL, 16, 0);
    sqe->flags |= IOSQE_BUFFER_SELECT;
    sqe->buf_group = 42;
  }

  if ((errno = -io_uring_submit(&ring)) != -2) {
    perror("io_uring_submit");
    return EXIT_FAILURE;
  }

  for (int i = 0; i < 2; i++) {
    struct io_uring_cqe *cqe;
    if ((errno = -io_uring_wait_cqe(&ring, &cqe)) != 0) {
      perror("io_uring_wait_cqe");
      return EXIT_FAILURE;
    }

    int bid = cqe->flags >> IORING_CQE_BUFFER_SHIFT;
    printf("%4d | 0x%05x | %6d | %3d | %8d |", cqe->res, cqe->flags,
           (cqe->flags & IORING_CQE_F_BUFFER) != 0, bid,
           (cqe->flags & IORING_CQE_F_BUF_MORE) != 0);

    if (cqe->res < 0) {
      printf("\n");
      errno = -cqe->res;
      perror("cqe result");
      return EXIT_FAILURE;
    }

    int n = cqe->res;

    io_uring_cqe_seen(&ring, cqe);

    printf(" \"");
    for (int i = 0; i < n; i++) {
      char c = ((char *)bufs[bid])[offsets[bid]++];
      if (c == '"' || c == '\\') {
        printf("\\%c", c);
      } else if (c >= 0x20 && c <= 0x7E) {
        printf("%c", c);
      } else if (c == '\n') {
        printf("\\n");
      } else {
        printf("\\x%02x", c);
      }
    }

    printf("\"\n");
  }

  printf("Done.\n");
  return EXIT_SUCCESS;
}

out-of-order.c

#include <errno.h>
#include <fcntl.h>
#include <liburing.h>
#include <liburing/io_uring.h>
#include <stdio.h>
#include <stdlib.h>
#include <sys/random.h>
#include <unistd.h>

int main(int argc, char *argv[]) {
  if (argc != 2) {
    printf("Error: missing random file path\n");
    printf("usage: %s RANDOM_FILE\n", argv[0]);
    return EXIT_FAILURE;
  }

  struct io_uring ring;

  const unsigned flags = IORING_SETUP_COOP_TASKRUN |
                         IORING_SETUP_SINGLE_ISSUER |
                         IORING_SETUP_DEFER_TASKRUN;

  if ((errno = -io_uring_queue_init(64, &ring, flags)) != 0) {
    perror("io_uring_queue_init");
    return EXIT_FAILURE;
  }

  struct io_uring_buf_ring *br;
  br = io_uring_setup_buf_ring(&ring, 2, 42, IOU_PBUF_RING_INC, &errno);
  if (!br) {
    errno = -errno;
    perror("io_uring_setup_buf_ring");
    return EXIT_FAILURE;
  }

  void *bufs[2] = {};
  int offsets[2] = {};
  for (int bid = 0; bid < 2; bid++) {
    const size_t size = 64;
    void *buf = calloc(size, 1);
    if (!buf) {
      perror("calloc");
      return EXIT_FAILURE;
    }

    bufs[bid] = buf;

    io_uring_buf_ring_add(br, buf, size, bid, 1, 0);
    io_uring_buf_ring_advance(br, 1);
  }

  printf(" user_data | res | flags   | buffer | bid | buf_more | data\n");
  printf("-----------+-----+---------+--------+-----+----------+-----\n");

  struct io_uring_sqe *sqe;

  int pipes[2];
  if (pipe2(pipes, O_CLOEXEC) != 0) {
    perror("open");
    return EXIT_FAILURE;
  }

  if (write(pipes[1], "Hello World\n", 12) != 12) {
    perror("write");
    return EXIT_FAILURE;
  }

  close(pipes[1]);

  int fd = open(argv[1], O_RDONLY | O_CLOEXEC);
  if (fd < 0) {
    perror("open");
    return EXIT_FAILURE;
  }

  off_t size = lseek(fd, 0, SEEK_END);
  if (size < 0) {
    perror("lseek");
    return EXIT_FAILURE;
  }

  __u64 offset;
  if (getrandom(&offset, sizeof(offset), 0) != sizeof(offset)) {
    perror("getrandom");
    return EXIT_FAILURE;
  }
  offset %= size;

  sqe = io_uring_get_sqe(&ring);
  io_uring_prep_read(sqe, fd, NULL, 16, offset);
  sqe->flags |= IOSQE_BUFFER_SELECT;
  sqe->buf_group = 42;
  sqe->user_data = 1;

  sqe = io_uring_get_sqe(&ring);
  io_uring_prep_read(sqe, pipes[0], NULL, 16, 0);
  sqe->flags |= IOSQE_BUFFER_SELECT;
  sqe->buf_group = 42;
  sqe->user_data = 2;

  if ((errno = -io_uring_submit(&ring)) != -2) {
    perror("io_uring_submit");
    return EXIT_FAILURE;
  }

  for (int i = 0; i < 2; i++) {
    struct io_uring_cqe *cqe;
    if ((errno = -io_uring_wait_cqe(&ring, &cqe)) != 0) {
      perror("io_uring_wait_cqe");
      return EXIT_FAILURE;
    }

    int bid = cqe->flags >> IORING_CQE_BUFFER_SHIFT;
    printf("%10lld | %4d | 0x%05x | %6d | %3d | %8d |", cqe->user_data,
           cqe->res, cqe->flags, (cqe->flags & IORING_CQE_F_BUFFER) != 0, bid,
           (cqe->flags & IORING_CQE_F_BUF_MORE) != 0);

    if (cqe->res < 0) {
      printf("\n");
      errno = -cqe->res;
      perror("cqe result");
      return EXIT_FAILURE;
    }

    int n = cqe->res;

    io_uring_cqe_seen(&ring, cqe);

    printf(" \"");
    for (int i = 0; i < n; i++) {
      char c = ((char *)bufs[bid])[offsets[bid]++];
      if (c == '"' || c == '\\') {
        printf("\\%c", c);
      } else if (c >= 0x20 && c <= 0x7E) {
        printf("%c", c);
      } else if (c == '\n') {
        printf("\\n");
      } else {
        printf("\\x%02x", (unsigned char)c);
      }
    }

    printf("\"\n");
  }

  printf("Done.\n");
  return EXIT_SUCCESS;
}