[PATCH] Add dummy definition of O_CLOEXEC

[Thomas De Schampheleire]

From: Robert Yang <liezhi.yang-***@public.gmane.org>

O_CLOEXEC is introduced from Linux 2.6.23, so old kernel doesn't have
it, we need check before use.

This patch is much more like a workaround, since it may need fcntl() use
FD_CLOEXEC to replace.

This problem was reported by "Ting Liu <b28495-***@public.gmane.org>"

[Thomas De Schampheleire <thomas.de.schampheleire-***@public.gmane.org:
- move dummy definition from libkmod-internal.h to missing.h
- update commit title]
---
libkmod/missing.h | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/libkmod/missing.h b/libkmod/missing.h
index 4c0d136..e123e98 100644
--- a/libkmod/missing.h
+++ b/libkmod/missing.h
@@ -19,6 +19,10 @@
# define __NR_finit_module -1
#endif

+#ifndef O_CLOEXEC
+#define O_CLOEXEC 0
+#endif
+
#ifndef HAVE_FINIT_MODULE
#include <errno.h>

[Lucas De Marchi]

On Tue, Sep 30, 2014 at 4:41 AM, Thomas De Schampheleire

Post by Thomas De Schampheleire
O_CLOEXEC is introduced from Linux 2.6.23, so old kernel doesn't have
it, we need check before use.

Humn... Do we really want to support kernels older than 2.6.23?

Adding a workaround like this IMO will just hide bugs because we rely
on O_CLOEXEC semantics. Doing nothing is not really what we want.
Maybe if ancient downstream distros want the workaround they can
define O_CLOEXEC by themselves during build... passing it in CFLAGS
should work

[Marco d'Itri]

Post by Lucas De Marchi
Humn... Do we really want to support kernels older than 2.6.23?

No, not even pre-systemd udev supports them.

[Thomas De Schampheleire]

Post by Lucas De Marchi
On Tue, Sep 30, 2014 at 4:41 AM, Thomas De Schampheleire

Post by Thomas De Schampheleire
O_CLOEXEC is introduced from Linux 2.6.23, so old kernel doesn't have
it, we need check before use.

Humn... Do we really want to support kernels older than 2.6.23?
Adding a workaround like this IMO will just hide bugs because we rely
on O_CLOEXEC semantics. Doing nothing is not really what we want.
Maybe if ancient downstream distros want the workaround they can
define O_CLOEXEC by themselves during build... passing it in CFLAGS
should work

This is the same type of distro for which the
implementation of be32toh was needed: RHEL5.
Ancient, yes, but still supported and used in corporate
environments, also to build modern systems, for
example using Buildroot or Openembedded.

The patch comes from openembedded and is about
to be integrated in Buildroot too, but it's far more
advantageous to have such changes integrated
upstream.

Thanks,
Thomas

[Lucas De Marchi]

On Thu, Oct 2, 2014 at 2:31 AM, Thomas De Schampheleire

Post by Thomas De Schampheleire

Post by Lucas De Marchi
On Tue, Sep 30, 2014 at 4:41 AM, Thomas De Schampheleire

Post by Thomas De Schampheleire
O_CLOEXEC is introduced from Linux 2.6.23, so old kernel doesn't have
it, we need check before use.

Humn... Do we really want to support kernels older than 2.6.23?
Adding a workaround like this IMO will just hide bugs because we rely
on O_CLOEXEC semantics. Doing nothing is not really what we want.
Maybe if ancient downstream distros want the workaround they can
define O_CLOEXEC by themselves during build... passing it in CFLAGS
should work

This is the same type of distro for which the
implementation of be32toh was needed: RHEL5.

Similar, but not the same. For the implementation of be32toh we depend
on *libc* having it. As I said in the original email, I was surprised
a libc could possibly miss it, but I was ok with adding a fallback
implementation (maybe we should even go one step further and do what
systemd does to check our use cases with sparse).

Post by Thomas De Schampheleire
Ancient, yes, but still supported and used in corporate
environments, also to build modern systems, for
example using Buildroot or Openembedded.

That's my worry. If you are building modern systems and you don't have
O_CLOEXEC (and assuming you are not trying to put 2.6.22 in your
embedded system), I'd say you are using the wrong headers, from host
instead of target.

Post by Thomas De Schampheleire
The patch comes from openembedded and is about
to be integrated in Buildroot too, but it's far more
advantageous to have such changes integrated
upstream.

They shouldn't be defining O_CLOEXEC to 0 in the first place. I don't
want to support leaking file descriptors to child process. You are
silently giving different behavior to your target depending on the
machine it was built with :-o.

CC'ing buildroot mailing list. Peter, do you do this for other packages as well?

[Thomas De Schampheleire]

On Thu, Oct 2, 2014 at 2:07 PM, Lucas De Marchi

Post by Lucas De Marchi
On Thu, Oct 2, 2014 at 2:31 AM, Thomas De Schampheleire

Post by Thomas De Schampheleire

Post by Lucas De Marchi
On Tue, Sep 30, 2014 at 4:41 AM, Thomas De Schampheleire

Post by Thomas De Schampheleire
O_CLOEXEC is introduced from Linux 2.6.23, so old kernel doesn't have
it, we need check before use.

Humn... Do we really want to support kernels older than 2.6.23?
Adding a workaround like this IMO will just hide bugs because we rely
on O_CLOEXEC semantics. Doing nothing is not really what we want.
Maybe if ancient downstream distros want the workaround they can
define O_CLOEXEC by themselves during build... passing it in CFLAGS
should work

This is the same type of distro for which the
implementation of be32toh was needed: RHEL5.

Similar, but not the same. For the implementation of be32toh we depend
on *libc* having it. As I said in the original email, I was surprised
a libc could possibly miss it, but I was ok with adding a fallback
implementation (maybe we should even go one step further and do what
systemd does to check our use cases with sparse).

Post by Thomas De Schampheleire
Ancient, yes, but still supported and used in corporate
environments, also to build modern systems, for
example using Buildroot or Openembedded.

That's my worry. If you are building modern systems and you don't have
O_CLOEXEC (and assuming you are not trying to put 2.6.22 in your
embedded system), I'd say you are using the wrong headers, from host
instead of target.

In Buildroot, we're not leaking host kernel headers into the target.
The kernel headers used for target compilation are those coming with
the toolchain, which are recent kernel headers supporting O_CLOEXEC.

The problem I'm encountering (and people on OpenEmbedded apparently
have too) is when building host-kmod, that is the kmod tools that run
on the host system (depmod --> kmod). These are (in buildroot at
least) built using the toolchain on the host system, and thus with the
kernel headers from the host system.
On RHEL5 systems these are old kernel headers from pre-O_CLOEXEC times.

