* [PATCH] depmod: Prevent module dependency files corruption due to parallel invocation. @ 2018-12-07 15:45 Michal Suchanek 2018-12-10 21:29 ` [PATCH v2 0/3] Fix dependency file corruption with parallel depmod invocation Michal Suchanek 2018-12-17 22:46 ` [PATCH v3 0/3] Fix dependency file corruption with parallel depmod invocation Michal Suchanek 0 siblings, 2 replies; 14+ messages in thread From: Michal Suchanek @ 2018-12-07 15:45 UTC (permalink / raw) To: linux-modules; +Cc: Michal Suchanek Depmod does not use unique filename for temporary files. There is no guarantee the user does not attempt to run mutiple depmod processes in parallel. If that happens a temporary file might be created by depmod(1st), truncated by depmod(2nd), and renamed to final name by depmod(1st) resulting in corrupted file seen by user. Due to missing mkstempat() this is more complex than it should be. Adding PID and random number to the filename should be reasonably reliable. Adding O_EXCL as mkstemp does fails creating the file rather than corrupting existing file. Also prevent dependency files missing. This happens because target files are removed before renaming the temporary file. Signed-off-by: Michal Suchanek <msuchanek@suse.de> --- tools/depmod.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/tools/depmod.c b/tools/depmod.c index 989d9077926c..5526ac892cf8 100644 --- a/tools/depmod.c +++ b/tools/depmod.c @@ -29,6 +29,7 @@ #include <string.h> #include <unistd.h> #include <sys/stat.h> +#include <sys/time.h> #include <sys/utsname.h> #include <shared/array.h> @@ -2398,6 +2399,10 @@ static int depmod_output(struct depmod *depmod, FILE *out) }; const char *dname = depmod->cfg->dirname; int dfd, err = 0; + struct timeval tv; + + gettimeofday(&tv, NULL); + srand(tv.tv_sec); if (out != NULL) dfd = -1; @@ -2412,15 +2417,17 @@ static int depmod_output(struct depmod *depmod, FILE *out) for (itr = depfiles; itr->name != NULL; itr++) { FILE *fp = out; - char tmp[NAME_MAX] = ""; + char tmp[NAME_MAX + 1] = ""; int r, ferr; if (fp == NULL) { - int flags = O_CREAT | O_TRUNC | O_WRONLY; + int flags = O_CREAT | O_TRUNC | O_WRONLY | O_EXCL; int mode = 0644; int fd; - snprintf(tmp, sizeof(tmp), "%s.tmp", itr->name); + snprintf(tmp, sizeof(tmp), "%s.%i.%i", itr->name, getpid(), + rand()); + tmp[NAME_MAX] = 0; fd = openat(dfd, tmp, flags, mode); if (fd < 0) { ERR("openat(%s, %s, %o, %o): %m\n", @@ -2451,7 +2458,6 @@ static int depmod_output(struct depmod *depmod, FILE *out) break; } - unlinkat(dfd, itr->name, 0); if (renameat(dfd, tmp, dfd, itr->name) != 0) { err = -errno; CRIT("renameat(%s, %s, %s, %s): %m\n", -- 2.19.2 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v2 0/3] Fix dependency file corruption with parallel depmod invocation 2018-12-07 15:45 [PATCH] depmod: Prevent module dependency files corruption due to parallel invocation Michal Suchanek @ 2018-12-10 21:29 ` Michal Suchanek 2018-12-10 21:29 ` [PATCH v2 1/3] depmod: prevent module dependency files missing during " Michal Suchanek ` (2 more replies) 2018-12-17 22:46 ` [PATCH v3 0/3] Fix dependency file corruption with parallel depmod invocation Michal Suchanek 1 sibling, 3 replies; 14+ messages in thread From: Michal Suchanek @ 2018-12-10 21:29 UTC (permalink / raw) To: linux-modules; +Cc: Michal Suchanek The files produced by depmod may be corrupted if multiple depmod processes running in parallel try to update them. Since there is nothing stopping the user running depmod in parallel this looks like a bug in depmod. When a package manager is used it synchronizes adding or removeing module files and depmod invocation. However, user might use other script that is not synchronized with package manager or invoke depmod by hand. When global synchronizarion is missing we cannot guarantee completely consistent state because adding and removing module files is anynchronous. However, we should guarantee the system is bootable if the user did not remove the modules needed for boot. For that the dependency file should not get removed or corrupted. Thanks Michal Michal Suchanek (3): depmod: prevent module dependency files missing during depmod invocation. depmod: prevent module dependency files corruption due to parallel invocation. depmod: shut up gcc insufficinet buffer warning. tools/depmod.c | 64 ++++++++++++++++++++++++++++++++++---------------- 1 file changed, 44 insertions(+), 20 deletions(-) -- 2.19.2 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 1/3] depmod: prevent module dependency files missing during depmod invocation. 2018-12-10 21:29 ` [PATCH v2 0/3] Fix dependency file corruption with parallel depmod invocation Michal Suchanek @ 2018-12-10 21:29 ` Michal Suchanek 2018-12-10 21:29 ` [PATCH v2 2/3] depmod: prevent module dependency files corruption due to parallel invocation Michal Suchanek 2018-12-10 21:29 ` [PATCH v2 3/3] depmod: shut up gcc insufficinet buffer warning Michal Suchanek 2 siblings, 0 replies; 14+ messages in thread From: Michal Suchanek @ 2018-12-10 21:29 UTC (permalink / raw) To: linux-modules; +Cc: Michal Suchanek Depmod deletes the module dependency files before moving the temporary files in their place. This results in user seeing no dependency files while they are updated. Remove the unlink call. The rename call should suffice to move the new file in place and unlink the old one. It should also do both atomically so there is no window when no dependency file exists. Signed-off-by: Michal Suchanek <msuchanek@suse.de> --- tools/depmod.c | 1 - 1 file changed, 1 deletion(-) diff --git a/tools/depmod.c b/tools/depmod.c index 989d9077926c..18c0d61b2db3 100644 --- a/tools/depmod.c +++ b/tools/depmod.c @@ -2451,7 +2451,6 @@ static int depmod_output(struct depmod *depmod, FILE *out) break; } - unlinkat(dfd, itr->name, 0); if (renameat(dfd, tmp, dfd, itr->name) != 0) { err = -errno; CRIT("renameat(%s, %s, %s, %s): %m\n", -- 2.19.2 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v2 2/3] depmod: prevent module dependency files corruption due to parallel invocation. 2018-12-10 21:29 ` [PATCH v2 0/3] Fix dependency file corruption with parallel depmod invocation Michal Suchanek 2018-12-10 21:29 ` [PATCH v2 1/3] depmod: prevent module dependency files missing during " Michal Suchanek @ 2018-12-10 21:29 ` Michal Suchanek 2018-12-17 18:10 ` Lucas De Marchi 2018-12-10 21:29 ` [PATCH v2 3/3] depmod: shut up gcc insufficinet buffer warning Michal Suchanek 2 siblings, 1 reply; 14+ messages in thread From: Michal Suchanek @ 2018-12-10 21:29 UTC (permalink / raw) To: linux-modules; +Cc: Michal Suchanek Depmod does not use unique filename for temporary files. There is no guarantee the user does not attempt to run mutiple depmod processes in parallel. If that happens a temporary file might be created by depmod(1st), truncated by depmod(2nd), and renamed to final name by depmod(1st) resulting in corrupted file seen by user. Due to missing mkstempat() this is more complex than it should be. Adding PID and timestamp to the filename should be reasonably reliable. Adding O_EXCL as mkstemp does fails creating the file rather than corrupting existing file. Signed-off-by: Michal Suchanek <msuchanek@suse.de> --- tools/depmod.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/tools/depmod.c b/tools/depmod.c index 18c0d61b2db3..3b6d16e76160 100644 --- a/tools/depmod.c +++ b/tools/depmod.c @@ -29,6 +29,7 @@ #include <string.h> #include <unistd.h> #include <sys/stat.h> +#include <sys/time.h> #include <sys/utsname.h> #include <shared/array.h> @@ -2398,6 +2399,9 @@ static int depmod_output(struct depmod *depmod, FILE *out) }; const char *dname = depmod->cfg->dirname; int dfd, err = 0; + struct timeval tv; + + gettimeofday(&tv, NULL); if (out != NULL) dfd = -1; @@ -2412,15 +2416,17 @@ static int depmod_output(struct depmod *depmod, FILE *out) for (itr = depfiles; itr->name != NULL; itr++) { FILE *fp = out; - char tmp[NAME_MAX] = ""; + char tmp[NAME_MAX + 1] = ""; int r, ferr; if (fp == NULL) { - int flags = O_CREAT | O_TRUNC | O_WRONLY; + int flags = O_CREAT | O_TRUNC | O_WRONLY | O_EXCL; int mode = 0644; int fd; - snprintf(tmp, sizeof(tmp), "%s.tmp", itr->name); + snprintf(tmp, sizeof(tmp), "%s.%i.%li.%li", itr->name, getpid(), + tv.tv_usec, tv.tv_sec); + tmp[NAME_MAX] = 0; fd = openat(dfd, tmp, flags, mode); if (fd < 0) { ERR("openat(%s, %s, %o, %o): %m\n", -- 2.19.2 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v2 2/3] depmod: prevent module dependency files corruption due to parallel invocation. 2018-12-10 21:29 ` [PATCH v2 2/3] depmod: prevent module dependency files corruption due to parallel invocation Michal Suchanek @ 2018-12-17 18:10 ` Lucas De Marchi 2018-12-17 22:07 ` Michal Suchánek 0 siblings, 1 reply; 14+ messages in thread From: Lucas De Marchi @ 2018-12-17 18:10 UTC (permalink / raw) To: Michal Suchanek; +Cc: linux-modules On Mon, Dec 10, 2018 at 2:03 PM Michal Suchanek <msuchanek@suse.de> wrote: > > Depmod does not use unique filename for temporary files. There is no > guarantee the user does not attempt to run mutiple depmod processes in > parallel. If that happens a temporary file might be created by > depmod(1st), truncated by depmod(2nd), and renamed to final name by > depmod(1st) resulting in corrupted file seen by user. > > Due to missing mkstempat() this is more complex than it should be. > Adding PID and timestamp to the filename should be reasonably reliable. > Adding O_EXCL as mkstemp does fails creating the file rather than > corrupting existing file. > > Signed-off-by: Michal Suchanek <msuchanek@suse.de> > --- > tools/depmod.c | 12 +++++++++--- > 1 file changed, 9 insertions(+), 3 deletions(-) > > diff --git a/tools/depmod.c b/tools/depmod.c > index 18c0d61b2db3..3b6d16e76160 100644 > --- a/tools/depmod.c > +++ b/tools/depmod.c > @@ -29,6 +29,7 @@ > #include <string.h> > #include <unistd.h> > #include <sys/stat.h> > +#include <sys/time.h> > #include <sys/utsname.h> > > #include <shared/array.h> > @@ -2398,6 +2399,9 @@ static int depmod_output(struct depmod *depmod, FILE *out) > }; > const char *dname = depmod->cfg->dirname; > int dfd, err = 0; > + struct timeval tv; > + > + gettimeofday(&tv, NULL); > > if (out != NULL) > dfd = -1; > @@ -2412,15 +2416,17 @@ static int depmod_output(struct depmod *depmod, FILE *out) > > for (itr = depfiles; itr->name != NULL; itr++) { > FILE *fp = out; > - char tmp[NAME_MAX] = ""; > + char tmp[NAME_MAX + 1] = ""; > int r, ferr; > > if (fp == NULL) { > - int flags = O_CREAT | O_TRUNC | O_WRONLY; > + int flags = O_CREAT | O_TRUNC | O_WRONLY | O_EXCL; > int mode = 0644; > int fd; > > - snprintf(tmp, sizeof(tmp), "%s.tmp", itr->name); > + snprintf(tmp, sizeof(tmp), "%s.%i.%li.%li", itr->name, getpid(), > + tv.tv_usec, tv.tv_sec); > + tmp[NAME_MAX] = 0; why? snprintf is guaranteed to nul-terminate the string. Lucas De Marchi > fd = openat(dfd, tmp, flags, mode); > if (fd < 0) { > ERR("openat(%s, %s, %o, %o): %m\n", > -- > 2.19.2 > -- Lucas De Marchi ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 2/3] depmod: prevent module dependency files corruption due to parallel invocation. 2018-12-17 18:10 ` Lucas De Marchi @ 2018-12-17 22:07 ` Michal Suchánek 2018-12-18 6:47 ` Yauheni Kaliuta 0 siblings, 1 reply; 14+ messages in thread From: Michal Suchánek @ 2018-12-17 22:07 UTC (permalink / raw) To: Lucas De Marchi; +Cc: linux-modules On Mon, 17 Dec 2018 10:10:37 -0800 Lucas De Marchi <lucas.de.marchi@gmail.com> wrote: > On Mon, Dec 10, 2018 at 2:03 PM Michal Suchanek <msuchanek@suse.de> wrote: > > > > Depmod does not use unique filename for temporary files. There is no > > guarantee the user does not attempt to run mutiple depmod processes in > > parallel. If that happens a temporary file might be created by > > depmod(1st), truncated by depmod(2nd), and renamed to final name by > > depmod(1st) resulting in corrupted file seen by user. > > > > Due to missing mkstempat() this is more complex than it should be. > > Adding PID and timestamp to the filename should be reasonably reliable. > > Adding O_EXCL as mkstemp does fails creating the file rather than > > corrupting existing file. > > > > Signed-off-by: Michal Suchanek <msuchanek@suse.de> > > --- > > tools/depmod.c | 12 +++++++++--- > > 1 file changed, 9 insertions(+), 3 deletions(-) > > > > diff --git a/tools/depmod.c b/tools/depmod.c > > index 18c0d61b2db3..3b6d16e76160 100644 > > --- a/tools/depmod.c > > +++ b/tools/depmod.c > > @@ -29,6 +29,7 @@ > > #include <string.h> > > #include <unistd.h> > > #include <sys/stat.h> > > +#include <sys/time.h> > > #include <sys/utsname.h> > > > > #include <shared/array.h> > > @@ -2398,6 +2399,9 @@ static int depmod_output(struct depmod *depmod, FILE *out) > > }; > > const char *dname = depmod->cfg->dirname; > > int dfd, err = 0; > > + struct timeval tv; > > + > > + gettimeofday(&tv, NULL); > > > > if (out != NULL) > > dfd = -1; > > @@ -2412,15 +2416,17 @@ static int depmod_output(struct depmod *depmod, FILE *out) > > > > for (itr = depfiles; itr->name != NULL; itr++) { > > FILE *fp = out; > > - char tmp[NAME_MAX] = ""; > > + char tmp[NAME_MAX + 1] = ""; > > int r, ferr; > > > > if (fp == NULL) { > > - int flags = O_CREAT | O_TRUNC | O_WRONLY; > > + int flags = O_CREAT | O_TRUNC | O_WRONLY | O_EXCL; > > int mode = 0644; > > int fd; > > > > - snprintf(tmp, sizeof(tmp), "%s.tmp", itr->name); > > + snprintf(tmp, sizeof(tmp), "%s.%i.%li.%li", itr->name, getpid(), > > + tv.tv_usec, tv.tv_sec); > > + tmp[NAME_MAX] = 0; > > why? snprintf is guaranteed to nul-terminate the string. > AFAIK it is guaranteed to not write after end of buffer. It is not guaranteed to terminate the string. To guarantee terminated string you need large enough buffer or a nul after the buffer. Or asprintf. Thanks Michal ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 2/3] depmod: prevent module dependency files corruption due to parallel invocation. 2018-12-17 22:07 ` Michal Suchánek @ 2018-12-18 6:47 ` Yauheni Kaliuta 2018-12-18 10:26 ` Michal Suchánek 0 siblings, 1 reply; 14+ messages in thread From: Yauheni Kaliuta @ 2018-12-18 6:47 UTC (permalink / raw) To: Michal Suchánek; +Cc: Lucas De Marchi, linux-modules Hi, Michal! >>>>> On Mon, 17 Dec 2018 23:07:43 +0100, Michal Suchánek wrote: > On Mon, 17 Dec 2018 10:10:37 -0800 > Lucas De Marchi <lucas.de.marchi@gmail.com> wrote: >> On Mon, Dec 10, 2018 at 2:03 PM Michal Suchanek <msuchanek@suse.de> wrote: >> > >> > Depmod does not use unique filename for temporary files. There is no >> > guarantee the user does not attempt to run mutiple depmod processes in >> > parallel. If that happens a temporary file might be created by >> > depmod(1st), truncated by depmod(2nd), and renamed to final name by >> > depmod(1st) resulting in corrupted file seen by user. >> > >> > Due to missing mkstempat() this is more complex than it should be. >> > Adding PID and timestamp to the filename should be reasonably reliable. >> > Adding O_EXCL as mkstemp does fails creating the file rather than >> > corrupting existing file. >> > >> > Signed-off-by: Michal Suchanek <msuchanek@suse.de> >> > --- >> > tools/depmod.c | 12 +++++++++--- >> > 1 file changed, 9 insertions(+), 3 deletions(-) >> > >> > diff --git a/tools/depmod.c b/tools/depmod.c >> > index 18c0d61b2db3..3b6d16e76160 100644 >> > --- a/tools/depmod.c >> > +++ b/tools/depmod.c >> > @@ -29,6 +29,7 @@ >> > #include <string.h> >> > #include <unistd.h> >> > #include <sys/stat.h> >> > +#include <sys/time.h> >> > #include <sys/utsname.h> >> > >> > #include <shared/array.h> >> > @@ -2398,6 +2399,9 @@ static int depmod_output(struct depmod *depmod, FILE *out) >> > }; >> > const char *dname = depmod->cfg->dirname; >> > int dfd, err = 0; >> > + struct timeval tv; >> > + >> > + gettimeofday(&tv, NULL); >> > >> > if (out != NULL) >> > dfd = -1; >> > @@ -2412,15 +2416,17 @@ static int depmod_output(struct depmod *depmod, FILE *out) >> > >> > for (itr = depfiles; itr->name != NULL; itr++) { >> > FILE *fp = out; >> > - char tmp[NAME_MAX] = ""; >> > + char tmp[NAME_MAX + 1] = ""; >> > int r, ferr; >> > >> > if (fp == NULL) { >> > - int flags = O_CREAT | O_TRUNC | O_WRONLY; >> > + int flags = O_CREAT | O_TRUNC | O_WRONLY | O_EXCL; >> > int mode = 0644; >> > int fd; >> > >> > - snprintf(tmp, sizeof(tmp), "%s.tmp", itr->name); >> > + snprintf(tmp, sizeof(tmp), "%s.%i.%li.%li", itr->name, getpid(), >> > + tv.tv_usec, tv.tv_sec); >> > + tmp[NAME_MAX] = 0; >> >> why? snprintf is guaranteed to nul-terminate the string. >> > AFAIK it is guaranteed to not write after end of buffer. It is > not guaranteed to terminate the string. To guarantee > terminated string you need large enough buffer or a nul after > the buffer. Or asprintf. Actually, it is. VS strncpy(). May be it is not so clear from the man page: ``` The functions snprintf() and vsnprintf() write at most size bytes (including the terminating null byte ('\0')) to str. ``` -- WBR, Yauheni Kaliuta ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 2/3] depmod: prevent module dependency files corruption due to parallel invocation. 2018-12-18 6:47 ` Yauheni Kaliuta @ 2018-12-18 10:26 ` Michal Suchánek 0 siblings, 0 replies; 14+ messages in thread From: Michal Suchánek @ 2018-12-18 10:26 UTC (permalink / raw) To: Yauheni Kaliuta; +Cc: Lucas De Marchi, linux-modules On Tue, 18 Dec 2018 08:47:58 +0200 Yauheni Kaliuta <yauheni.kaliuta@redhat.com> wrote: > Hi, Michal! > > >>>>> On Mon, 17 Dec 2018 23:07:43 +0100, Michal Suchánek wrote: > > > On Mon, 17 Dec 2018 10:10:37 -0800 > > Lucas De Marchi <lucas.de.marchi@gmail.com> wrote: > > >> On Mon, Dec 10, 2018 at 2:03 PM Michal Suchanek <msuchanek@suse.de> wrote: > >> > > >> > Depmod does not use unique filename for temporary files. There is no > >> > guarantee the user does not attempt to run mutiple depmod processes in > >> > parallel. If that happens a temporary file might be created by > >> > depmod(1st), truncated by depmod(2nd), and renamed to final name by > >> > depmod(1st) resulting in corrupted file seen by user. > >> > > >> > Due to missing mkstempat() this is more complex than it should be. > >> > Adding PID and timestamp to the filename should be reasonably reliable. > >> > Adding O_EXCL as mkstemp does fails creating the file rather than > >> > corrupting existing file. > >> > > >> > Signed-off-by: Michal Suchanek <msuchanek@suse.de> > >> > --- > >> > tools/depmod.c | 12 +++++++++--- > >> > 1 file changed, 9 insertions(+), 3 deletions(-) > >> > > >> > diff --git a/tools/depmod.c b/tools/depmod.c > >> > index 18c0d61b2db3..3b6d16e76160 100644 > >> > --- a/tools/depmod.c > >> > +++ b/tools/depmod.c > >> > @@ -29,6 +29,7 @@ > >> > #include <string.h> > >> > #include <unistd.h> > >> > #include <sys/stat.h> > >> > +#include <sys/time.h> > >> > #include <sys/utsname.h> > >> > > >> > #include <shared/array.h> > >> > @@ -2398,6 +2399,9 @@ static int depmod_output(struct depmod *depmod, FILE *out) > >> > }; > >> > const char *dname = depmod->cfg->dirname; > >> > int dfd, err = 0; > >> > + struct timeval tv; > >> > + > >> > + gettimeofday(&tv, NULL); > >> > > >> > if (out != NULL) > >> > dfd = -1; > >> > @@ -2412,15 +2416,17 @@ static int depmod_output(struct depmod *depmod, FILE *out) > >> > > >> > for (itr = depfiles; itr->name != NULL; itr++) { > >> > FILE *fp = out; > >> > - char tmp[NAME_MAX] = ""; > >> > + char tmp[NAME_MAX + 1] = ""; > >> > int r, ferr; > >> > > >> > if (fp == NULL) { > >> > - int flags = O_CREAT | O_TRUNC | O_WRONLY; > >> > + int flags = O_CREAT | O_TRUNC | O_WRONLY | O_EXCL; > >> > int mode = 0644; > >> > int fd; > >> > > >> > - snprintf(tmp, sizeof(tmp), "%s.tmp", itr->name); > >> > + snprintf(tmp, sizeof(tmp), "%s.%i.%li.%li", itr->name, getpid(), > >> > + tv.tv_usec, tv.tv_sec); > >> > + tmp[NAME_MAX] = 0; > >> > >> why? snprintf is guaranteed to nul-terminate the string. > >> > > AFAIK it is guaranteed to not write after end of buffer. It is > > not guaranteed to terminate the string. To guarantee > > terminated string you need large enough buffer or a nul after > > the buffer. Or asprintf. > > Actually, it is. VS strncpy(). May be it is not so clear from the > man page: > > ``` > The functions snprintf() and vsnprintf() write at most size > bytes (including the terminating null byte ('\0')) to str. > ``` > Yes, that's it. The POSIX page is much clearer: >> The snprintf() function shall be equivalent to sprintf(), with the >> addition of the n argument which states the size of the buffer referred >> to by s. If n is zero, nothing shall be written and s may be a null >> pointer. Otherwise, output bytes beyond the n‐1st shall be >> discarded instead of being written to the array, and a null byte is >> written at the end of the bytes actually written into the array. Thanks Michal ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 3/3] depmod: shut up gcc insufficinet buffer warning. 2018-12-10 21:29 ` [PATCH v2 0/3] Fix dependency file corruption with parallel depmod invocation Michal Suchanek 2018-12-10 21:29 ` [PATCH v2 1/3] depmod: prevent module dependency files missing during " Michal Suchanek 2018-12-10 21:29 ` [PATCH v2 2/3] depmod: prevent module dependency files corruption due to parallel invocation Michal Suchanek @ 2018-12-10 21:29 ` Michal Suchanek 2018-12-17 18:19 ` Lucas De Marchi 2 siblings, 1 reply; 14+ messages in thread From: Michal Suchanek @ 2018-12-10 21:29 UTC (permalink / raw) To: linux-modules; +Cc: Michal Suchanek In a couple of places depmod concatenates the module directory and filename with snprintf. This can technically overflow creating an unterminated string if module directory name is long. Use openat instead as is done elsewhere in depmod. This avoids the snprintf, the extra buffer on stack, and the gcc warning. It may even fix a corner case when the module direcotry name is just under PATH_MAX. Signed-off-by: Michal Suchanek <msuchanek@suse.de> --- tools/depmod.c | 51 ++++++++++++++++++++++++++++++++++---------------- 1 file changed, 35 insertions(+), 16 deletions(-) diff --git a/tools/depmod.c b/tools/depmod.c index 3b6d16e76160..fb76f823c016 100644 --- a/tools/depmod.c +++ b/tools/depmod.c @@ -1389,19 +1389,42 @@ static int depmod_modules_build_array(struct depmod *depmod) return 0; } +static FILE * dfdopen(const char * dname, const char * filename, int flags, const char * mode) + { + int fd, dfd; + FILE * ret; + + dfd = open(dname, O_RDONLY); + if (dfd < 0) { + WRN("could not open directory %s: %m\n", dname); + return NULL; + } + + fd = openat(dfd, filename, flags); + if (fd < 0) { + WRN("could not open %s at %s: %m\n", filename, dname); + ret = NULL; + } else { + ret = fdopen(fd, mode); + if (!ret) + WRN("could not associate stream with %s: %m\n", filename); + } + close(dfd); + return ret; +} + + + static void depmod_modules_sort(struct depmod *depmod) { - char order_file[PATH_MAX], line[PATH_MAX]; + char line[PATH_MAX]; + const char * order_file = "modules.order"; FILE *fp; unsigned idx = 0, total = 0; - snprintf(order_file, sizeof(order_file), "%s/modules.order", - depmod->cfg->dirname); - fp = fopen(order_file, "r"); - if (fp == NULL) { - WRN("could not open %s: %m\n", order_file); + fp = dfdopen(depmod->cfg->dirname, order_file, O_RDONLY, "r"); + if (fp == NULL) return; - } while (fgets(line, sizeof(line), fp) != NULL) { size_t len = strlen(line); @@ -1409,8 +1432,8 @@ static void depmod_modules_sort(struct depmod *depmod) if (len == 0) continue; if (line[len - 1] != '\n') { - ERR("%s:%u corrupted line misses '\\n'\n", - order_file, idx); + ERR("%s/%s:%u corrupted line misses '\\n'\n", + depmod->cfg->dirname, order_file, idx); goto corrupted; } } @@ -2287,18 +2310,14 @@ static int output_builtin_bin(struct depmod *depmod, FILE *out) { FILE *in; struct index_node *idx; - char infile[PATH_MAX], line[PATH_MAX], modname[PATH_MAX]; + char line[PATH_MAX], modname[PATH_MAX]; if (out == stdout) return 0; - snprintf(infile, sizeof(infile), "%s/modules.builtin", - depmod->cfg->dirname); - in = fopen(infile, "r"); - if (in == NULL) { - WRN("could not open %s: %m\n", infile); + in = dfdopen(depmod->cfg->dirname, "modules.builtin", O_RDONLY, "r"); + if (in == NULL) return 0; - } idx = index_create(); if (idx == NULL) { -- 2.19.2 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v2 3/3] depmod: shut up gcc insufficinet buffer warning. 2018-12-10 21:29 ` [PATCH v2 3/3] depmod: shut up gcc insufficinet buffer warning Michal Suchanek @ 2018-12-17 18:19 ` Lucas De Marchi 0 siblings, 0 replies; 14+ messages in thread From: Lucas De Marchi @ 2018-12-17 18:19 UTC (permalink / raw) To: Michal Suchanek; +Cc: linux-modules On Mon, Dec 10, 2018 at 2:03 PM Michal Suchanek <msuchanek@suse.de> wrote: > > In a couple of places depmod concatenates the module directory and filename > with snprintf. This can technically overflow creating an unterminated string if > module directory name is long. Use openat instead as is done elsewhere in > depmod. This avoids the snprintf, the extra buffer on stack, and the gcc > warning. It may even fix a corner case when the module direcotry name is just > under PATH_MAX. > > Signed-off-by: Michal Suchanek <msuchanek@suse.de> > --- > tools/depmod.c | 51 ++++++++++++++++++++++++++++++++++---------------- > 1 file changed, 35 insertions(+), 16 deletions(-) > > diff --git a/tools/depmod.c b/tools/depmod.c > index 3b6d16e76160..fb76f823c016 100644 > --- a/tools/depmod.c > +++ b/tools/depmod.c > @@ -1389,19 +1389,42 @@ static int depmod_modules_build_array(struct depmod *depmod) > return 0; > } > > +static FILE * dfdopen(const char * dname, const char * filename, int flags, const char * mode) coding style here is not consistent with the rest. We don't have space after "*". > + { and this is wrongly indented. Other than that this LGTM and I can fixup the styling issues while applying. Thanks. Lucas De Marchi > + int fd, dfd; > + FILE * ret; > + > + dfd = open(dname, O_RDONLY); > + if (dfd < 0) { > + WRN("could not open directory %s: %m\n", dname); > + return NULL; > + } > + > + fd = openat(dfd, filename, flags); > + if (fd < 0) { > + WRN("could not open %s at %s: %m\n", filename, dname); > + ret = NULL; > + } else { > + ret = fdopen(fd, mode); > + if (!ret) > + WRN("could not associate stream with %s: %m\n", filename); > + } > + close(dfd); > + return ret; > +} > + > + > + > static void depmod_modules_sort(struct depmod *depmod) > { > - char order_file[PATH_MAX], line[PATH_MAX]; > + char line[PATH_MAX]; > + const char * order_file = "modules.order"; > FILE *fp; > unsigned idx = 0, total = 0; > > - snprintf(order_file, sizeof(order_file), "%s/modules.order", > - depmod->cfg->dirname); > - fp = fopen(order_file, "r"); > - if (fp == NULL) { > - WRN("could not open %s: %m\n", order_file); > + fp = dfdopen(depmod->cfg->dirname, order_file, O_RDONLY, "r"); > + if (fp == NULL) > return; > - } > > while (fgets(line, sizeof(line), fp) != NULL) { > size_t len = strlen(line); > @@ -1409,8 +1432,8 @@ static void depmod_modules_sort(struct depmod *depmod) > if (len == 0) > continue; > if (line[len - 1] != '\n') { > - ERR("%s:%u corrupted line misses '\\n'\n", > - order_file, idx); > + ERR("%s/%s:%u corrupted line misses '\\n'\n", > + depmod->cfg->dirname, order_file, idx); > goto corrupted; > } > } > @@ -2287,18 +2310,14 @@ static int output_builtin_bin(struct depmod *depmod, FILE *out) > { > FILE *in; > struct index_node *idx; > - char infile[PATH_MAX], line[PATH_MAX], modname[PATH_MAX]; > + char line[PATH_MAX], modname[PATH_MAX]; > > if (out == stdout) > return 0; > > - snprintf(infile, sizeof(infile), "%s/modules.builtin", > - depmod->cfg->dirname); > - in = fopen(infile, "r"); > - if (in == NULL) { > - WRN("could not open %s: %m\n", infile); > + in = dfdopen(depmod->cfg->dirname, "modules.builtin", O_RDONLY, "r"); > + if (in == NULL) > return 0; > - } > > idx = index_create(); > if (idx == NULL) { > -- > 2.19.2 > -- Lucas De Marchi ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v3 0/3] Fix dependency file corruption with parallel depmod invocation 2018-12-07 15:45 [PATCH] depmod: Prevent module dependency files corruption due to parallel invocation Michal Suchanek 2018-12-10 21:29 ` [PATCH v2 0/3] Fix dependency file corruption with parallel depmod invocation Michal Suchanek @ 2018-12-17 22:46 ` Michal Suchanek 2018-12-17 22:46 ` [PATCH v3 1/3] depmod: prevent module dependency files missing during " Michal Suchanek ` (2 more replies) 1 sibling, 3 replies; 14+ messages in thread From: Michal Suchanek @ 2018-12-17 22:46 UTC (permalink / raw) To: linux-modules; +Cc: Lucas De Marchi, Jean Delvare, Michal Suchanek The files produced by depmod may be corrupted if multiple depmod processes running in parallel try to update them. Since there is nothing stopping the user running depmod in parallel this looks like a bug in depmod. v3: - remove superfluous whitespace, open flag, and duplicate nul termination v2: - split into separate patches, add gcc warning fix Michal Suchanek (3): depmod: prevent module dependency files missing during depmod invocation. depmod: prevent module dependency files corruption due to parallel invocation. depmod: shut up gcc insufficinet buffer warning. tools/depmod.c | 61 ++++++++++++++++++++++++++++++++++---------------- 1 file changed, 42 insertions(+), 19 deletions(-) -- 2.19.2 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v3 1/3] depmod: prevent module dependency files missing during depmod invocation. 2018-12-17 22:46 ` [PATCH v3 0/3] Fix dependency file corruption with parallel depmod invocation Michal Suchanek @ 2018-12-17 22:46 ` Michal Suchanek 2018-12-17 22:46 ` [PATCH v3 2/3] depmod: prevent module dependency files corruption due to parallel invocation Michal Suchanek 2018-12-17 22:46 ` [PATCH v3 3/3] depmod: shut up gcc insufficinet buffer warning Michal Suchanek 2 siblings, 0 replies; 14+ messages in thread From: Michal Suchanek @ 2018-12-17 22:46 UTC (permalink / raw) To: linux-modules; +Cc: Lucas De Marchi, Jean Delvare, Michal Suchanek Depmod deletes the module dependency files before moving the temporary files in their place. This results in user seeing no dependency files while they are updated. Remove the unlink call. The rename call should suffice to move the new file in place and unlink the old one. It should also do both atomically so there is no window when no dependency file exists. Signed-off-by: Michal Suchanek <msuchanek@suse.de> --- tools/depmod.c | 1 - 1 file changed, 1 deletion(-) diff --git a/tools/depmod.c b/tools/depmod.c index 989d9077926c..18c0d61b2db3 100644 --- a/tools/depmod.c +++ b/tools/depmod.c @@ -2451,7 +2451,6 @@ static int depmod_output(struct depmod *depmod, FILE *out) break; } - unlinkat(dfd, itr->name, 0); if (renameat(dfd, tmp, dfd, itr->name) != 0) { err = -errno; CRIT("renameat(%s, %s, %s, %s): %m\n", -- 2.19.2 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v3 2/3] depmod: prevent module dependency files corruption due to parallel invocation. 2018-12-17 22:46 ` [PATCH v3 0/3] Fix dependency file corruption with parallel depmod invocation Michal Suchanek 2018-12-17 22:46 ` [PATCH v3 1/3] depmod: prevent module dependency files missing during " Michal Suchanek @ 2018-12-17 22:46 ` Michal Suchanek 2018-12-17 22:46 ` [PATCH v3 3/3] depmod: shut up gcc insufficinet buffer warning Michal Suchanek 2 siblings, 0 replies; 14+ messages in thread From: Michal Suchanek @ 2018-12-17 22:46 UTC (permalink / raw) To: linux-modules; +Cc: Lucas De Marchi, Jean Delvare, Michal Suchanek Depmod does not use unique filename for temporary files. There is no guarantee the user does not attempt to run mutiple depmod processes in parallel. If that happens a temporary file might be created by depmod(1st), truncated by depmod(2nd), and renamed to final name by depmod(1st) resulting in corrupted file seen by user. Due to missing mkstempat() this is more complex than it should be. Adding PID and timestamp to the filename should be reasonably reliable. Adding O_EXCL as mkstemp does fails creating the file rather than corrupting existing file. Signed-off-by: Michal Suchanek <msuchanek@suse.de> --- v3: - remove superfluous code to terminate the string - snprintf should do it - remove superfluous O_TRUNC - not needed with O_EXCL --- tools/depmod.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/tools/depmod.c b/tools/depmod.c index 18c0d61b2db3..0f7e33ccfd59 100644 --- a/tools/depmod.c +++ b/tools/depmod.c @@ -29,6 +29,7 @@ #include <string.h> #include <unistd.h> #include <sys/stat.h> +#include <sys/time.h> #include <sys/utsname.h> #include <shared/array.h> @@ -2398,6 +2399,9 @@ static int depmod_output(struct depmod *depmod, FILE *out) }; const char *dname = depmod->cfg->dirname; int dfd, err = 0; + struct timeval tv; + + gettimeofday(&tv, NULL); if (out != NULL) dfd = -1; @@ -2416,11 +2420,12 @@ static int depmod_output(struct depmod *depmod, FILE *out) int r, ferr; if (fp == NULL) { - int flags = O_CREAT | O_TRUNC | O_WRONLY; + int flags = O_CREAT | O_EXCL | O_WRONLY; int mode = 0644; int fd; - snprintf(tmp, sizeof(tmp), "%s.tmp", itr->name); + snprintf(tmp, sizeof(tmp), "%s.%i.%li.%li", itr->name, getpid(), + tv.tv_usec, tv.tv_sec); fd = openat(dfd, tmp, flags, mode); if (fd < 0) { ERR("openat(%s, %s, %o, %o): %m\n", -- 2.19.2 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v3 3/3] depmod: shut up gcc insufficinet buffer warning. 2018-12-17 22:46 ` [PATCH v3 0/3] Fix dependency file corruption with parallel depmod invocation Michal Suchanek 2018-12-17 22:46 ` [PATCH v3 1/3] depmod: prevent module dependency files missing during " Michal Suchanek 2018-12-17 22:46 ` [PATCH v3 2/3] depmod: prevent module dependency files corruption due to parallel invocation Michal Suchanek @ 2018-12-17 22:46 ` Michal Suchanek 2 siblings, 0 replies; 14+ messages in thread From: Michal Suchanek @ 2018-12-17 22:46 UTC (permalink / raw) To: linux-modules; +Cc: Lucas De Marchi, Jean Delvare, Michal Suchanek In a couple of places depmod concatenates the module directory and filename with snprintf. This can technically overflow creating wrong path if module directory name is long. Use openat instead as is done elsewhere in depmod. This avoids the snprintf, the extra buffer on stack, and the gcc warning. Signed-off-by: Michal Suchanek <msuchanek@suse.de> --- v3: - remove superfluous whitespace --- tools/depmod.c | 51 ++++++++++++++++++++++++++++++++++---------------- 1 file changed, 35 insertions(+), 16 deletions(-) diff --git a/tools/depmod.c b/tools/depmod.c index 0f7e33ccfd59..0cf845a0dbc7 100644 --- a/tools/depmod.c +++ b/tools/depmod.c @@ -1389,19 +1389,42 @@ static int depmod_modules_build_array(struct depmod *depmod) return 0; } +static FILE *dfdopen(const char *dname, const char *filename, int flags, const char *mode) +{ + int fd, dfd; + FILE *ret; + + dfd = open(dname, O_RDONLY); + if (dfd < 0) { + WRN("could not open directory %s: %m\n", dname); + return NULL; + } + + fd = openat(dfd, filename, flags); + if (fd < 0) { + WRN("could not open %s at %s: %m\n", filename, dname); + ret = NULL; + } else { + ret = fdopen(fd, mode); + if (!ret) + WRN("could not associate stream with %s: %m\n", filename); + } + close(dfd); + return ret; +} + + + static void depmod_modules_sort(struct depmod *depmod) { - char order_file[PATH_MAX], line[PATH_MAX]; + char line[PATH_MAX]; + const char * order_file = "modules.order"; FILE *fp; unsigned idx = 0, total = 0; - snprintf(order_file, sizeof(order_file), "%s/modules.order", - depmod->cfg->dirname); - fp = fopen(order_file, "r"); - if (fp == NULL) { - WRN("could not open %s: %m\n", order_file); + fp = dfdopen(depmod->cfg->dirname, order_file, O_RDONLY, "r"); + if (fp == NULL) return; - } while (fgets(line, sizeof(line), fp) != NULL) { size_t len = strlen(line); @@ -1409,8 +1432,8 @@ static void depmod_modules_sort(struct depmod *depmod) if (len == 0) continue; if (line[len - 1] != '\n') { - ERR("%s:%u corrupted line misses '\\n'\n", - order_file, idx); + ERR("%s/%s:%u corrupted line misses '\\n'\n", + depmod->cfg->dirname, order_file, idx); goto corrupted; } } @@ -2287,18 +2310,14 @@ static int output_builtin_bin(struct depmod *depmod, FILE *out) { FILE *in; struct index_node *idx; - char infile[PATH_MAX], line[PATH_MAX], modname[PATH_MAX]; + char line[PATH_MAX], modname[PATH_MAX]; if (out == stdout) return 0; - snprintf(infile, sizeof(infile), "%s/modules.builtin", - depmod->cfg->dirname); - in = fopen(infile, "r"); - if (in == NULL) { - WRN("could not open %s: %m\n", infile); + in = dfdopen(depmod->cfg->dirname, "modules.builtin", O_RDONLY, "r"); + if (in == NULL) return 0; - } idx = index_create(); if (idx == NULL) { -- 2.19.2 ^ permalink raw reply related [flat|nested] 14+ messages in thread
end of thread, other threads:[~2018-12-18 10:26 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-12-07 15:45 [PATCH] depmod: Prevent module dependency files corruption due to parallel invocation Michal Suchanek 2018-12-10 21:29 ` [PATCH v2 0/3] Fix dependency file corruption with parallel depmod invocation Michal Suchanek 2018-12-10 21:29 ` [PATCH v2 1/3] depmod: prevent module dependency files missing during " Michal Suchanek 2018-12-10 21:29 ` [PATCH v2 2/3] depmod: prevent module dependency files corruption due to parallel invocation Michal Suchanek 2018-12-17 18:10 ` Lucas De Marchi 2018-12-17 22:07 ` Michal Suchánek 2018-12-18 6:47 ` Yauheni Kaliuta 2018-12-18 10:26 ` Michal Suchánek 2018-12-10 21:29 ` [PATCH v2 3/3] depmod: shut up gcc insufficinet buffer warning Michal Suchanek 2018-12-17 18:19 ` Lucas De Marchi 2018-12-17 22:46 ` [PATCH v3 0/3] Fix dependency file corruption with parallel depmod invocation Michal Suchanek 2018-12-17 22:46 ` [PATCH v3 1/3] depmod: prevent module dependency files missing during " Michal Suchanek 2018-12-17 22:46 ` [PATCH v3 2/3] depmod: prevent module dependency files corruption due to parallel invocation Michal Suchanek 2018-12-17 22:46 ` [PATCH v3 3/3] depmod: shut up gcc insufficinet buffer warning Michal Suchanek
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).