Bug 1737812

In upstream code, the close is done in close_fds with MAX_FD_NO = 65535 instead of 2000000:

/* Closes all descriptors after stderr. */
static void
close_fds(void)
{
        /*
         * An arbitrary upper limit for the maximum file descriptor number
         * returned by RLIMIT_NOFILE.
         */
        const int MAX_FD_NO = 65535;

        /* The lower limit is the same as for _POSIX_OPEN_MAX. */
        const int MIN_FD_NO = 20;

        int fd;
        struct rlimit rlim;

        if (getrlimit(RLIMIT_NOFILE, &rlim) || rlim.rlim_max > MAX_FD_NO)
                fd = MAX_FD_NO;
        else if (rlim.rlim_max < MIN_FD_NO)
                fd = MIN_FD_NO;
        else
                fd = rlim.rlim_max - 1;

        for (; fd > STDERR_FILENO; --fd)
                close(fd);
}

In RH7-8, pam 1.1.8:
#define MAX_FD_NO 2000000

Actually I've mistakenly thought that the RHEL-8 code is the same as on RHEL-7, but that was already replaced with the close_fds() call. So the problem on RHEL-8 is much much lower than on RHEL-7. But there is still room for optimization if /proc/self/fd is available.

On RHEL8, this is not a perf issue anymore. With selinux enabled, I got 4 ms with nofile = 4096 or 5M. With the patch below, it take only 2ms.
But for me, using an arbitrary MAX_FD_NO seems to be a bug, so we can try a minor improvement.

###### limits.conf
foo		hard	nofile	5242880
foo		soft	nofile	5242880

###### Without the patch
# ./check_user foo
pam_start 1565099210.112
pam_authenticate 1565099210.116
Password: 
pam_acct_mgmt 1565099212.918
ending 1565099212.922
Authenticated
pam_end 1565099212.922

###### With the patch
# ./check_user foo
pam_start 1565100047.233
pam_authenticate 1565100047.234
Password: 
pam_acct_mgmt 1565100049.152
ending 1565100049.154
Authenticated
pam_end 1565100049.154



/* Closes all descriptors after stderr. */
static void
close_fds(void)
{
        int fd;
        DIR *dir;
        struct dirent *dent;

        if ((dir = opendir("/proc/self/fd")) != NULL) {
                while ((dent = readdir(dir)) != NULL) {
                        if (!strcmp(dent->d_name, ".")
                            || !strcmp(dent->d_name, ".."))
                                continue;
                        fd = atoi(dent->d_name);
                        if (fd > STDERR_FILENO)
                                close(fd);
                }
        }
}


$ cat pam-1.3.1-closefds.patch
diff -up Linux-PAM-1.3.1/libpam/pam_modutil_sanitize.c.cbesson Linux-PAM-1.3.1/libpam/pam_modutil_sanitize.c
--- Linux-PAM-1.3.1/libpam/pam_modutil_sanitize.c.cbesson	2017-02-10 11:10:15.000000000 +0100
+++ Linux-PAM-1.3.1/libpam/pam_modutil_sanitize.c	2019-08-06 15:29:35.000000000 +0200
@@ -10,6 +10,7 @@
 #include <fcntl.h>
 #include <syslog.h>
 #include <sys/resource.h>
+#include <dirent.h>
 
 /*
  * Creates a pipe, closes its write end, redirects fd to its read end.
@@ -116,27 +117,20 @@ redirect_out(pam_handle_t *pamh, enum pa
 static void
 close_fds(void)
 {
-	/*
-	 * An arbitrary upper limit for the maximum file descriptor number
-	 * returned by RLIMIT_NOFILE.
-	 */
-	const int MAX_FD_NO = 65535;
-
-	/* The lower limit is the same as for _POSIX_OPEN_MAX. */
-	const int MIN_FD_NO = 20;
-
 	int fd;
-	struct rlimit rlim;
+	DIR *dir;
+	struct dirent *dent;
 
