[PATCH] Add dummy definition of O_CLOEXEC

[PATCH] Add dummy definition of O_CLOEXEC

[Thomas De Schampheleire]

From: Robert Yang <liezhi.yang@windriver.com>

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@freescale.com>"

[Thomas De Schampheleire <thomas.de.schampheleire@gmail.com:
 - 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>
 
-- 
1.7.1

Re: [PATCH] Add dummy definition of O_CLOEXEC

[Lucas De Marchi]

On Tue, Sep 30, 2014 at 4:41 AM, Thomas De Schampheleire
<patrickdepinguin@gmail.com> wrote:
> From: Robert Yang <liezhi.yang@windriver.com>
>
> 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

-- 
Lucas De Marchi

Re: [PATCH] Add dummy definition of O_CLOEXEC

[Marco d'Itri]

[-- Attachment #1: Type: text/plain, Size: 199 bytes --]

On Oct 01, Lucas De Marchi <lucas.de.marchi@gmail.com> wrote:

> Humn... Do we really want to support kernels older than 2.6.23?
No, not even pre-systemd udev supports them.

-- 
ciao,
Marco

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 648 bytes --]

Re: [PATCH] Add dummy definition of O_CLOEXEC

[Thomas De Schampheleire]

Lucas De Marchi <lucas.de.marchi@gmail.com> schreef:
>On Tue, Sep 30, 2014 at 4:41 AM, Thomas De Schampheleire
><patrickdepinguin@gmail.com> wrote:
>> From: Robert Yang <liezhi.yang@windriver.com>
>>
>> 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

Re: [PATCH] Add dummy definition of O_CLOEXEC

[Lucas De Marchi]

On Thu, Oct 2, 2014 at 2:31 AM, Thomas De Schampheleire
<patrickdepinguin@gmail.com> wrote:
> Lucas De Marchi <lucas.de.marchi@gmail.com> schreef:
>>On Tue, Sep 30, 2014 at 4:41 AM, Thomas De Schampheleire
>><patrickdepinguin@gmail.com> wrote:
>>> From: Robert Yang <liezhi.yang@windriver.com>
>>>
>>> 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).


> 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.

> 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?


-- 
Lucas De Marchi

Re: [PATCH] Add dummy definition of O_CLOEXEC

[Thomas De Schampheleire]

On Thu, Oct 2, 2014 at 2:07 PM, Lucas De Marchi
<lucas.de.marchi@gmail.com> wrote:
> On Thu, Oct 2, 2014 at 2:31 AM, Thomas De Schampheleire
> <patrickdepinguin@gmail.com> wrote:
>> Lucas De Marchi <lucas.de.marchi@gmail.com> schreef:
>>>On Tue, Sep 30, 2014 at 4:41 AM, Thomas De Schampheleire
>>><patrickdepinguin@gmail.com> wrote:
>>>> From: Robert Yang <liezhi.yang@windriver.com>
>>>>
>>>> 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).
>
>
>> 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.

>
>> 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

Re: [PATCH] Add dummy definition of O_CLOEXEC

[Lucas De Marchi]

On Thu, Oct 2, 2014 at 10:29 AM, Thomas De Schampheleire
<patrickdepinguin@gmail.com> wrote:
> On Thu, Oct 2, 2014 at 2:07 PM, Lucas De Marchi
> <lucas.de.marchi@gmail.com> wrote:
>> On Thu, Oct 2, 2014 at 2:31 AM, Thomas De Schampheleire
>> <patrickdepinguin@gmail.com> wrote:
>>> Lucas De Marchi <lucas.de.marchi@gmail.com> schreef:
>>>>On Tue, Sep 30, 2014 at 4:41 AM, Thomas De Schampheleire
>>>><patrickdepinguin@gmail.com> wrote:
>>>>> From: Robert Yang <liezhi.yang@windriver.com>
>>>>>
>>>>> 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).
>>
>>
>>> 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.
>
>>
>>> 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.

-- 
Lucas De Marchi

Re: [PATCH] Add dummy definition of O_CLOEXEC

[Thomas De Schampheleire]

Hi Lucas,

On Thu, Oct 2, 2014 at 7:55 PM, Lucas De Marchi
<lucas.de.marchi@gmail.com> wrote:

>>> 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

Re: [PATCH] Add dummy definition of O_CLOEXEC

[Lucas De Marchi]

On Fri, Oct 3, 2014 at 7:27 AM, Thomas De Schampheleire
<patrickdepinguin@gmail.com> wrote:
> Hi Lucas,
>
> On Thu, Oct 2, 2014 at 7:55 PM, Lucas De Marchi
> <lucas.de.marchi@gmail.com> wrote:
>
>>>> 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.

> 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.

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

>
> 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.

> 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?


-- 
Lucas De Marchi

Re: [PATCH] Add dummy definition of O_CLOEXEC

[Thomas De Schampheleire]

Hi Lucas,

On Tue, Oct 7, 2014 at 12:42 AM, Lucas De Marchi
<lucas.de.marchi@gmail.com> wrote:
>>
>> 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.
>
>> 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.
>
> The problem here is that you are silently accepting the misbehave.
>
>>
>> 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.
>
>> 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