Post by Lucas De Marchi

Post by Thomas De Schampheleire
The patch comes from openembedded and is about
to be integrated in Buildroot too, but it's far more
advantageous to have such changes integrated
upstream.

They shouldn't be defining O_CLOEXEC to 0 in the first place. I don't
want to support leaking file descriptors to child process. You are
silently giving different behavior to your target depending on the
machine it was built with :-o.

As explained above, this is not what is happening. The O_CLOEXEC dummy
definition to 0 is only relevant when building host tools (depmod) on
old systems like RHEL5. These host tools are effectively run on the
same host where they are built. When building for the target, or when
building on modern host systems, the dummy does not set in and there
is anyway no impact.

Hope this clarifies,

Thomas

[Lucas De Marchi]

On Thu, Oct 2, 2014 at 10:29 AM, Thomas De Schampheleire

Post by Thomas De Schampheleire
On Thu, Oct 2, 2014 at 2:07 PM, Lucas De Marchi

Post by Lucas De Marchi
On Thu, Oct 2, 2014 at 2:31 AM, Thomas De Schampheleire

Post by Thomas De Schampheleire

Post by Lucas De Marchi
On Tue, Sep 30, 2014 at 4:41 AM, Thomas De Schampheleire

Post by Thomas De Schampheleire
O_CLOEXEC is introduced from Linux 2.6.23, so old kernel doesn't have
it, we need check before use.

Humn... Do we really want to support kernels older than 2.6.23?
Adding a workaround like this IMO will just hide bugs because we rely
on O_CLOEXEC semantics. Doing nothing is not really what we want.
Maybe if ancient downstream distros want the workaround they can
define O_CLOEXEC by themselves during build... passing it in CFLAGS
should work

This is the same type of distro for which the
implementation of be32toh was needed: RHEL5.

Similar, but not the same. For the implementation of be32toh we depend
on *libc* having it. As I said in the original email, I was surprised
a libc could possibly miss it, but I was ok with adding a fallback
implementation (maybe we should even go one step further and do what
systemd does to check our use cases with sparse).