-	if (getrlimit(RLIMIT_NOFILE, &rlim) || rlim.rlim_max > MAX_FD_NO)
-		fd = MAX_FD_NO;
-	else if (rlim.rlim_max < MIN_FD_NO)
-		fd = MIN_FD_NO;
-	else
-		fd = rlim.rlim_max - 1;
-
-	for (; fd > STDERR_FILENO; --fd)
-		close(fd);
+	if ((dir = opendir("/proc/self/fd")) != NULL) {
+		while ((dent = readdir(dir)) != NULL) {
+			if (!strcmp(dent->d_name, ".")
+			    || !strcmp(dent->d_name, ".."))
+				continue;
+			fd = atoi(dent->d_name);
+			if (fd > STDERR_FILENO)
+				close(fd);
+		}
+	}
 }
 
 int

The patch must properly fallback to the previous behavior if opendir fails. Also please submit that as PR on upstream project in GitHub.

Yep, you're right. Will try to submit that on upstream.


--- libpam/pam_modutil_sanitize.c	2019-08-07 11:36:40.000000000 +0200
+++ libpam/pam_modutil_sanitize.c.new	2019-08-07 12:09:35.000000000 +0200
@@ -10,6 +10,7 @@
 #include <fcntl.h>
 #include <syslog.h>
 #include <sys/resource.h>
+#include <dirent.h>
 
 /*
  * Creates a pipe, closes its write end, redirects fd to its read end.
@@ -116,6 +117,9 @@
 static void
 close_fds(void)
 {
+	DIR *dir;
+	struct dirent *dent;
+
 	/*
 	 * An arbitrary upper limit for the maximum file descriptor number
 	 * returned by RLIMIT_NOFILE.
@@ -128,15 +132,30 @@
 	int fd;
 	struct rlimit rlim;
 
-	if (getrlimit(RLIMIT_NOFILE, &rlim) || rlim.rlim_max > MAX_FD_NO)
-		fd = MAX_FD_NO;
-	else if (rlim.rlim_max < MIN_FD_NO)
-		fd = MIN_FD_NO;
-	else
-		fd = rlim.rlim_max - 1;
-
-	for (; fd > STDERR_FILENO; --fd)
-		close(fd);
+	/* 
+	 * if /proc is mounted, we can optimize what fd can be closed,
+ 	 * but if it isn't available, fall back to the previous behavior.
+	 */
+	if ((dir = opendir("/proc/self/fd")) != NULL) {
+		while ((dent = readdir(dir)) != NULL) {
+			if (!strcmp(dent->d_name, ".")
+			    || !strcmp(dent->d_name, ".."))
+				continue;
+			fd = atoi(dent->d_name);
+			if (fd > STDERR_FILENO)
+				close(fd);
+		}
+	} else {
+		if (getrlimit(RLIMIT_NOFILE, &rlim) || rlim.rlim_max > MAX_FD_NO)
+			fd = MAX_FD_NO;
+		else if (rlim.rlim_max < MIN_FD_NO)
+			fd = MIN_FD_NO;
+		else
+			fd = rlim.rlim_max - 1;
+	
+		for (; fd > STDERR_FILENO; --fd)
+			close(fd);
+	}
 }

Submitted.

https://github.com/linux-pam/linux-pam/pull/130

Created attachment 1646412 [details]
Test tool

This simple tool will perform the authentication.

You will need a pam configuration file /etc/pam.d/tst-pam-unix-closefd with just:
auth required pam_unix.so

If you want the authentication to succeed (not necessary for the test) you also need an user tstpamunix with password pamunix01PSWD

Then just run the test with:
make tst-pam_unix-closefd LDFLAGS='-lpam' CFLAGS='-g -O2 -Wall'
strace -f -o log ./tst-pam_unix-closefd

There should be a line with close(345) in the log and there should not be a line with close(344) in the log.

Small correction: The user tstpamunix must be present on the system for the test to work.

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHBA-2020:1780