On Fri, Oct 3, 2014 at 7:27 AM, Thomas De Schampheleire <patrickdepinguin@xxxxxxxxx> wrote: > Hi Lucas, > > On Thu, Oct 2, 2014 at 7:55 PM, Lucas De Marchi > <lucas.de.marchi@xxxxxxxxx> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-modules" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
