> -----Original Message----- > From: NeilBrown [mailto:neilb@xxxxxxx] > Sent: Tuesday, August 30, 2011 1:32 AM > To: Patelczyk, Maciej > Cc: linux-raid@xxxxxxxxxxxxxxx > Subject: Re: [PATCH] mdadm: close parent file descriptors when starting > mdmon. > > On Mon, 29 Aug 2011 17:49:49 +0200 Maciej Patelczyk > <maciej.patelczyk@xxxxxxxxx> wrote: > > > When mdadm is invoked by fork-and-exec it inherits all open file > > descriptors and when mdadm forks to exec mdmon those file descriptors > > are passed to mdmon. Mdmon closes only first 97 fd and that in some > > cases is not enough. > > Can you describe and actual can when it is not enough? Maybe there is > some > other problem where mdadm is not closing things as it should. There is one: CIM Server (Sfcbd - Small Footprint CIM Broker) -> registered provider -> intermediate library -> mdadm -> mdmon We register provider in Sfcbd which brings some functionality to cim server. It uses our library which calls mdadm (popen). Later mdadm is calling mdmon (fork&exec). The problem is that Sfcbd is a server and it has more than 100 files opened. Open files handles are passed down to mdadm (popen behavior) and here we have a problem. > > This commit adds function which looks at the '/proc/<pid>/fd' > directory > > and closes all inherited file descriptors except the standard ones > (0-2). > > I'm not thrilled by this approach. > For a start, the loop could close the fd that is being used to > read /proc/<pid>/fd, so subsequent fds won't get seen or closed. > That's a bug. There is a 'dirfd()' function but it depends on _BSD_SOURCE || _SVID_SOURCE flags so I don't know if this is acceptable. There could be additional check to skip dir fd. > I would much rather do as the comment suggests and use O_CLOEXEC on all > opens > so that everything gets closed when we do an exec. So do I. But we cannot control scenarios in which mdadm is used. O_CLOEXEC is nice but that gives you nothing in scenario like I presented. It looks like Sfcbd is not using O_CLOEXEC. Optional parameter to popen/exec in my opinion would be better. But this is not the case. > > About the most I would be willing to do in closing more fds before the > exec > is to keep closing until we get too many failures. > e.g. > fd = 3; > failed = 0; > while (failed < 20) { > if (close(fd) < 0) > failed ++; > else > failed = 0; > fd++; > } > Imagine: 1. Open 100 files (user #1 action) 2. Open 100 files (user #2 action) 3. Close 43 files from #1 (user #1 action) 4. call mdadm (user #3 action) ... It's not 'most likely' but possible. maciej > NeilBrown > > > > > > Signed-off-by: Maciej Patelczyk <maciej.patelczyk@xxxxxxxxx> > > --- > > util.c | 41 +++++++++++++++++++++++++++++++++++++---- > > 1 files changed, 37 insertions(+), 4 deletions(-) > > > > diff --git a/util.c b/util.c > > index ce03239..b844447 100644 > > --- a/util.c > > +++ b/util.c > > @@ -30,7 +30,12 @@ > > #include <sys/un.h> > > #include <ctype.h> > > #include <dirent.h> > > +#include <sys/types.h> > > #include <signal.h> > > +#include <stdlib.h> > > +#include <string.h> > > +#include <errno.h> > > +#include <unistd.h> > > > > /* > > * following taken from linux/blkpg.h because they aren't > > @@ -1571,11 +1576,41 @@ int mdmon_running(int devnum) > > return 0; > > } > > > > +static void close_parent_fds(void) > > +{ > > + pid_t pid; > > + char buf[128]; > > + DIR *dirp; > > + struct dirent *d_entry; > > + int fd; > > + > > + pid = getpid(); > > + if (snprintf(buf, sizeof(buf), "/proc/%d/fd", (int)pid) < 0) > > + return; > > + > > + dirp = opendir((const char *)buf); > > + if (!dirp) > > + return; > > + > > + while((d_entry = readdir(dirp)) != NULL) { > > + if (!strcmp(d_entry->d_name, ".") || > > + !strcmp(d_entry->d_name, "..")) > > + continue; > > + errno = 0; > > + fd = (int)strtol(d_entry->d_name, NULL, 10); > > + if (errno) > > + continue; > > + if (fd > 2) > > + close(fd); > > + } > > + closedir(dirp); > > +} > > + > > int start_mdmon(int devnum) > > { > > int i; > > int len; > > - pid_t pid; > > + pid_t pid; > > int status; > > char pathbuf[1024]; > > char *paths[4] = { > > @@ -1603,9 +1638,7 @@ int start_mdmon(int devnum) > > > > switch(fork()) { > > case 0: > > - /* FIXME yuk. CLOSE_EXEC?? */ > > - for (i=3; i < 100; i++) > > - close(i); > > + close_parent_fds(); > > for (i=0; paths[i]; i++) > > if (paths[i][0]) > > execl(paths[i], "mdmon", -- To unsubscribe from this list: send the line "unsubscribe linux-raid" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
