do you have a patch so we can see how bad this looks in practice ? If it is just a simple ifdef, I don't think there would be much opposition to this idea.

No patch at the moment, sorry.

I was thinking more about a runtime fallback if memfd_create() returns -1 and errno is set to ENOSYS rather than a ifdef.

reintroducing that code would reintroduce the bug the commit was addressing, right  ?  if we care about supporting 3.16 kernels, I think a better fall back strategy would probably be to use shm_open instead (there's a patch for that on bug 761095).  But, personally, I'm not convinced fall back makes sense !  

wayland is a chance to get away from technical baggage, so it seems a little strange to me to try to work on platforms that don't offer features that have been around for years.

Emanuele if you don't have an #ifdef won't it fail to build because of the #include <linux/memfd.h> ?

(In reply to Ray Strode [halfline] from comment #4)
> Emanuele if you don't have an #ifdef won't it fail to build because of the
> #include <linux/memfd.h> ?

#ifdef protects you from failing to compile with an old libc, a runtime fallback on ENOSYS protects you from failing to run with an old kernel. They complement each other. Whether you need neither, one or both depends on how long ago the syscall was added to libc and the kernel.

(In reply to Ray Strode [halfline] from comment #3)
> reintroducing that code would reintroduce the bug the commit was addressing,
> right  ?

From Bug #761095:

"If it fills then desktop will start crashing with SIGBUS errors."

This is obviously not ideal, but seems better than crashing immediately? :-)

(In reply to Ray Strode [halfline] from comment #3)
> wayland is a chance to get away from technical baggage, so it seems a little
> strange to me to try to work on platforms that don't offer features that
> have been around for years.

memfds were new in Linux 3.17 (~ 18 months old) which is really fairly new. Distributions typically try to support running release N on the kernel from release N-1, because otherwise upgrading is a pain (if there's a regression in the new kernel on your hardware, it's desirable to be able to boot with the old one). In particular, in Debian 9 we're hoping to have GNOME defaulting to Wayland, but if that crashes horribly under the 3.16 kernel from Debian 8, it's unhelpful.

The other factor that makes support for older kernels desirable is that outside the x86 + nVidia/AMD/Intel desktop hardware bubble, it often isn't practical to use a new kernel, because hardware frequently doesn't work with a mainline kernel, and hardware-vendor kernels are usually something rather old patched to within an inch of its life. This is not a desirable situation, but unfortunately it's the situation the industry is in right now :-(

(In reply to Ray Strode [halfline] from comment #3)
> I think a better fall
> back strategy would probably be to use shm_open instead (there's a patch for
> that on bug 761095)

That would indeed be less prone to running out of space due to random apps writing out temp files, although I believe shm_open() is implemented in terms of writing to /dev/shm, which is just another tmpfs with its own separate quota.

okay i'm going to withdraw my objections, but we should add a big comment saying that the code is slated for removal.

Created attachment 327740 [details] [review]
wayland: fall back to shm_open if memfd unavailable

Debian stable currently ships with a 3.16 kernel, so
it doesn't have memfd available.

This commit adds shm_open fall back code for that case
(for now).

^ that's untested, can someone with debian stable try it ?

I'll give it a shot shortly, thanks!

we may have to check for EPERM in addition to ENOSYS.  see this, oddly coincidental, irc log from this morning:

[07:15:26] <memeka> i fail to run GTK3 apps under wayland - it crashes with "creating shared memory file failed: Operation not permitted"
[07:16:37] <memeka> it crashes in create_shm_pool in gdkdisplay-wayland.c
[07:26:40] <halfline> memeka: what's the output of 'uname -a' ?
[07:27:11] <memeka> kernel 3.10.96, armv7l
[07:27:37] <halfline> memeka: and the version of gtk3 ?
[07:27:52] <memeka> i'm running debian, only created a root user
[07:27:55] <memeka> gtk3.20
[07:28:19] <pq> halfline, shouldn't that cause something like ENOSYS rather than EPERM?
[07:28:46] <halfline> memeka: sounds like you're hitting https://bugzilla.gnome.org/show_bug.cgi?id=766341
[07:28:49] <halfline> pq: you'd think
[07:28:55] <pq> :-p
[07:29:14] <halfline> pq: but apparently not ...his kernel is too old for memfd_create
[07:29:18] <halfline> and he's getting EPERM
[07:29:41] <pq> right, that confused me
[07:32:18] <halfline> pq: i wonder if __NR_memfd_create maps to a different syscall
[07:32:35] <halfline> pq: (say it's accidentally getting the x86 number or something)

(In reply to Ray Strode [halfline] from comment #11)
> we may have to check for EPERM in addition to ENOSYS.  see this, oddly
> coincidental, irc log from this morning

I'd be tempted to ignore errno and just assume that any memfd_create() failure should result in fallback... is there any reason memfd_create() could fail where trying shm_open() wouldn't be a valid fallback?

Looking at the memfd_create man page, EMFILE/ENFILE would fail in the same way for shm_open(), ENOMEM would probably have the same result for shm_open() too, and EINVAL can only happen if it's used wrong.

Created attachment 327791 [details] [review]
wayland: fall back to shm_open if memfd unavailable

Debian stable currently ships with a 3.16 kernel, so
it doesn't have memfd available.

This commit adds shm_open fall back code for that case
(for now).

Review of attachment 327791 [details] [review]:

Other than that, looks fine.

::: gdk/wayland/gdkdisplay-wayland.c
@@ +1100,3 @@
+          random_byte == '/')
+        continue;
+

This looks a bit manual and wasteful to me. Instead of throwing away most of the randomness and using only one byte at a time, I'd probably translate the whole random int into hex digits in one go. No need to look out for / and \0 either then.

Created attachment 327976 [details] [review]
wayland: fall back to shm_open if memfd unavailable

Debian stable currently ships with a 3.16 kernel, so
it doesn't have memfd available.

This commit adds shm_open fall back code for that case
(for now).

So attachment 327976 [details] [review] does it in a way that minimizes the number of thrown away random bits.

It might be a little over engineered, i'll do one more pass that is slightly more likely to lead to unhandled collisions but much simpler.

Review of attachment 327976 [details] [review]:

::: gdk/wayland/gdkdisplay-wayland.c
@@ +1132,1 @@
   ret = syscall (__NR_memfd_create, "gdk-wayland", MFD_CLOEXEC);

so i'm not handling EINTR here, which means we could end up using yucky shm fds on memfd systems sporatically.  I'm also not remembering to use fall back for subsequent calls so users stuck with yucky shm fds will do futile memfd_create calls every time.

(In reply to Ray Strode [halfline] from comment #11)
> we may have to check for EPERM in addition to ENOSYS.  see this, oddly
> coincidental, irc log from this morning:
So this turns out to be a bug in the code, I thought syscall() returned the error code (inverted), but it sets errno.

Created attachment 327984 [details] [review]
wayland: fix error handling for memfd_create

We currently use syscall() directly to invoke memfd_create,
since the function isn't available in libc headers yet.

The code, though, mishandles how errors are passed from syscall().
It assumes syscall returns the error code directly (but negative),
when in fact, syscall() uses errno.

Also, the code fails to retry on EINTR.

This commit moves the handling of memfd create to a helper function,
and changes the code to use errno and handle EINTR.

Created attachment 327985 [details] [review]
wayland: fall back to shm_open if memfd unavailable

Debian stable currently ships with a 3.16 kernel, so
it doesn't have memfd available.

This commit adds shm_open fall back code for that case
(for now).

this round should be in better shape, I think. I'd still like Emanuele to test this before we push it, of course.

Review of attachment 327984 [details] [review]:

::: gdk/wayland/gdkdisplay-wayland.c
@@ +1087,3 @@
+      ret = syscall (__NR_memfd_create, "gdk-wayland", MFD_CLOEXEC);
+    }
+  while (ret < 0 && errno != EINTR);

that should be == EINTR not != EINTR

Created attachment 327992 [details] [review]
wayland: fix error handling for memfd_create

We currently use syscall() directly to invoke memfd_create,
since the function isn't available in libc headers yet.

The code, though, mishandles how errors are passed from syscall().
It assumes syscall returns the error code directly (but negative),
when in fact, syscall() uses errno.

Also, the code fails to retry on EINTR.

This commit moves the handling of memfd create to a helper function,
and changes the code to use errno and handle EINTR.

Created attachment 327993 [details] [review]
wayland: fall back to shm_open if memfd unavailable

Debian stable currently ships with a 3.16 kernel, so
it doesn't have memfd available.

This commit adds shm_open fall back code for that case
(for now).

Review of attachment 327992 [details] [review]:

Looks good

Review of attachment 327993 [details] [review]:

thanks for bearing with my pedantic review... looks fine to me know (modulo testing).

Review of attachment 327992 [details] [review]:

::: gdk/wayland/gdkdisplay-wayland.c
@@ +1092,3 @@
+    {
+      g_critical (G_STRLOC ": creating shared memory file failed: %m");
+      return NULL;

still needs work, this shouldn't say NULL, but should say -1.

@@ -1090,3 @@
-  ret = syscall (__NR_memfd_create, "gdk-wayland", MFD_CLOEXEC);
-
-  if (ret < 0)

we ended up inadvertently losing error handling here.

Created attachment 328002 [details] [review]
wayland: fix error handling for memfd_create

We currently use syscall() directly to invoke memfd_create,
since the function isn't available in libc headers yet.

The code, though, mishandles how errors are passed from syscall().
It assumes syscall returns the error code directly (but negative),
when in fact, syscall() uses errno.

Also, the code fails to retry on EINTR.

This commit moves the handling of memfd create to a helper function,
and changes the code to use errno and handle EINTR.

Created attachment 328003 [details] [review]
wayland: fall back to shm_open if memfd unavailable

Debian stable currently ships with a 3.16 kernel, so
it doesn't have memfd available.

This commit adds shm_open fall back code for that case
(for now).

Review of attachment 328003 [details] [review]:

eek every time i got close the browser tab with the patch i spot something new.

::: gdk/wayland/gdkdisplay-wayland.c
@@ +1102,3 @@
+           * See bug 766341
+           */
+          if (ret < 0 && errno == -ENOSYS)

should be ENOSYS not -ENOSYS

Created attachment 328005 [details] [review]
wayland: fall back to shm_open if memfd unavailable

Debian stable currently ships with a 3.16 kernel, so
it doesn't have memfd available.

This commit adds shm_open fall back code for that case
(for now).

Review of attachment 328005 [details] [review]:

Stil looks good to me

::: gdk/wayland/gdkdisplay-wayland.c
@@ +1125,3 @@
   if (ret < 0)
+    g_critical (G_STRLOC ": creating shared memory file failed: %m (fallback: %s)",
+                force_shm_open? "true" : "false");

Instead of the meaningless 'fallback', I might reword this as

"creating shared memory file (%s) failed: %m", force_shm_open ? "shm_open" : "memfd_create"

Review of attachment 328002 [details] [review]:

still looks good to me

Emanuele, do these patches work for you ?

Sorry, I got massively distracted by other things and I won't be able to test before the end of next week. :/

no worries; the patches will patiently wait here for your testing

just a friendly ping, no worries if you're too busy.

Emanuele, just another gentle nudge. :-)

Sorry for taking so long.

Tested with kernel 3.14, worked flawlessy, thanks!

Attachment 328002 [details] pushed as bb2ca3b - wayland: fix error handling for memfd_create
Attachment 328005 [details] pushed as 2f3cb31 - wayland: fall back to shm_open if memfd unavailable