Re: [PATCH] mark newly opened fds as FD_CLOEXEC (close on exec) [part 2]

["Maciej Żenczykowski" <zenczykowski@gmail.com>]

But that's a kernel feature, and I don't think we can rely on it being
present in iptables userspace (it has to be backwards compatible to
IMHO at least back to 2.6.9 if at all possible),
and adding the "try with flag, if fails, retry without flag and
manually set it" logic seems overkill.

I think such details only matter for multithreaded programs with exec races.
Which I don't believe to be the case here.

On Wed, Mar 21, 2012 at 1:27 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Wed, 2012-03-21 at 03:52 -0700, Maciej Żenczykowski wrote:
>> From: Maciej Żenczykowski <maze@google.com>
>>
>> This is iptables-1.4.11-cloexec.patch from Fedora 18 iptables source
>> rpm, in particular:
>>   http://kojipkgs.fedoraproject.org/packages/iptables/1.4.12.2/4.fc18/src/iptables-1.4.12.2-4.fc18.src.rpm
>>
>> Signed-off-by: Maciej Żenczykowski <maze@google.com>
>> ---
>>  extensions/libxt_set.h |    7 +++++++
>>  libiptc/libiptc.c      |    8 ++++++++
>>  2 files changed, 15 insertions(+), 0 deletions(-)
>>
>> diff --git a/extensions/libxt_set.h b/extensions/libxt_set.h
>> index 4ac84fa9c022..47c3f5b6f5d4 100644
>> --- a/extensions/libxt_set.h
>> +++ b/extensions/libxt_set.h
>> @@ -2,6 +2,7 @@
>>  #define _LIBXT_SET_H
>>
>>  #include <unistd.h>
>> +#include <fcntl.h>
>>  #include <sys/types.h>
>>  #include <sys/socket.h>
>>  #include <errno.h>
>> @@ -23,6 +24,12 @@ get_version(unsigned *version)
>>               xtables_error(OTHER_PROBLEM,
>>                             "Can't open socket to ipset.\n");
>>
>> +     if (fcntl(sockfd, F_SETFD, FD_CLOEXEC) == -1) {
>> +             xtables_error(OTHER_PROBLEM,
>> +                           "Could not set close on exec: %s\n",
>> +                           strerror(errno));
>> +     }
>> +
>>       req_version.op = IP_SET_OP_VERSION;
>>       res = getsockopt(sockfd, SOL_IP, SO_IP_SET, &req_version, &size);
>>       if (res != 0)
>> diff --git a/libiptc/libiptc.c b/libiptc/libiptc.c
>> index 13e41d525f28..63965e738596 100644
>> --- a/libiptc/libiptc.c
>> +++ b/libiptc/libiptc.c
>> @@ -29,6 +29,8 @@
>>   *   - performance work: speedup initial ruleset parsing.
>>   *   - sponsored by ComX Networks A/S (http://www.comx.dk/)
>>   */
>> +#include <unistd.h>
>> +#include <fcntl.h>
>>  #include <sys/types.h>
>>  #include <sys/socket.h>
>>  #include <stdbool.h>
>> @@ -1316,6 +1318,12 @@ TC_INIT(const char *tablename)
>>       if (sockfd < 0)
>>               return NULL;
>>
>> +     if (fcntl(sockfd, F_SETFD, FD_CLOEXEC) == -1) {
>> +             fprintf(stderr, "Could not set close on exec: %s\n",
>> +                     strerror(errno));
>> +             abort();
>> +     }
>> +
>>  retry:
>>       s = sizeof(info);
>>
>
> Looks fine, but since commit a677a039b (flag parameters: socket and
> socketpair) from Ulrich Drepper, we can pass SOCK_CLOEXEC directly in
> the socket() call, thus avoiding extra fcntl() call.
>
> Not relevant to iptables (opening this socket in iptables is not
> performance critical), but worth to mention for reference.
>
>
>



-- 
Maciej A. Żenczykowski
Kernel Networking Developer @ Google
1600 Amphitheatre Parkway, Mountain View, CA 94043
tel: +1 (650) 253-0062
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html