Short I/O is indeed impossible in your implementation, but this is a consequence of the implementation which would also make changing the implementation of proc_rwmem() quite hard. Usual Unix API design is to return success if any bytes where moved, and only if no i/o happen, error could be returned. For proc_rwmem() this was not done, probably because the kernel function can (and does) return both error and decrement uio_resid. I would prefer that this detail not leaked into your functions.

Another thing which I do not quite like is the PROC_HOLD() added to proc_read/writemem. E.g., for kern_proc.c, you get a recursion on hold count. Generally, the caller must already own the hold count on the process it is operating on, otherwise the action can be aborted at any time due to the process exiting. The recursion seems harmless because PHOLD() is not blocking, but still I dislike it.

Overall, I think that the helpers are useful and should be added.

In D4245#89542, @kib wrote:

Short I/O is indeed impossible in your implementation, but this is a consequence of the implementation which would also make changing the implementation of proc_rwmem() quite hard. Usual Unix API design is to return success if any bytes where moved, and only if no i/o happen, error could be returned. For proc_rwmem() this was not done, probably because the kernel function can (and does) return both error and decrement uio_resid. I would prefer that this detail not leaked into your functions.

My argument was that any API which modifies a process' address space isn't really implementing an "I/O operation" at all, at least in the usual sense. IMHO, the only reason proc_rwmem() exposes the possibility of short I/O is because it uses uio(9), which is designed to be used for true I/O operations where this is indeed possible.

Would you be satisfied with an optional ssize_t * parameter to proc_readmem/writemem that allows the caller to determine the number of bytes read or written? That is, all current callers would pass NULL, but in the future they could be modified to check for short I/Os without modifying the KPI.

Another thing which I do not quite like is the PROC_HOLD() added to proc_read/writemem. E.g., for kern_proc.c, you get a recursion on hold count. Generally, the caller must already own the hold count on the process it is operating on, otherwise the action can be aborted at any time due to the process exiting. The recursion seems harmless because PHOLD() is not blocking, but still I dislike it.

Hm, I don't follow. proc_readmem/writemem don't modify the hold count. They do the same thing as proc_rwmem() and require that the caller hold the process.

In D4245#89661, @markj wrote:

In D4245#89542, @kib wrote:

Short I/O is indeed impossible in your implementation, but this is a consequence of the implementation which would also make changing the implementation of proc_rwmem() quite hard. Usual Unix API design is to return success if any bytes where moved, and only if no i/o happen, error could be returned. For proc_rwmem() this was not done, probably because the kernel function can (and does) return both error and decrement uio_resid. I would prefer that this detail not leaked into your functions.

My argument was that any API which modifies a process' address space isn't really implementing an "I/O operation" at all, at least in the usual sense. IMHO, the only reason proc_rwmem() exposes the possibility of short I/O is because it uses uio(9), which is designed to be used for true I/O operations where this is indeed possible.

Would you be satisfied with an optional ssize_t * parameter to proc_readmem/writemem that allows the caller to determine the number of bytes read or written? That is, all current callers would pass NULL, but in the future they could be modified to check for short I/Os without modifying the KPI.

No, this is probably too burdensome for the consumers of the KPI.

First, note that proc_rwmem() current implementation is somewhat unnatural. Assume, just for the discussion, that somebody asked to read two aligned consequtive pages of the process memory. Also, suppose that the first page is mapped, while second page is not. Then, depending on how the request is split in the uio vector, you either get one page read, and uio_resid advanced by a page, or not. In any case, you get EFAULT.

IMHO, the right interface there is to return short read and no error, if both io and error occured. Changing proc_rwmem() is outside the scope of your patch. But, for the new functions, IMO it is reasonable to provide the reasonable interface. In other words, I would remove the KASSERT(), and return the count of bytes moved if any, instead of a concurrent error.

Another thing which I do not quite like is the PROC_HOLD() added to proc_read/writemem. E.g., for kern_proc.c, you get a recursion on hold count. Generally, the caller must already own the hold count on the process it is operating on, otherwise the action can be aborted at any time due to the process exiting. The recursion seems harmless because PHOLD() is not blocking, but still I dislike it.

Hm, I don't follow. proc_readmem/writemem don't modify the hold count. They do the same thing as proc_rwmem() and require that the caller hold the process.

My bad, I misread the patch, mixing different fragments without re-checking my memory. In fact, the complaint is probably still valid, but directed at the fasttrap_isa.c functions (which is unrelated, but highlighted by the patch).

In D4245#89699, @kib wrote:

In D4245#89661, @markj wrote:

In D4245#89542, @kib wrote:

Short I/O is indeed impossible in your implementation, but this is a consequence of the implementation which would also make changing the implementation of proc_rwmem() quite hard. Usual Unix API design is to return success if any bytes where moved, and only if no i/o happen, error could be returned. For proc_rwmem() this was not done, probably because the kernel function can (and does) return both error and decrement uio_resid. I would prefer that this detail not leaked into your functions.

My argument was that any API which modifies a process' address space isn't really implementing an "I/O operation" at all, at least in the usual sense. IMHO, the only reason proc_rwmem() exposes the possibility of short I/O is because it uses uio(9), which is designed to be used for true I/O operations where this is indeed possible.

Would you be satisfied with an optional ssize_t * parameter to proc_readmem/writemem that allows the caller to determine the number of bytes read or written? That is, all current callers would pass NULL, but in the future they could be modified to check for short I/Os without modifying the KPI.

No, this is probably too burdensome for the consumers of the KPI.

First, note that proc_rwmem() current implementation is somewhat unnatural. Assume, just for the discussion, that somebody asked to read two aligned consequtive pages of the process memory. Also, suppose that the first page is mapped, while second page is not. Then, depending on how the request is split in the uio vector, you either get one page read, and uio_resid advanced by a page, or not. In any case, you get EFAULT.

IMHO, the right interface there is to return short read and no error, if both io and error occured. Changing proc_rwmem() is outside the scope of your patch. But, for the new functions, IMO it is reasonable to provide the reasonable interface. In other words, I would remove the KASSERT(), and return the count of bytes moved if any, instead of a concurrent error.

Ok, that makes sense to me.

Another thing which I do not quite like is the PROC_HOLD() added to proc_read/writemem. E.g., for kern_proc.c, you get a recursion on hold count. Generally, the caller must already own the hold count on the process it is operating on, otherwise the action can be aborted at any time due to the process exiting. The recursion seems harmless because PHOLD() is not blocking, but still I dislike it.

Hm, I don't follow. proc_readmem/writemem don't modify the hold count. They do the same thing as proc_rwmem() and require that the caller hold the process.

My bad, I misread the patch, mixing different fragments without re-checking my memory. In fact, the complaint is probably still valid, but directed at the fasttrap_isa.c functions (which is unrelated, but highlighted by the patch).

Oh, yeah, the current usage of the hold count (as well as proc locking) in fasttrap isn't correct. I have some patches that attempt to fix it, but that'll be a separate change.