On Mon, Aug 17, 2020 at 04:24:29PM +0100, Chris Down wrote: > Applications which manipulate MSRs from userspace often do so > infrequently, and all at once. As such, the default printk ratelimit > architecture supplied by pr_err_ratelimited doesn't do enough to prevent > kmsg becoming completely overwhelmed with their messages and pushing > other salient information out of the circular buffer. In one case, I saw > over 80% of kmsg being filled with these messages, and the default kmsg > buffer being completely filled less than 5 minutes after boot(!). > > This change makes things much less aggressive, while still achieving the
s/This change makes/Make/ > original goal of fiter_write(). Operators will still get warnings that > MSRs are being manipulated from userspace, but they won't have other > also potentially useful messages pushed out of the kmsg buffer. > > Of course, one can boot with `allow_writes=1` to avoid these messages at > all, but that then has the downfall that one doesn't get _any_ > notification at all about these problems in the first place, and so is > much less likely to forget to fix it. One might rather it was less > binary: it was still logged, just less often, so that application > developers _do_ have the incentive to improve their current methods, > without us having to push other useful stuff out of the kmsg buffer. > > This one example isn't the point, of course: I'm sure there are plenty > of other non-ideal-but-pragmatic cases where people are writing to MSRs > from userspace right now, and it will take time for those people to find > other solutions. > > Overall, this patch keeps the intent of the original patch, while Avoid having "This patch" or "This commit" in the commit message. It is tautologically useless. Also, do $ git grep 'This patch' Documentation/process for more details. > mitigating its sometimes heavy effects on kmsg composition. > > Signed-off-by: Chris Down <[email protected]> > Cc: Borislav Petkov <[email protected]> > Cc: Jakub Kicinski <[email protected]> > --- > arch/x86/kernel/msr.c | 17 +++++++++++++---- > 1 file changed, 13 insertions(+), 4 deletions(-) > > diff --git a/arch/x86/kernel/msr.c b/arch/x86/kernel/msr.c > index 49dcfb85e773..3babb0e58b0e 100644 > --- a/arch/x86/kernel/msr.c > +++ b/arch/x86/kernel/msr.c > @@ -80,18 +80,27 @@ static ssize_t msr_read(struct file *file, char __user > *buf, > > static int filter_write(u32 reg) > { > + /* > + * Banging MSRs usually happens all at once, and can easily saturate Yeah we have a lot of non-native speakers so "Banging" might be confusing - please formulate less colloquial. > + * kmsg. Only allow 1 MSR message every 30 seconds. > + * > + * We could be smarter here and do it per-MSR, or whatever, but it would Please avoid the "we" in text as it is ambiguous - try formulating passively like your commit message. > + * certainly be more complex, and this is enough at least to stop > + * completely saturating the ring buffer. > + */ > + static DEFINE_RATELIMIT_STATE(fw_rs, 30 * HZ, 1); > + > switch (allow_writes) { > case MSR_WRITES_ON: return 0; > case MSR_WRITES_OFF: return -EPERM; > default: break; > } > > - if (reg == MSR_IA32_ENERGY_PERF_BIAS) > + if (!__ratelimit(&fw_rs) || reg == MSR_IA32_ENERGY_PERF_BIAS) > return 0; Let's keep those two tests separate: if (!__ratelimit(&fw_rs)) return 0; if (reg == MSR_IA32_ENERGY_PERF_BIAS) return 0; > > - pr_err_ratelimited("Write to unrecognized MSR 0x%x by %s\n" > - "Please report to [email protected]\n", > - reg, current->comm); > + pr_err("Write to unrecognized MSR 0x%x by %s\n" > + "Please report to [email protected]\n", reg, current->comm); > > return 0; > } > -- > 2.28.0 > -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette
