Page MenuHomeFreeBSD

graphics/wayland: disable posix_fallocate on FreeBSD < 13
ClosedPublic

Authored by jbeich on Feb 27 2020, 6:15 PM.

Details

Summary

Regressed by D23696.

On FreeBSD < 13 neither memfd_create exists nor posix_fallocate works with file descriptors returned by shm_open. As SHM_ANON code wasn't upstreamed and is not used on FreeBSD 13 just disable posix_fallocate without version checks.

Test Plan
  • Builds fine FreeBSD 11.3/12.0/12.1/13.0
  • Works fine with x11-wm/sway on FreeBSD 13.0 (nop there)
  • Confirmed to fix regression on FreeBSD 12.*

Diff Detail

Repository
rP FreeBSD ports repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

jbeich created this revision.Feb 27 2020, 6:15 PM

I have tested this on my 12-STABLE kabylake system under sway. Previously xfce4-terminal would fault on startup, with this patch xfce4-terminal now works as expected. Thanks!

jbeich updated this revision to Diff 68900.Feb 27 2020, 6:59 PM
jbeich edited the summary of this revision. (Show Details)
jbeich edited the test plan for this revision. (Show Details)
  • Slightly adjust commit message and code comment
zeising accepted this revision.Feb 27 2020, 8:01 PM
zeising added subscribers: kevans, zeising.

Approved

Can you please explain this to me a bit more, please?
This is a bug (or missing feature) in FreeBSD 12 that posix_fallocate() doesn't work with shm_open()? Should we poke @kevans about merging that change to FreeBSD 12 if possible?
I believe that the code will call os_resize_anonyous_file(), which call posix_fallocate(), even with the call to memfd_create(), if this works on FreeBSD 13, perhaps the patch should be made conditional on that version anyway? Or am I misreading the wayland code?
Apologies for all the question, I'm just a bit confused trying to understand it all.
Thank you for debugging this!

This revision is now accepted and ready to land.Feb 27 2020, 8:01 PM

I also wonder if we have the same pattern of shm_open() posix_fallocate() in other places (mesa comes to mind). I haven't looked though.

jbeich added a comment.EditedFeb 27 2020, 8:18 PM

This is a bug (or missing feature) in FreeBSD 12 that posix_fallocate() doesn't work with shm_open()?

Missing feature but using posix_fallocate with shm_open is unusual compared to memfd_create.

Should we poke @kevans about merging that change to FreeBSD 12 if possible?

Existing -RELEASE that haven't reached EOL still need to be supported in ports/. Backporting memfd_create would be more useful but maybe more risky.

I believe that the code will call os_resize_anonyous_file(), which call posix_fallocate(), even with the call to memfd_create(), if this works on FreeBSD 13, perhaps the patch should be made conditional on that version anyway? Or am I misreading the wayland code?

posix_fallocate with shm_open was added as a prerequisite for memfd_create support (as explained in rS356512).

This is a bug (or missing feature) in FreeBSD 12 that posix_fallocate() doesn't work with shm_open()?

Missing feature but using posix_fallocate with shm_open is unusual compared to memfd_create.

Should we poke @kevans about merging that change to FreeBSD 12 if possible?

Existing -RELEASE that haven't reached EOL still need to be supported in ports/. Backporting memfd_create would be more useful but maybe more risky.

I know that, but perhaps having this fixed, even if memfd_create() is not possible to merge sets us up better once the current release of 12 goes EOL. Of course having memfd_create() in 12 would be best. I'll ask @kevans about it.

I believe that the code will call os_resize_anonyous_file(), which call posix_fallocate(), even with the call to memfd_create(), if this works on FreeBSD 13, perhaps the patch should be made conditional on that version anyway? Or am I misreading the wayland code?

FreeBSD 13 uses memfd_create(MFD_ALLOW_SEALING) instead of shm_open(SHM_ANON). See also:
https://gitlab.freedesktop.org/wayland/wayland/-/merge_requests/4
https://gitlab.freedesktop.org/wayland/wayland/-/merge_requests/29

So, I misread the code the first time around. In the original code (before this patch) it looked like it would call memfd_create() and then posix_fallocate(), and I thought that with your patch it would not call posix_fallocate() at all. I see now that this is not the case, you only do #undef HAVE_POSIX_FALLOCATE when we don't have memfd_create() and fall back to shm_anon() instead.
Sorry for the noise and thanks for the explanation.

We could maybe MFC posix_fallocate support for shmfd, but it'd be a little contorted because we can't change struct fileops like we did in head. memfd_create could be MFC'd, but file sealing cannot and I'm not sure we want it if file sealing cannot be supported.

If shm_open(SHM_ANON) code is ever upstreamed then #undef HAVE_POSIX_FALLOCATE should be replaced with ENODEV check in addition to EINVAL and EOPNOTSUPP. However, doing so is a minor pessimization due to extra syscall that always fails.

We could maybe MFC posix_fallocate support for shmfd, but it'd be a little contorted because we can't change struct fileops like we did in head. memfd_create could be MFC'd, but file sealing cannot and I'm not sure we want it if file sealing cannot be supported.

If it is possible, it could be helpful to merge posix_fallocate() support for shmfd, but if it is not possible, we have to live with our workaround(s) for the lifetime of FreeBSD 12. memfd_create without file sealing is probably not a good idea to merge, then I believe it's better we fall back to shm_open(SHM_ANON) togeter with posix_fallocate() or ftruncate() or similar.