Allow setting O_NONBLOCK on shm file descriptors

[val_packett.cool]

Summary

Software likes to do interesting things with file descriptors:

if (fcntl(fd, F_SETFL, fd_flags | O_NONBLOCK) == -1) {
	wlr_log_errno(WLR_ERROR, "failed to set FD flags");
	gamma_control_send_failed(gamma_control);
	goto error_fd;
}

­— https://github.com/swaywm/wlroots/blob/421283935b42ed889043cd06e590a61d8317e88c/types/wlr_gamma_control_v1.c#L70-L81

I'm not sure why you'd be worried about blocking when receiving shared memory — defense against pipes/sockets being maliciously sent, most likely — but this works just fine with unlinked temporary files and does not work with shm_open(SHM_ANON). Let's fix that.

(This patch will ignore nonblock and async on shm, mqueue, procdesc and sem — I hope that's not a problem, but I can create a special ioctl handler just for shm if refusing these flags is wanted for these other fd types.)

[markj]

What are the semantics on Linux? I can imagine O_NONBLOCK meaning "return EAGAIN if the page is swapped out," in which case this patch is not sufficient.

I don't think it's a good idea to enable these ioctls on all of these descriptor types. mqueues support setting O_NONBLOCK at open() time, for instance, and it might be useful to define some semantic for non-blocking process descriptors. In returning an error we reserve these flags for future extensions; once we simply start returning 0 instead, there's less guarantee that future extensions to actually do something with those flags won't break existing software.

[val_packett.cool]

In D19330#414021, @markj wrote:

What are the semantics on Linux? I can imagine O_NONBLOCK meaning "return EAGAIN if the page is swapped out," in which case this patch is not sufficient.

I haven't looked at Linux, but [[ https://github.com/freebsd/freebsd/blob/724199fd4be1d5274c9fabdfb9bb84778cf8de41/sys/kern/vfs_vnops.c#L1485-L1487 | our semantics for regular files is return (0) ]], so it would make sense to do the same thing between shm and an unlinked file, right?

I don't think it's a good idea to enable these ioctls on all of these descriptor types. mqueues support setting O_NONBLOCK at open() time, for instance, and it might be useful to define some semantic for non-blocking process descriptors. In returning an error we reserve these flags for future extensions; once we simply start returning 0 instead, there's less guarantee that future extensions to actually do something with those flags won't break existing software.

Oh, okay. I'll change it to shm only.

[markj]

In D19330#414129, @greg_unrelenting.technology wrote:

In D19330#414021, @markj wrote:

What are the semantics on Linux? I can imagine O_NONBLOCK meaning "return EAGAIN if the page is swapped out," in which case this patch is not sufficient.

I haven't looked at Linux, but [[ https://github.com/freebsd/freebsd/blob/724199fd4be1d5274c9fabdfb9bb84778cf8de41/sys/kern/vfs_vnops.c#L1485-L1487 | our semantics for regular files is return (0) ]], so it would make sense to do the same thing between shm and an unlinked file, right?

I don't really follow. For regular files, O_NONBLOCK means "don't block on open." vn_read() will translate O_NONBLOCK to IO_NDELAY, but most filesystems don't do anything with that. In the snippet you referenced, the code has a comment, "refuse to block when reading," but with this patch shm_read() will block anyway because uiomove_object_page() provides no mechanism for the caller to ask for non-blocking semantics. So either Linux ignores the O_NONBLOCK flag when reading from a shm object and the referenced comment is bogus, or Linux actually honours the flag, in which case this patch isn't sufficient. In the former case, the code should simply be eliminated rather than changing the kernel.

[val_packett.cool]

In D19330#414136, @markj wrote:

I don't really follow. For regular files, O_NONBLOCK means "don't block on open." vn_read() will translate O_NONBLOCK to IO_NDELAY, but most filesystems don't do anything with that. In the snippet you referenced, the code has a comment, "refuse to block when reading," but with this patch shm_read() will block anyway because uiomove_object_page() provides no mechanism for the caller to ask for non-blocking semantics. So either Linux ignores the O_NONBLOCK flag when reading from a shm object and the referenced comment is bogus, or Linux actually honours the flag, in which case this patch isn't sufficient. In the former case, the code should simply be eliminated rather than changing the kernel.

Yes, for regular files, O_NONBLOCK means "don't block on open" — therefore, it doesn't mean anything when the file has already been opened — so it is silently ignored in ioctl/fcntl in vn_ioctl. I'm *only* talking about fcntl, not opening.

As I understand it, the referenced comment isn't bogus, it's defensive code: imagine if a client application passes a pipe or socket instead of the expected file/shm. It would have the ability to make the display server hang forever. The server sets nonblock to prevent that from happening. It's not supposed to do anything in the good case (file/shm was actually sent).

[markj]

In D19330#414142, @greg_unrelenting.technology wrote:

In D19330#414136, @markj wrote:

I don't really follow. For regular files, O_NONBLOCK means "don't block on open." vn_read() will translate O_NONBLOCK to IO_NDELAY, but most filesystems don't do anything with that. In the snippet you referenced, the code has a comment, "refuse to block when reading," but with this patch shm_read() will block anyway because uiomove_object_page() provides no mechanism for the caller to ask for non-blocking semantics. So either Linux ignores the O_NONBLOCK flag when reading from a shm object and the referenced comment is bogus, or Linux actually honours the flag, in which case this patch isn't sufficient. In the former case, the code should simply be eliminated rather than changing the kernel.

Yes, for regular files, O_NONBLOCK means "don't block on open" — therefore, it doesn't mean anything when the file has already been opened — so it is silently ignored in ioctl/fcntl in vn_ioctl. I'm *only* talking about fcntl, not opening.

As I understand it, the referenced comment isn't bogus, it's defensive code: imagine if a client application passes a pipe or socket instead of the expected file/shm. It would have the ability to make the display server hang forever. The server sets nonblock to prevent that from happening. It's not supposed to do anything in the good case (file/shm was actually sent).

I see now, thanks. I still think that the change should apply only to shm descriptors, and it would be good to confirm that Linux does in fact ignore O_NONBLOCK on shm descriptors. (I'm not sure that you can even read() from them on Linux?)

[val_packett.cool]

In D19330#414453, @markj wrote:

I still think that the change should apply only to shm descriptors

Yeah, I just haven't had time to upload the updated patch, should be good now.

and it would be good to confirm that Linux does in fact ignore O_NONBLOCK on shm descriptors. (I'm not sure that you can even read() from them on Linux?)

Quick test:

int main() {
	int shmfd = shm_open("memes", O_CREAT, 0600);
	printf("fcntl(shmfd, F_SETFL, O_NONBLOCK): %d\n", fcntl(shmfd, F_SETFL, O_NONBLOCK));
	int memfd = syscall(SYS_memfd_create, "lol", MFD_CLOEXEC);
	printf("fcntl(memfd, F_SETFL, O_NONBLOCK): %d\n", fcntl(memfd, F_SETFL, O_NONBLOCK));
}

fcntl(shmfd, F_SETFL, O_NONBLOCK): 0
fcntl(memfd, F_SETFL, O_NONBLOCK): 0

shm on Linux is straight up just files on a tmpfs mounted at /dev/shm (after running the above test, /dev/shm/memes remains), so unsurprisingly the behavior matches regular files. The more interesting memfd, which is the equivalent of our SHM_ANON, also takes the flag fine.