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 PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Tue, 19 Oct 2021 12:30:28 -0500 From: "Zanussi, Tom" <tom.zanussi@...ux.intel.com> To: Steven Rostedt <rostedt@...dmis.org>, Yang Jihong <yangjihong1@...wei.com> Cc: linux-kernel <linux-kernel@...r.kernel.org> Subject: Re: [QUESTION] Performance deterioration caused by commit 85f726a35e504418 Hi Steve, On 10/18/2021 9:51 PM, Steven Rostedt wrote: > On Tue, 19 Oct 2021 10:39:47 +0800 > Yang Jihong <yangjihong1@...wei.com> wrote: > >> Hi Steve, >> >> On 2021/10/18 21:37, Steven Rostedt wrote: >>> On Mon, 18 Oct 2021 11:23:14 +0800 >>> Yang Jihong <yangjihong1@...wei.com> wrote: >>> >>>> Hi Tom and Steven, >>>> >>>> commit 85f726a35e504418 use strncpy instead of memcpy when copying comm, >>>> on ARM64 machine, this commit causes performance degradation. >>>> >>>> I test the number of instructions executed by invoking the >>>> trace_sched_switch function once on an arm64 machine: >>>> 1. Use memcpy, the number of instructions executed is 850. >>>> 2. Use strncpy, the number of instructions executed 1100. >>>> That is, use strncpy is almost 250 more instructions than memcpy. >>>> >>>> Has the impact on performance been considered in this commit? :) >>>> What is the impact of revert the patch? >>>> >>> >>> It's a security issue. And like everything security, there's always going >>> to be a performance impact. Look at the performance impact due to spectre >>> and meltdown! >>> >>> That said, although memcpy() may not be used, we don't need strncpy. >>> strncpy() will pad the rest of the string with nul bytes. But since the >>> memory the string is being recorded into is already initialized (or can be >>> if it isn't), we could use the faster strlcpy(). >>> >>> Have you tried testing it by switching strncpy() with strlcpy()? >>> >> I have tried testing it by switching strncpy() with strlcpy(), there is >> no performance improvement, probably because the strlen function is >> called in strlpy and the string is traversed each time. > > Then there's not much we can do. Security trumps performance. Not to > mention, the garbage in the comm after the '\0' causes the histograms to > produce strange results. > > Now for the saved_cmdlines, since it isn't exported directly to user space, > that one may be put back to memcpy(). > > Tom, was there a reason to change saved_cmdlines(), as I'm not sure that is > leaked. It looks like it is printed with the normal seq_printf() in > saved_cmdlines_show(). > I don't think either of the changes in commit 85f726a35e504418 are directly related to the original problem [1] and therefore changing them back to memcpy or whatever shouldn't affect the histograms since that data is never used in keys. Commit 85f726a35e504418 was basically a follow-on to commit 9f0bbf3115ca (tracing: Use strncpy instead of memcpy for string keys in hist triggers) and was added for completeness after examining other uses of memcpy in the tracing code (there's even a comment in there from you about possible performance hits from changing it ;-) So anyway, as far as the histograms go, I think optimizing the two changes in 85f726a35e504418 while ignoring trailing garbage can be done without affecting the histogram correctness. Tom [1] https://lore.kernel.org/all/50c35ae1267d64eee975b8125e151e600071d4dc.1549309756.git.tom.zanussi@linux.intel.com/ > And it doesn't even look like the saved_cmdlines() is even initialized to > zero, so it itself could leak memory if it was exposed. > > -- Steve >
Powered by blists - more mailing lists