Post by Thomas De Schampheleire
Ancient, yes, but still supported and used in corporate
environments, also to build modern systems, for
example using Buildroot or Openembedded.

That's my worry. If you are building modern systems and you don't have
O_CLOEXEC (and assuming you are not trying to put 2.6.22 in your
embedded system), I'd say you are using the wrong headers, from host
instead of target.

In Buildroot, we're not leaking host kernel headers into the target.
The kernel headers used for target compilation are those coming with
the toolchain, which are recent kernel headers supporting O_CLOEXEC.
The problem I'm encountering (and people on OpenEmbedded apparently
have too) is when building host-kmod, that is the kmod tools that run
on the host system (depmod --> kmod). These are (in buildroot at
least) built using the toolchain on the host system, and thus with the
kernel headers from the host system.
On RHEL5 systems these are old kernel headers from pre-O_CLOEXEC times.

Post by Lucas De Marchi

Post by Thomas De Schampheleire
The patch comes from openembedded and is about
to be integrated in Buildroot too, but it's far more
advantageous to have such changes integrated
upstream.

They shouldn't be defining O_CLOEXEC to 0 in the first place. I don't
want to support leaking file descriptors to child process. You are
silently giving different behavior to your target depending on the
machine it was built with :-o.

As explained above, this is not what is happening. The O_CLOEXEC dummy
definition to 0 is only relevant when building host tools (depmod) on
old systems like RHEL5. These host tools are effectively run on the
same host where they are built. When building for the target, or when
building on modern host systems, the dummy does not set in and there
is anyway no impact.
Hope this clarifies,

Yep, this clarifies, thanks.

However it only makes sense for this specific scenario, i.e. you don't
care for depmod leaking fds on the host... It's not something
acceptable to do on upstream, sorry.

[Thomas De Schampheleire]

Hi Lucas,

On Thu, Oct 2, 2014 at 7:55 PM, Lucas De Marchi

Post by Lucas De Marchi

Post by Thomas De Schampheleire

Post by Lucas De Marchi
They shouldn't be defining O_CLOEXEC to 0 in the first place. I don't
want to support leaking file descriptors to child process. You are
silently giving different behavior to your target depending on the
machine it was built with :-o.

As explained above, this is not what is happening. The O_CLOEXEC dummy
definition to 0 is only relevant when building host tools (depmod) on
old systems like RHEL5. These host tools are effectively run on the
same host where they are built. When building for the target, or when
building on modern host systems, the dummy does not set in and there
is anyway no impact.
Hope this clarifies,

Yep, this clarifies, thanks.
However it only makes sense for this specific scenario, i.e. you don't
care for depmod leaking fds on the host... It's not something
acceptable to do on upstream, sorry.

I'm trying to understand the reason you object to this patch.

We both agree that the dummy implementation of O_CLOEXEC will only
take effect when building kmod using kernel headers < 2.6.23, correct?

In such a case, there are three paths:

1. Don't make any change (the current situation). In this case, kmod
cannot be compiled due to the missing O_CLOEXEC definition and thus
cannot be used at all.

2. Add a dummy definition (as proposed by the patch) to allow
compilation of kmod. In this case, the O_CLOEXEC flag will not take
effect, indeed with the leaking file descriptors into child processes,
but only in this exceptional case where kmod is built for older
kernels.

3. Use explicit fcntl(..., FD_CLOEXEC...) calls after the open's.
However, this is not easily done without impact on the standard case
with modern headers. I assume you will like this even less.

To me, case 2 seems a valid solution to an otherwise unusable kmod on
systems with old kernels / kernel headers. The impact of the leaked
file descriptors is to be accepted in this case.

However, if you still feel this is not upstreamable, then I will back
off and hope that Peter will accept the patch in Buildroot :)

Thanks,
Thomas

[Lucas De Marchi]

On Fri, Oct 3, 2014 at 7:27 AM, Thomas De Schampheleire

Post by Thomas De Schampheleire
Hi Lucas,
On Thu, Oct 2, 2014 at 7:55 PM, Lucas De Marchi

Post by Lucas De Marchi

Post by Thomas De Schampheleire

Post by Lucas De Marchi
They shouldn't be defining O_CLOEXEC to 0 in the first place. I don't
want to support leaking file descriptors to child process. You are
silently giving different behavior to your target depending on the
machine it was built with :-o.

As explained above, this is not what is happening. The O_CLOEXEC dummy
definition to 0 is only relevant when building host tools (depmod) on
old systems like RHEL5. These host tools are effectively run on the
same host where they are built. When building for the target, or when
building on modern host systems, the dummy does not set in and there
is anyway no impact.
Hope this clarifies,

