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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 4 Jun 2015 11:45:42 +0200
From:	Milos Vyletel <milos@...hat.com>
To:	Ingo Molnar <mingo@...nel.org>
Cc:	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	Paul Mackerras <paulus@...ba.org>,
	Ingo Molnar <mingo@...hat.com>,
	Arnaldo Carvalho de Melo <acme@...nel.org>,
	Jiri Olsa <jolsa@...nel.org>, Don Zickus <dzickus@...hat.com>,
	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
	"Steven Rostedt (Red Hat)" <rostedt@...dmis.org>,
	"open list:PERFORMANCE EVENT..." <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] perf tools: avoid possible race condition in copyfile

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

> 
> * Milos Vyletel <milos@...hat.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@...hat.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@...nel.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/