shm: Handle swap pager allocation failures

[markj]

Summary

shm_alloc() can fail if swap reservation fails (i.e., vm.overcommit is
non-zero) or racct is imposing some limits on swap usage.

PR: 282994

[kib]

sys/kern/uipc_shm.c

965

IMO it is worth to put the check outside of if (largepage), to allow phys_pager_allocate() to fail in future.

In principle, phys pager should be the subject to some limit.

1252–1253

goto out

1259

goto out

1366

add the out label there

[olce]

sys/kern/uipc_shm.c

965

Isn't the test the opposite of the desired one?

[markj]

Apply reviewer suggestions, fix a couple of bugs.

sys/kern/uipc_shm.c

965

Yes, and I need to avoid leaking memory there.

[olce]

sys/kern/uipc_shm.c

1365–1366

Unfortunately, I think you just can't move freeing path, keeping a single instance of it, without introducing a flag somehow.

I guess you did it because such a free() was indeed missing as the else counterpart (which doesn't exist) of the if (error = 0) { in the if (flags & O_CREAT) { branch above, but then the current version is also going to free it in the non-error case (the same if (error = 0) {) which will cause some use after free as path (the pointer) is copied into the object put in the SHM dictionary.

Moving instead the free() inside the following if (error != 0) { block, but before the out label, doesn't work either, as path must be freed in the else block matching the if (shmfd == NULL) {, i.e., the branch where the SHM already exists.

If you choose to restore the original free() there, I'd suggest also moving it to the top, before locking the rangelock. An (unsophisticated) alternative could be to unconditionally call free(path, M_SHMFD) in the out: block, as if path is not filled it is set to NULL, as well as before the successful exit (return (0);). YMMV.

1367–1368

While here, I would suggest to completely move the out: sub-block at the end of the function, just replacing it here with one more goto out;. I think this in general makes the code more readable, as it moves the error path away from the rest (and also allows to de-indent it; here, each line is short so that doesn't matter much, but in other cases it can).

[markj]

Fix handling of the path buffer

[kib]

sys/kern/uipc_shm.c

966–967

Then why not allocate the object before shmfd?

1292

IMO it would be (much) simpler if path was freed on return. NULL it at the start of the function, and when consumed.

[olce]

I tend to agree with kib@ for both of its inline comments. The second comment is the second alternative I listed. I'm fine either way.

[markj]

Simplify error handling some more.

[markj]

One more fixup.