Yep, this clarifies, thanks.
However it only makes sense for this specific scenario, i.e. you don't
care for depmod leaking fds on the host... It's not something
acceptable to do on upstream, sorry.

I'm trying to understand the reason you object to this patch.
We both agree that the dummy implementation of O_CLOEXEC will only
take effect when building kmod using kernel headers < 2.6.23, correct?

yes. But I don't want to pretend that kmod works fine there. It doesn't.

Post by Thomas De Schampheleire
1. Don't make any change (the current situation). In this case, kmod
cannot be compiled due to the missing O_CLOEXEC definition and thus
cannot be used at all.
2. Add a dummy definition (as proposed by the patch) to allow
compilation of kmod. In this case, the O_CLOEXEC flag will not take
effect, indeed with the leaking file descriptors into child processes,
but only in this exceptional case where kmod is built for older
kernels.

The problem here is that you are silently accepting the misbehave.

Post by Thomas De Schampheleire
3. Use explicit fcntl(..., FD_CLOEXEC...) calls after the open's.
However, this is not easily done without impact on the standard case
with modern headers. I assume you will like this even less.
To me, case 2 seems a valid solution to an otherwise unusable kmod on
systems with old kernels / kernel headers. The impact of the leaked
file descriptors is to be accepted in this case.

I'm all for accepting the leaked file descriptors *in this case*. Not
in the general case in which one takes kmod and realizes it works fine
in < 2.6.23. Because it doesn't and pretending it does is just asking
for invalid bug reports.

So as I said, I don't think this is material for upstream.

What we could do is to hide the definition of O_CLOEXEC behind a
configure flag... but then the amount of ifdef, the benefits from it
and having to maintain that on build sys don't pay off IMO.

Post by Thomas De Schampheleire
However, if you still feel this is not upstreamable, then I will back
off and hope that Peter will accept the patch in Buildroot :)

I do think this could be accepted there. AFAICS he already acked on
it. Just make sure this is a patch that gets applied on host-kmod
only, not the one running on target. Let's try this approach and come
back to what I suggested above if it becomes too troublesome to
downstream?

[Thomas De Schampheleire]

Hi Lucas,

On Tue, Oct 7, 2014 at 12:42 AM, Lucas De Marchi

Post by Lucas De Marchi

Post by Thomas De Schampheleire
I'm trying to understand the reason you object to this patch.
We both agree that the dummy implementation of O_CLOEXEC will only
take effect when building kmod using kernel headers < 2.6.23, correct?

yes. But I don't want to pretend that kmod works fine there. It doesn't.

Post by Thomas De Schampheleire
1. Don't make any change (the current situation). In this case, kmod
cannot be compiled due to the missing O_CLOEXEC definition and thus
cannot be used at all.
2. Add a dummy definition (as proposed by the patch) to allow
compilation of kmod. In this case, the O_CLOEXEC flag will not take
effect, indeed with the leaking file descriptors into child processes,
but only in this exceptional case where kmod is built for older
kernels.

The problem here is that you are silently accepting the misbehave.

Post by Thomas De Schampheleire
3. Use explicit fcntl(..., FD_CLOEXEC...) calls after the open's.
However, this is not easily done without impact on the standard case
with modern headers. I assume you will like this even less.
To me, case 2 seems a valid solution to an otherwise unusable kmod on
systems with old kernels / kernel headers. The impact of the leaked
file descriptors is to be accepted in this case.

I'm all for accepting the leaked file descriptors *in this case*. Not
in the general case in which one takes kmod and realizes it works fine
in < 2.6.23. Because it doesn't and pretending it does is just asking
for invalid bug reports.
So as I said, I don't think this is material for upstream.
What we could do is to hide the definition of O_CLOEXEC behind a
configure flag... but then the amount of ifdef, the benefits from it
and having to maintain that on build sys don't pay off IMO.

Post by Thomas De Schampheleire
However, if you still feel this is not upstreamable, then I will back
off and hope that Peter will accept the patch in Buildroot :)

I do think this could be accepted there. AFAICS he already acked on
it. Just make sure this is a patch that gets applied on host-kmod
only, not the one running on target. Let's try this approach and come
back to what I suggested above if it becomes too troublesome to
downstream?

Thanks for your further explanation.
I will push the patch to buildroot, as suggested.

Best regards,
Thomas