[PATCH] perf tools: avoid possible race condition in copyfile

[PATCH] perf tools: avoid possible race condition in copyfile

[Milos Vyletel]

Use unique temporary files when copying to buildid dir to prevent races
in case multiple instances are trying to copy same file. This is done by

- creating template in form <path>/.<filename>.XXXXXX where the suffix is
  used by mkstemp() to create unique file
- change file mode
- copy content
- if successful link temp file to target file
- unlink temp file

At this point the only file left at target path should be the desired
one either created by us or other instance if we raced. This should also
prevent not yet fully copied files to be visible to to other perf
instances that could try to parse them.

On top of that slow_copyfile no longer needs to deal with file mode when
creating file since temporary file is already created and mode is set.

Signed-off-by: Milos Vyletel <milos@redhat.com>
---
 tools/perf/util/util.c | 48 ++++++++++++++++++++++++++++++++----------------
 1 file changed, 32 insertions(+), 16 deletions(-)

diff --git a/tools/perf/util/util.c b/tools/perf/util/util.c
index 4ee6d0d..fec1e13 100644
--- a/tools/perf/util/util.c
+++ b/tools/perf/util/util.c
@@ -72,20 +72,17 @@ int mkdir_p(char *path, mode_t mode)
 	return (stat(path, &st) && mkdir(path, mode)) ? -1 : 0;
 }
 
-static int slow_copyfile(const char *from, const char *to, mode_t mode)
+static int slow_copyfile(const char *from, const char *to)
 {
 	int err = -1;
 	char *line = NULL;
 	size_t n;
 	FILE *from_fp = fopen(from, "r"), *to_fp;
-	mode_t old_umask;
 
 	if (from_fp == NULL)
 		goto out;
 
-	old_umask = umask(mode ^ 0777);
 	to_fp = fopen(to, "w");
-	umask(old_umask);
 	if (to_fp == NULL)
 		goto out_fclose_from;
 
@@ -108,36 +105,55 @@ int copyfile_mode(const char *from, const char *to, mode_t mode)
 	struct stat st;
 	void *addr;
 	int err = -1;
+	char *tmp = NULL, *ptr = NULL;
 
 	if (stat(from, &st))
 		goto out;
 
-	if (st.st_size == 0) /* /proc? do it slowly... */
-		return slow_copyfile(from, to, mode);
-
-	fromfd = open(from, O_RDONLY);
-	if (fromfd < 0)
+	/* extra 'x' at the end is to reserve space for '.' */
+	if (asprintf(&tmp, "%s.XXXXXXx", to) < 0) {
+		tmp = NULL;
 		goto out;
+	}
+	ptr = strrchr(tmp, '/');
+	if (!ptr)
+		goto out;
+	ptr = memmove(ptr + 1, ptr, strlen(ptr) - 1);
+	*ptr = '.';
 
-	tofd = creat(to, mode);
+	tofd = mkstemp(tmp);
 	if (tofd < 0)
-		goto out_close_from;
+		goto out;
+
+	if (fchmod(tofd, mode))
+		goto out_close_to;
+
+	if (st.st_size == 0) { /* /proc? do it slowly... */
+		err = slow_copyfile(from, tmp);
+		goto out_close_to;
+	}
+
+	fromfd = open(from, O_RDONLY);
+	if (fromfd < 0)
+		goto out_close_to;
 
 	addr = mmap(NULL, st.st_size, PROT_READ, MAP_PRIVATE, fromfd, 0);
 	if (addr == MAP_FAILED)
-		goto out_close_to;
+		goto out_close_from;
 
 	if (write(tofd, addr, st.st_size) == st.st_size)
 		err = 0;
 
 	munmap(addr, st.st_size);
-out_close_to:
-	close(tofd);
-	if (err)
-		unlink(to);
 out_close_from:
 	close(fromfd);
+out_close_to:
+	close(tofd);
+	if (!err)
+		err = link(tmp, to);
+	unlink(tmp);
 out:
+	free(tmp);
 	return err;
 }
 
-- 
2.4.2

Re: [PATCH] perf tools: avoid possible race condition in copyfile

[Ingo Molnar]

* Milos Vyletel <milos@redhat.com> wrote:

> Use unique temporary files when copying to buildid dir to prevent races
> in case multiple instances are trying to copy same file. This is done by
> 
> - creating template in form <path>/.<filename>.XXXXXX where the suffix is
>   used by mkstemp() to create unique file
> - change file mode
> - copy content
> - if successful link temp file to target file
> - unlink temp file
> 
> At this point the only file left at target path should be the desired
> one either created by us or other instance if we raced. This should also
> prevent not yet fully copied files to be visible to to other perf
> instances that could try to parse them.
> 
> On top of that slow_copyfile no longer needs to deal with file mode when
> creating file since temporary file is already created and mode is set.
> 
> Signed-off-by: Milos Vyletel <milos@redhat.com>

Ok, that looks nice!

Assuming it passes testing you can add my ack to it:

  Acked-by: Ingo Molnar <mingo@kernel.org>

Is there any other place in tools/perf where we are using file locking or racy 
shared access to the same file(s)?

Thanks,

	Ingo

Re: [PATCH] perf tools: avoid possible race condition in copyfile

[Milos Vyletel]

On Thu, Jun 04, 2015 at 11:11:00AM +0200, Ingo Molnar wrote:

