Bug 476887

It'll take me a little time to work out how the vector reads ans writes work.

Thanks for looking into this, @pjfloyd.  Let me know if there's anything I can do to help.

Looking at the code, I did a quick and dirty job for aio_read and aio_write which doesn't correctly check everything. I've mostly copied the Darwin implementation. I should manage to do the vector versions this week.

The aio code needs more work, but I'm getting close to having something working for writev and readv.

paulf> LD_PRELOAD=obj/usr/home/paulf/build/src/amd64.amd64/tmp/lib/libc.so.7 /home/paulf/scratch/valgrind/vg-in-place -q ./obj/usr/home/paulf/build/src/amd64.amd64/tests/sys/aio/aio_test vectored_file_poll
aio_test: WARNING: Running test cases outside of kyua(1) is unsupported
aio_test: WARNING: No isolation nor timeout control is being applied; you may get unexpected failures; see atf-test-case(4)
Unsafe AIO is enabled
passed

I've made quite a few improvements, but there are a few things that I'm not sure about.

First, a quick Valgrind syscall digression. For synchronous syscalls that read and write memory, Valgrind will track pre- and post-syscall reads and pre-syscall writes. The tracking allows it to flag reads of uninitialized memory and to mark written memory as initialized. async reads are more complicated since we can't use the post-syscall, we need something else to know when to mark the memory as initialized.

I started looking at the Darwin implementation for aio_read but it looks fairly seriously wrong to me. (I'll get to the vector versions in a bit).

The Darwin implementation does the following
- in pre-syscall aio_read it registers the read
- in pre-syscall aio_return it deregisters the read and if the deregistration succeeds sets a global flag to true
- in post-syscall aio_return it checks the global flag and marks the memory as initialized

I see two problems with that
1. the global flag looks unsafe if two or more aio_reads are in flight
2. I don't think that the user is obliged to call aio_return. To me that just seems like an async error code - lots of software doesn't bother to check error codes.

From what I've seen there are 3 ways that users can check whether an aio_read has completed or not
1. poll aio_error
2. wait for the read to finish using aio_suspend or aio_waitcomplete (I'm not sure what the difference is between those two)
3. use the sigevent

Just checked my copy of Design and Implementation. aio_waitcomplete combines suspend and return.
Can you confirm that after any of the 3 above the read will have completed and the memory should be marked as initialized?

I haven't looked at the sigevent. That might be a significant amount of work to support in Valgrind.

Back to the vector versions. I suppose that all of the above ways to indicate completion also apply and that they are atomic (that is they only apply when all of the vector of memory blocks have been read to).

Thanks for the quick work!

> async reads are more complicated since we can't use the post-syscall, we need something else to know when to mark the memory as initialized.

A conforming implementation of async_read could be fully synchronous.  In fact, that's what DragonflyBSD does.  Could valgrind cheat a bit and assume that all async_read calls complete as soon as the syscall is done?  It wouldn't catch a "user read memory before aio_error indicated completion" bug, but it would still catch the usual errors like out-of-bounds reads.

> 1. the global flag looks unsafe if two or more aio_reads are in flight

Definitely a problem.  POSIX AIO isn't very useful if you can't have multiple reads in flight.

> 2. I don't think that the user is obliged to call aio_return. To me that just seems like an async error code - lots of software doesn't bother to check error codes.

Actually, the user _is_ obliged to call aio_return (except when using aio_waitcomplete, as you observed).  That's because aio_return also releases some resources in-kernel.  If the user never calls it, their program will probably hang eventually.  There is a proposed code change to eliminate the requirement when using kqueue, but it isn't merged yet (https://reviews.freebsd.org/D33144).

> Can you confirm that after any of the 3 above the read will have completed and the memory should be marked as initialized?

> Back to the vector versions. I suppose that all of the above ways to indicate completion also apply and that they are atomic (that is they only apply when all of the vector of memory blocks have been read to).

Yep, they're just as atomic as readv and writev.

Yes, if the user calls aio_return too.  Without that, you can't know whether the read was successful.

Synchonising with aio_return shouldn't be too difficult. I'll do something along the same lines as Darwin but without the global 'was_a_successful_aio_read' variable and I'll put everything in the aio_return pre-syscall so there's no need to comminicate between aio_return pre- and post-syscall.

Then I just need a testcase and it should be ready to go.

I think I'll open a freebsd buzilla item for the manpages.
The Linux one isn't fantastic but it's a lot clearer from the user's perspective.
https://www.man7.org/linux/man-pages/man3/aio_read.3.html
FreeBSD goes into a lot of implementation details
https://man.freebsd.org/cgi/man.cgi?query=aio_read&apropos=0&sektion=2&manpath=FreeBSD+13.2-RELEASE+and+Ports&arch=default&format=html
plus this is wrong
"aio_read()  will  read  iocb->aio_nbytes  from  the buffer pointed to by iocb->aio_buf"
the read is from the fd into the buffer

Last thing to do will be to take a look at the Valgrind doc.

I think that 4.2.2 needs to say something about async reads
https://valgrind.org/docs/manual/mc-manual.html#mc-manual.uninitvals

> plus this is wrong
> "aio_read()  will  read  iocb->aio_nbytes  from  the buffer pointed to by

Oops.  Thanks for spotting that.  I've just opened a review to fix it.

commit a15b387e630226b13944126fd3d75e79e551fcb2 (HEAD -> master, origin/master, origin/HEAD)
Author: Paul Floyd <pjfloyd@wanadoo.fr>
Date:   Thu Nov 16 21:33:33 2023 +0100

    Bug 476887 - WARNING: unhandled amd64-freebsd syscall: 578
    
    Adds syscall wrappers for aio_readv and aio_writev
    Also rewrote the wrappers for aio_read, aio_write and aio_return
    as they weren't correctly checking the async memory.
    
    The code is similar th that of Darwin with one exception.
    Darwin uses a global variable to communicate between the
    pre- and post- aio_return wrappers. I don't think that is
    safe when there are multiple aio reads in flight. Instead
    I put everything in the pre- aio_return wrapper.

Thanks!  I look forward to trying it out in the next release.  BTW, aio_writev and aio_readv can also be invoked from lio_listio.  That syscall is a real can of worms.  However, I don't know of any programs that actually do that.