> 
> * Milos Vyletel <milos@redhat.com> wrote:
> 
> > Use unique temporary files when copying to buildid dir to prevent races
> > in case multiple instances are trying to copy same file. This is done by
> > 
> > - creating template in form <path>/.<filename>.XXXXXX where the suffix is
> >   used by mkstemp() to create unique file
> > - change file mode
> > - copy content
> > - if successful link temp file to target file
> > - unlink temp file
> > 
> > At this point the only file left at target path should be the desired
> > one either created by us or other instance if we raced. This should also
> > prevent not yet fully copied files to be visible to to other perf
> > instances that could try to parse them.
> > 
> > On top of that slow_copyfile no longer needs to deal with file mode when
> > creating file since temporary file is already created and mode is set.
> > 
> > Signed-off-by: Milos Vyletel <milos@redhat.com>
> 
> Ok, that looks nice!

Thanks.

> 
> Assuming it passes testing you can add my ack to it:
> 

I did some testing myself by running perf record, archive and reading
the data on other system as well as running perf buildid-cache on perf
binary itself. I did revert fix from 0635b0f that to expose the race
with EEXIST and my recreator test passed sucessfully. I've also added
debug printfs in the code to really make sure the temporary files are
created, exist and then are moved back.
Everything worked as expected but this can surely use some more testing.

>   Acked-by: Ingo Molnar <mingo@kernel.org>
> 
> Is there any other place in tools/perf where we are using file locking or racy 
> shared access to the same file(s)?
> 

There are two places in code I'm aware of which copy files and the both
end up calling copyfile_mode that this patch modifies. Those places are

build_id_cache__add_kcore
  kcore_copy
    kcore_copy__copy_file
      copyfile_mode

build_id_cache__add_s
  copyfile
    copyfile_mode

unless I'm missing something we should cover all cases.

Milos

Re: [PATCH] perf tools: avoid possible race condition in copyfile

[Arnaldo Carvalho de Melo]

Em Thu, Jun 04, 2015 at 11:11:00AM +0200, Ingo Molnar escreveu:
> * Milos Vyletel <milos@redhat.com> wrote:
> > On top of that slow_copyfile no longer needs to deal with file mode when
> > creating file since temporary file is already created and mode is set.
> > 
> > Signed-off-by: Milos Vyletel <milos@redhat.com>
> 
> Ok, that looks nice!
> 
> Assuming it passes testing you can add my ack to it:
> 
>   Acked-by: Ingo Molnar <mingo@kernel.org>

Can you please try applying it on tip/perf/core instead? I think it is
clashing with:

commit 9c9f5a2f1944e8b6bf2b618d04b31e1c1760637e
Author: Namhyung Kim <namhyung@kernel.org>
Date:   Mon May 18 09:30:18 2015 +0900

    perf tools: Introduce copyfile_offset() function

- Arnaldo

Re: [PATCH] perf tools: avoid possible race condition in copyfile

[Milos Vyletel]

On Mon, Jun 08, 2015 at 10:39:57AM -0300, Arnaldo Carvalho de Melo wrote:

> Em Thu, Jun 04, 2015 at 11:11:00AM +0200, Ingo Molnar escreveu:
> > * Milos Vyletel <milos@redhat.com> wrote:
> > > On top of that slow_copyfile no longer needs to deal with file mode when
> > > creating file since temporary file is already created and mode is set.
> > > 
> > > Signed-off-by: Milos Vyletel <milos@redhat.com>
> > 
> > Ok, that looks nice!
> > 
> > Assuming it passes testing you can add my ack to it:
> > 
> >   Acked-by: Ingo Molnar <mingo@kernel.org>
> 
> Can you please try applying it on tip/perf/core instead? I think it is
> clashing with:
> 
> commit 9c9f5a2f1944e8b6bf2b618d04b31e1c1760637e
> Author: Namhyung Kim <namhyung@kernel.org>
> Date:   Mon May 18 09:30:18 2015 +0900
> 
>     perf tools: Introduce copyfile_offset() function
> 

And also with 

commit 0b1de0be1eac7b23e89cb43c17b02d38ead6b6c8
Author: Namhyung Kim <namhyung@kernel.org>
Date:   Mon May 18 09:30:17 2015 +0900

    perf tools: Add rm_rf() utility function

Will resend version based on tip/perf/core after I rerun my testing.

Milos

Re: [PATCH] perf tools: avoid possible race condition in copyfile

[Arnaldo Carvalho de Melo]

Em Mon, Jun 08, 2015 at 03:58:43PM +0200, Milos Vyletel escreveu:
> On Mon, Jun 08, 2015 at 10:39:57AM -0300, Arnaldo Carvalho de Melo wrote:
> > Can you please try applying it on tip/perf/core instead? I think it is
> > clashing with:

> > commit 9c9f5a2f1944e8b6bf2b618d04b31e1c1760637e
> > Author: Namhyung Kim <namhyung@kernel.org>
> > Date:   Mon May 18 09:30:18 2015 +0900

> >     perf tools: Introduce copyfile_offset() function

> And also with 
 
> commit 0b1de0be1eac7b23e89cb43c17b02d38ead6b6c8
> Author: Namhyung Kim <namhyung@kernel.org>
> Date:   Mon May 18 09:30:17 2015 +0900
 
>     perf tools: Add rm_rf() utility function
 
> Will resend version based on tip/perf/core after I rerun my testing.

Thanks a lot!

- Arnaldo