All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] x86/umip: Add a umip= cmdline switch
@ 2021-09-07 20:04 Borislav Petkov
  2021-09-11  1:14 ` Ricardo Neri
                   ` (3 more replies)
  0 siblings, 4 replies; 16+ messages in thread
From: Borislav Petkov @ 2021-09-07 20:04 UTC (permalink / raw)
  To: X86 ML; +Cc: Ricardo Neri, LKML, Marcus Rückert

From: Borislav Petkov <bp@suse.de>

And add the first control option

  umip=warnings_off

which disables warnings resulting from emulating UMIP-enabled insns.

The actual use case is for users playing games in wine, games like
Overwatch, for example, which go nuts with SIDT:

  [Di Sep  7 00:24:05 2021] umip_printk: 1345 callbacks suppressed
  [Di Sep  7 00:24:05 2021] umip: someapp.exe[29231] ip:14064cdba sp:11b7c0: SIDT instruction cannot be used by applications.
  [Di Sep  7 00:24:05 2021] umip: someapp.exe[29231] ip:14064cdba sp:11b7c0: For now, expensive software emulation returns the result.
  ...
  [Di Sep  7 00:26:06 2021] umip_printk: 2227 callbacks suppressed
  [Di Sep  7 00:26:06 2021] umip: someapp.exe[29231] ip:14064cdba sp:11b940: SIDT instruction cannot be used by applications.

filling up the kernel log unnecessarily with the same info over and over
again which doesn't mean a whit to the users - they just wanna play.

So add a boot-time control switch for those warning messages.

Reported-by: Marcus Rückert <mrueckert@suse.com>
Signed-off-by: Borislav Petkov <bp@suse.de>
---
 .../admin-guide/kernel-parameters.txt         |  5 +++
 arch/x86/kernel/umip.c                        | 33 +++++++++++++++++--
 2 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 828d11441ebf..815d022c3e87 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -5887,6 +5887,11 @@
 	unknown_nmi_panic
 			[X86] Cause panic on unknown NMI.
 
+	umip=warnings_off
+			[X86]
+			* warnings_off - do not issue warnings when emulating
+			  UMIP-enabled instructions.
+
 	usbcore.authorized_default=
 			[USB] Default USB device authorization:
 			(default -1 = authorized except for wireless USB,
diff --git a/arch/x86/kernel/umip.c b/arch/x86/kernel/umip.c
index 576b47e7523d..1d37dc626011 100644
--- a/arch/x86/kernel/umip.c
+++ b/arch/x86/kernel/umip.c
@@ -90,10 +90,19 @@ static const char * const umip_insns[5] = {
 	[UMIP_INST_STR] = "STR",
 };
 
-#define umip_pr_err(regs, fmt, ...) \
+static struct umip_config {
+	__u64 warnings_off	: 1,
+	      __reserved	: 63;
+} umip_cfg;
+
+#define umip_pr_err(regs, fmt, ...)				\
 	umip_printk(regs, KERN_ERR, fmt, ##__VA_ARGS__)
-#define umip_pr_warn(regs, fmt, ...) \
-	umip_printk(regs, KERN_WARNING, fmt,  ##__VA_ARGS__)
+
+#define umip_pr_warn(regs, fmt, ...)					\
+({									\
+	if (!umip_cfg.warnings_off)					\
+		umip_printk(regs, KERN_WARNING, fmt,  ##__VA_ARGS__);	\
+})
 
 /**
  * umip_printk() - Print a rate-limited message
@@ -407,5 +416,23 @@ bool fixup_umip_exception(struct pt_regs *regs)
 
 	/* increase IP to let the program keep going */
 	regs->ip += insn.length;
+
 	return true;
 }
+
+static int __init parse_umip_param(char *str)
+{
+	if (!str)
+		return 0;
+
+	if (*str == '=')
+		str++;
+
+	if (!strcmp(str, "warnings_off"))
+		umip_cfg.warnings_off = 1;
+	else
+		return 0;
+
+	return 1;
+}
+__setup("umip", parse_umip_param);
-- 
2.29.2


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] x86/umip: Add a umip= cmdline switch
  2021-09-07 20:04 [PATCH] x86/umip: Add a umip= cmdline switch Borislav Petkov
@ 2021-09-11  1:14 ` Ricardo Neri
  2021-09-11  9:20   ` Borislav Petkov
  2021-09-13 21:45 ` [PATCH] x86/umip: Add a umip= cmdline switch Ricardo Neri
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 16+ messages in thread
From: Ricardo Neri @ 2021-09-11  1:14 UTC (permalink / raw)
  To: Borislav Petkov; +Cc: X86 ML, LKML, Marcus Rückert

On Tue, Sep 07, 2021 at 10:04:54PM +0200, Borislav Petkov wrote:
> From: Borislav Petkov <bp@suse.de>
> 
> And add the first control option
> 
>   umip=warnings_off
> 
> which disables warnings resulting from emulating UMIP-enabled insns.
> 
> The actual use case is for users playing games in wine, games like
> Overwatch, for example, which go nuts with SIDT:
> 
>   [Di Sep  7 00:24:05 2021] umip_printk: 1345 callbacks suppressed
>   [Di Sep  7 00:24:05 2021] umip: someapp.exe[29231] ip:14064cdba sp:11b7c0: SIDT instruction cannot be used by applications.
>   [Di Sep  7 00:24:05 2021] umip: someapp.exe[29231] ip:14064cdba sp:11b7c0: For now, expensive software emulation returns the result.
>   ...
>   [Di Sep  7 00:26:06 2021] umip_printk: 2227 callbacks suppressed
>   [Di Sep  7 00:26:06 2021] umip: someapp.exe[29231] ip:14064cdba sp:11b940: SIDT instruction cannot be used by applications.
> 
> filling up the kernel log unnecessarily with the same info over and over
> again which doesn't mean a whit to the users - they just wanna play.
> 
> So add a boot-time control switch for those warning messages.
> 
> Reported-by: Marcus Rückert <mrueckert@suse.com>
> Signed-off-by: Borislav Petkov <bp@suse.de>
> ---
>  .../admin-guide/kernel-parameters.txt         |  5 +++
>  arch/x86/kernel/umip.c                        | 33 +++++++++++++++++--
>  2 files changed, 35 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 828d11441ebf..815d022c3e87 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -5887,6 +5887,11 @@
>  	unknown_nmi_panic
>  			[X86] Cause panic on unknown NMI.
>  
> +	umip=warnings_off
> +			[X86]
> +			* warnings_off - do not issue warnings when emulating
> +			  UMIP-enabled instructions.
> +
>  	usbcore.authorized_default=
>  			[USB] Default USB device authorization:
>  			(default -1 = authorized except for wireless USB,
> diff --git a/arch/x86/kernel/umip.c b/arch/x86/kernel/umip.c
> index 576b47e7523d..1d37dc626011 100644
> --- a/arch/x86/kernel/umip.c
> +++ b/arch/x86/kernel/umip.c
> @@ -90,10 +90,19 @@ static const char * const umip_insns[5] = {
>  	[UMIP_INST_STR] = "STR",
>  };
>  
> -#define umip_pr_err(regs, fmt, ...) \
> +static struct umip_config {
> +	__u64 warnings_off	: 1,
> +	      __reserved	: 63;
> +} umip_cfg;
> +
> +#define umip_pr_err(regs, fmt, ...)				\
>  	umip_printk(regs, KERN_ERR, fmt, ##__VA_ARGS__)
> -#define umip_pr_warn(regs, fmt, ...) \
> -	umip_printk(regs, KERN_WARNING, fmt,  ##__VA_ARGS__)
> +
> +#define umip_pr_warn(regs, fmt, ...)					\
> +({									\
> +	if (!umip_cfg.warnings_off)					\
> +		umip_printk(regs, KERN_WARNING, fmt,  ##__VA_ARGS__);	\

If it is printing the same information again and again, wouldn't it be
simpler to have a umip_pr_warn_once()?

Thanks and BR,
Ricardo

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] x86/umip: Add a umip= cmdline switch
  2021-09-11  1:14 ` Ricardo Neri
@ 2021-09-11  9:20   ` Borislav Petkov
  2021-09-13 21:38     ` Ricardo Neri
  0 siblings, 1 reply; 16+ messages in thread
From: Borislav Petkov @ 2021-09-11  9:20 UTC (permalink / raw)
  To: Ricardo Neri; +Cc: X86 ML, LKML, Marcus Rückert

On Fri, Sep 10, 2021 at 06:14:59PM -0700, Ricardo Neri wrote:
> If it is printing the same information again and again, wouldn't it be
> simpler to have a umip_pr_warn_once()?

If you do a once thing, you're blocking any other programs from warning,
output you probably wanna see.

With the command line switch you do the same but you're at least pushing
the user to become active and do it first. I.e., with enabling that
option, the user basically says that she/he is not interested in any of
that output and that is ok.

The optimal thing would be to ratelimit it per process but that would be
an overkill and not really needed.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] x86/umip: Add a umip= cmdline switch
  2021-09-11  9:20   ` Borislav Petkov
@ 2021-09-13 21:38     ` Ricardo Neri
  2021-09-14 16:51       ` Borislav Petkov
  0 siblings, 1 reply; 16+ messages in thread
From: Ricardo Neri @ 2021-09-13 21:38 UTC (permalink / raw)
  To: Borislav Petkov; +Cc: X86 ML, LKML, Marcus Rückert

On Sat, Sep 11, 2021 at 11:20:59AM +0200, Borislav Petkov wrote:
> On Fri, Sep 10, 2021 at 06:14:59PM -0700, Ricardo Neri wrote:
> > If it is printing the same information again and again, wouldn't it be
> > simpler to have a umip_pr_warn_once()?
> 
> If you do a once thing, you're blocking any other programs from warning,
> output you probably wanna see.

That is right. Although, I am not sure programs you can have in
the same machine that also want to use UMIP-protected instructions.
> 
> With the command line switch you do the same but you're at least pushing
> the user to become active and do it first. I.e., with enabling that
> option, the user basically says that she/he is not interested in any of
> that output and that is ok.
> 
> The optimal thing would be to ratelimit it per process but that would be
> an overkill and not really needed.

Indeed.

Thanks and BR,
Ricardo

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] x86/umip: Add a umip= cmdline switch
  2021-09-07 20:04 [PATCH] x86/umip: Add a umip= cmdline switch Borislav Petkov
  2021-09-11  1:14 ` Ricardo Neri
@ 2021-09-13 21:45 ` Ricardo Neri
  2021-09-14 16:52   ` Borislav Petkov
  2021-09-23 15:03 ` [tip: x86/cpu] x86/umip: Downgrade warning messages to debug loglevel tip-bot2 for Borislav Petkov
  2021-09-25 11:31 ` tip-bot2 for Borislav Petkov
  3 siblings, 1 reply; 16+ messages in thread
From: Ricardo Neri @ 2021-09-13 21:45 UTC (permalink / raw)
  To: Borislav Petkov; +Cc: X86 ML, LKML, Marcus Rückert

On Tue, Sep 07, 2021 at 10:04:54PM +0200, Borislav Petkov wrote:
> From: Borislav Petkov <bp@suse.de>
> 
> And add the first control option
> 
>   umip=warnings_off
> 
> which disables warnings resulting from emulating UMIP-enabled insns.
> 
> The actual use case is for users playing games in wine, games like
> Overwatch, for example, which go nuts with SIDT:
> 
>   [Di Sep  7 00:24:05 2021] umip_printk: 1345 callbacks suppressed
>   [Di Sep  7 00:24:05 2021] umip: someapp.exe[29231] ip:14064cdba sp:11b7c0: SIDT instruction cannot be used by applications.
>   [Di Sep  7 00:24:05 2021] umip: someapp.exe[29231] ip:14064cdba sp:11b7c0: For now, expensive software emulation returns the result.
>   ...
>   [Di Sep  7 00:26:06 2021] umip_printk: 2227 callbacks suppressed
>   [Di Sep  7 00:26:06 2021] umip: someapp.exe[29231] ip:14064cdba sp:11b940: SIDT instruction cannot be used by applications.
> 
> filling up the kernel log unnecessarily with the same info over and over
> again which doesn't mean a whit to the users - they just wanna play.
> 
> So add a boot-time control switch for those warning messages.
> 
> Reported-by: Marcus Rückert <mrueckert@suse.com>
> Signed-off-by: Borislav Petkov <bp@suse.de>
> ---
>  .../admin-guide/kernel-parameters.txt         |  5 +++
>  arch/x86/kernel/umip.c                        | 33 +++++++++++++++++--
>  2 files changed, 35 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 828d11441ebf..815d022c3e87 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -5887,6 +5887,11 @@
>  	unknown_nmi_panic
>  			[X86] Cause panic on unknown NMI.
>  
> +	umip=warnings_off
> +			[X86]
> +			* warnings_off - do not issue warnings when emulating
> +			  UMIP-enabled instructions.
> +
>  	usbcore.authorized_default=
>  			[USB] Default USB device authorization:
>  			(default -1 = authorized except for wireless USB,
> diff --git a/arch/x86/kernel/umip.c b/arch/x86/kernel/umip.c
> index 576b47e7523d..1d37dc626011 100644
> --- a/arch/x86/kernel/umip.c
> +++ b/arch/x86/kernel/umip.c
> @@ -90,10 +90,19 @@ static const char * const umip_insns[5] = {
>  	[UMIP_INST_STR] = "STR",
>  };
>  
> -#define umip_pr_err(regs, fmt, ...) \
> +static struct umip_config {
> +	__u64 warnings_off	: 1,
> +	      __reserved	: 63;
> +} umip_cfg;
> +
> +#define umip_pr_err(regs, fmt, ...)				\
>  	umip_printk(regs, KERN_ERR, fmt, ##__VA_ARGS__)
> -#define umip_pr_warn(regs, fmt, ...) \
> -	umip_printk(regs, KERN_WARNING, fmt,  ##__VA_ARGS__)
> +
> +#define umip_pr_warn(regs, fmt, ...)					\
> +({									\
> +	if (!umip_cfg.warnings_off)					\
> +		umip_printk(regs, KERN_WARNING, fmt,  ##__VA_ARGS__);	\
> +})
>  
>  /**
>   * umip_printk() - Print a rate-limited message
> @@ -407,5 +416,23 @@ bool fixup_umip_exception(struct pt_regs *regs)
>  
>  	/* increase IP to let the program keep going */
>  	regs->ip += insn.length;
> +

Strictly, this hunk does not logically belong to this patch. I guess it
is harmless, though. Admittedly, the new line should have been there
since umip.c was added.

>  	return true;
>  }
> +
> +static int __init parse_umip_param(char *str)
> +{
> +	if (!str)
> +		return 0;
> +
> +	if (*str == '=')
> +		str++;
> +
> +	if (!strcmp(str, "warnings_off"))
> +		umip_cfg.warnings_off = 1;
> +	else
> +		return 0;
> +
> +	return 1;
> +}
> +__setup("umip", parse_umip_param);

Wouldn't it be better to use parse_option_str() with __setup("umip=",
parse_umip_param)? This would avoid the first two checks.

Thanks and BR,
Ricardo

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] x86/umip: Add a umip= cmdline switch
  2021-09-13 21:38     ` Ricardo Neri
@ 2021-09-14 16:51       ` Borislav Petkov
  2021-09-15 11:34         ` Ricardo Neri
  0 siblings, 1 reply; 16+ messages in thread
From: Borislav Petkov @ 2021-09-14 16:51 UTC (permalink / raw)
  To: Ricardo Neri; +Cc: X86 ML, LKML, Marcus Rückert

On Mon, Sep 13, 2021 at 02:38:36PM -0700, Ricardo Neri wrote:
> That is right. Although, I am not sure programs you can have in
> the same machine that also want to use UMIP-protected instructions.

Sure, another game. :-P

But srsly, looking at those two:

        umip_pr_warn(regs, "%s instruction cannot be used by applications.\n",
                        umip_insns[umip_inst]);

        umip_pr_warn(regs, "For now, expensive software emulation returns the result.\n");

Why are they there at all?

I mean, I can hardly imagine userspace doing anything about them.

They're all likely old, arcane applications or games run in wine which
people have no access to the source code anyway so come to think of it,
the once thing is starting to make more sense to me now.

Sure, that:

        umip_pr_err(regs, "segfault in emulation. error%x\n",
                    X86_PF_USER | X86_PF_WRITE);

should be issued unconditionally but I'm wondering if those warning
messages are needed at all. And if not, I should probably simply rip
them all out.

Or at least silence them by default and flip the cmdline switch logic to
enable them for users who are interested in those things but they should
be silent by default.

I.e., you'd need to supply

	umip=warnings_on

on the cmdline to actually see them.

Hmmm?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] x86/umip: Add a umip= cmdline switch
  2021-09-13 21:45 ` [PATCH] x86/umip: Add a umip= cmdline switch Ricardo Neri
@ 2021-09-14 16:52   ` Borislav Petkov
  0 siblings, 0 replies; 16+ messages in thread
From: Borislav Petkov @ 2021-09-14 16:52 UTC (permalink / raw)
  To: Ricardo Neri; +Cc: X86 ML, LKML, Marcus Rückert

On Mon, Sep 13, 2021 at 02:45:03PM -0700, Ricardo Neri wrote:
> Strictly, this hunk does not logically belong to this patch. I guess it
> is harmless, though. Admittedly, the new line should have been there
> since umip.c was added.

Yeah, I don't like crammed code - that's why I've added it.

> Wouldn't it be better to use parse_option_str() with __setup("umip=",
> parse_umip_param)? This would avoid the first two checks.

Sure.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] x86/umip: Add a umip= cmdline switch
  2021-09-14 16:51       ` Borislav Petkov
@ 2021-09-15 11:34         ` Ricardo Neri
  2021-09-15 12:21           ` Marcus Rückert
  2021-09-15 14:46           ` [PATCH] x86/umip: Downgrade warning messages to debug loglevel Borislav Petkov
  0 siblings, 2 replies; 16+ messages in thread
From: Ricardo Neri @ 2021-09-15 11:34 UTC (permalink / raw)
  To: Borislav Petkov; +Cc: X86 ML, LKML, Marcus Rückert

On Tue, Sep 14, 2021 at 06:51:22PM +0200, Borislav Petkov wrote:
> On Mon, Sep 13, 2021 at 02:38:36PM -0700, Ricardo Neri wrote:
> > That is right. Although, I am not sure programs you can have in
> > the same machine that also want to use UMIP-protected instructions.
> 
> Sure, another game. :-P
> 
> But srsly, looking at those two:
> 
>         umip_pr_warn(regs, "%s instruction cannot be used by applications.\n",
>                         umip_insns[umip_inst]);
> 
>         umip_pr_warn(regs, "For now, expensive software emulation returns the result.\n");
> 
> Why are they there at all?
> 
> I mean, I can hardly imagine userspace doing anything about them.

The goal at the time was encourage users to report bugs on the
applications and eventually have them fixed. It also meant to warn users
about degraded performance due to emulation. To my knowledge, no one has
reported the latter thus far.

> 
> They're all likely old, arcane applications or games run in wine which
> people have no access to the source code anyway so come to think of it,
> the once thing is starting to make more sense to me now.

Indeed, no one has reported "modern" application using these
instructions.

> 
> Sure, that:
> 
>         umip_pr_err(regs, "segfault in emulation. error%x\n",
>                     X86_PF_USER | X86_PF_WRITE);
> 
> should be issued unconditionally but I'm wondering if those warning
> messages are needed at all. And if not, I should probably simply rip
> them all out.
> 
> Or at least silence them by default and flip the cmdline switch logic to
> enable them for users who are interested in those things but they should
> be silent by defauilt.

Since after almost 4 years, performance degradation does not seem to be a
concern, I think it is sensible to remove the warnings.

> 
> I.e., you'd need to supply
> 
> 	umip=warnings_on
> 
> on the cmdline to actually see them.

They could also be salvaged by converting them to umiip_pr_debug(), just
to err on the cautious side without having to add a new command line
argument.

Thanks and BR,
Ricardo

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] x86/umip: Add a umip= cmdline switch
  2021-09-15 11:34         ` Ricardo Neri
@ 2021-09-15 12:21           ` Marcus Rückert
  2021-09-15 13:00             ` Ricardo Neri
  2021-09-15 14:46           ` [PATCH] x86/umip: Downgrade warning messages to debug loglevel Borislav Petkov
  1 sibling, 1 reply; 16+ messages in thread
From: Marcus Rückert @ 2021-09-15 12:21 UTC (permalink / raw)
  To: Ricardo Neri; +Cc: Borislav Petkov, X86 ML, LKML

On Wed, 15 Sep 2021 04:34:10 -0700
Ricardo Neri <ricardo.neri-calderon@linux.intel.com> wrote:
> > They're all likely old, arcane applications or games run in wine
> > which people have no access to the source code anyway so come to
> > think of it, the once thing is starting to make more sense to me
> > now.
> 
> Indeed, no one has reported "modern" application using these
> instructions.

I am not sure if Blizzard Entertainment would tell us why they use this
CPU instruction in Overwatch. And that game is "only" 5 years old.

   darix

-- 
Always remember:
  Never accept the world as it appears to be.
    Dare to see it for what it could be.
      The world can always use more heroes.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] x86/umip: Add a umip= cmdline switch
  2021-09-15 12:21           ` Marcus Rückert
@ 2021-09-15 13:00             ` Ricardo Neri
  2021-09-15 13:14               ` Borislav Petkov
  0 siblings, 1 reply; 16+ messages in thread
From: Ricardo Neri @ 2021-09-15 13:00 UTC (permalink / raw)
  To: Marcus Rückert; +Cc: Borislav Petkov, X86 ML, LKML

On Wed, Sep 15, 2021 at 02:21:23PM +0200, Marcus Rückert wrote:
> On Wed, 15 Sep 2021 04:34:10 -0700
> Ricardo Neri <ricardo.neri-calderon@linux.intel.com> wrote:
> > > They're all likely old, arcane applications or games run in wine
> > > which people have no access to the source code anyway so come to
> > > think of it, the once thing is starting to make more sense to me
> > > now.
> > 
> > Indeed, no one has reported "modern" application using these
> > instructions.
> 
> I am not sure if Blizzard Entertainment would tell us why they use this
> CPU instruction in Overwatch. And that game is "only" 5 years old.

Ah! 5 years old does not seem too old to me. Then it is not only old
applications. Then the warning did catch an app that could in theory be
fixed (if Overwatch is still maintained).

Thanks and BR,
Ricardo

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] x86/umip: Add a umip= cmdline switch
  2021-09-15 13:00             ` Ricardo Neri
@ 2021-09-15 13:14               ` Borislav Petkov
  0 siblings, 0 replies; 16+ messages in thread
From: Borislav Petkov @ 2021-09-15 13:14 UTC (permalink / raw)
  To: Ricardo Neri; +Cc: Marcus Rückert, X86 ML, LKML

On Wed, Sep 15, 2021 at 06:00:07AM -0700, Ricardo Neri wrote:
> Ah! 5 years old does not seem too old to me. Then it is not only old
> applications. Then the warning did catch an app that could in theory be
> fixed (if Overwatch is still maintained).

I wouldn't hold my breath though.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH] x86/umip: Downgrade warning messages to debug loglevel
  2021-09-15 11:34         ` Ricardo Neri
  2021-09-15 12:21           ` Marcus Rückert
@ 2021-09-15 14:46           ` Borislav Petkov
  2021-09-16  0:27             ` Ricardo Neri
  1 sibling, 1 reply; 16+ messages in thread
From: Borislav Petkov @ 2021-09-15 14:46 UTC (permalink / raw)
  To: Ricardo Neri; +Cc: X86 ML, LKML, Marcus Rückert

On Wed, Sep 15, 2021 at 04:34:10AM -0700, Ricardo Neri wrote:
> The goal at the time was encourage users to report bugs on the
> applications and eventually have them fixed. It also meant to warn users
> about degraded performance due to emulation. To my knowledge, no one has
> reported the latter thus far.

Probably because people do not even get to need UMIP a whole lot,
apparently.

> Since after almost 4 years, performance degradation does not seem to be a
> concern, I think it is sensible to remove the warnings.

Yap.

> They could also be salvaged by converting them to umiip_pr_debug(), just
> to err on the cautious side without having to add a new command line
> argument.

Yap, that's a good idea too:

---
From: Borislav Petkov <bp@suse.de>
Date: Wed, 15 Sep 2021 16:39:18 +0200
Subject: [PATCH] x86/umip: Downgrade warning messages to debug loglevel
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

After four years in the wild, those have not fullfilled their
initial purpose of pushing people to fix their software to not use
UMIP-emulated instructions, and to warn users about the degraded
emulation performance.

Yet, the only thing that "degrades" performance is overflowing dmesg
with those:

  [Di Sep  7 00:24:05 2021] umip_printk: 1345 callbacks suppressed
  [Di Sep  7 00:24:05 2021] umip: someapp.exe[29231] ip:14064cdba sp:11b7c0: SIDT instruction cannot be used by applications.
  [Di Sep  7 00:24:05 2021] umip: someapp.exe[29231] ip:14064cdba sp:11b7c0: For now, expensive software emulation returns the result.
  ...
  [Di Sep  7 00:26:06 2021] umip_printk: 2227 callbacks suppressed
  [Di Sep  7 00:26:06 2021] umip: someapp.exe[29231] ip:14064cdba sp:11b940: SIDT instruction cannot be used by applications.

and users don't really care about that - they just want to play their
games in wine.

So convert those to debug loglevel - in case someone is still interested
in them, someone can boot with "debug" on the kernel cmdline.

Reported-by: Marcus Rückert <mrueckert@suse.com>
Signed-off-by: Borislav Petkov <bp@suse.de>
Link: https://lkml.kernel.org/r/20210907200454.30458-1-bp@alien8.de
---
 arch/x86/kernel/umip.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/umip.c b/arch/x86/kernel/umip.c
index 576b47e7523d..5a4b21389b1d 100644
--- a/arch/x86/kernel/umip.c
+++ b/arch/x86/kernel/umip.c
@@ -92,8 +92,8 @@ static const char * const umip_insns[5] = {
 
 #define umip_pr_err(regs, fmt, ...) \
 	umip_printk(regs, KERN_ERR, fmt, ##__VA_ARGS__)
-#define umip_pr_warn(regs, fmt, ...) \
-	umip_printk(regs, KERN_WARNING, fmt,  ##__VA_ARGS__)
+#define umip_pr_debug(regs, fmt, ...) \
+	umip_printk(regs, KERN_DEBUG, fmt,  ##__VA_ARGS__)
 
 /**
  * umip_printk() - Print a rate-limited message
@@ -361,10 +361,10 @@ bool fixup_umip_exception(struct pt_regs *regs)
 	if (umip_inst < 0)
 		return false;
 
-	umip_pr_warn(regs, "%s instruction cannot be used by applications.\n",
+	umip_pr_debug(regs, "%s instruction cannot be used by applications.\n",
 			umip_insns[umip_inst]);
 
-	umip_pr_warn(regs, "For now, expensive software emulation returns the result.\n");
+	umip_pr_debug(regs, "For now, expensive software emulation returns the result.\n");
 
 	if (emulate_umip_insn(&insn, umip_inst, dummy_data, &dummy_data_size,
 			      user_64bit_mode(regs)))
-- 
2.29.2


-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] x86/umip: Downgrade warning messages to debug loglevel
  2021-09-15 14:46           ` [PATCH] x86/umip: Downgrade warning messages to debug loglevel Borislav Petkov
@ 2021-09-16  0:27             ` Ricardo Neri
  2021-09-23 14:59               ` Borislav Petkov
  0 siblings, 1 reply; 16+ messages in thread
From: Ricardo Neri @ 2021-09-16  0:27 UTC (permalink / raw)
  To: Borislav Petkov; +Cc: X86 ML, LKML, Marcus Rückert

On Wed, Sep 15, 2021 at 04:46:20PM +0200, Borislav Petkov wrote:
> On Wed, Sep 15, 2021 at 04:34:10AM -0700, Ricardo Neri wrote:
> > The goal at the time was encourage users to report bugs on the
> > applications and eventually have them fixed. It also meant to warn users
> > about degraded performance due to emulation. To my knowledge, no one has
> > reported the latter thus far.
> 
> Probably because people do not even get to need UMIP a whole lot,
> apparently.
> 
> > Since after almost 4 years, performance degradation does not seem to be a
> > concern, I think it is sensible to remove the warnings.
> 
> Yap.
> 
> > They could also be salvaged by converting them to umiip_pr_debug(), just
> > to err on the cautious side without having to add a new command line
> > argument.
> 
> Yap, that's a good idea too:
> 
> ---
> From: Borislav Petkov <bp@suse.de>
> Date: Wed, 15 Sep 2021 16:39:18 +0200
> Subject: [PATCH] x86/umip: Downgrade warning messages to debug loglevel
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
> 
> After four years in the wild, those have not fullfilled their
> initial purpose of pushing people to fix their software to not use
> UMIP-emulated instructions, and to warn users about the degraded
> emulation performance.
> 
> Yet, the only thing that "degrades" performance is overflowing dmesg
> with those:
> 
>   [Di Sep  7 00:24:05 2021] umip_printk: 1345 callbacks suppressed
>   [Di Sep  7 00:24:05 2021] umip: someapp.exe[29231] ip:14064cdba sp:11b7c0: SIDT instruction cannot be used by applications.
>   [Di Sep  7 00:24:05 2021] umip: someapp.exe[29231] ip:14064cdba sp:11b7c0: For now, expensive software emulation returns the result.
>   ...
>   [Di Sep  7 00:26:06 2021] umip_printk: 2227 callbacks suppressed
>   [Di Sep  7 00:26:06 2021] umip: someapp.exe[29231] ip:14064cdba sp:11b940: SIDT instruction cannot be used by applications.
> 
> and users don't really care about that - they just want to play their
> games in wine.
> 
> So convert those to debug loglevel - in case someone is still interested
> in them, someone can boot with "debug" on the kernel cmdline.
> 
> Reported-by: Marcus Rückert <mrueckert@suse.com>
> Signed-off-by: Borislav Petkov <bp@suse.de>
> Link: https://lkml.kernel.org/r/20210907200454.30458-1-bp@alien8.de
> ---
>  arch/x86/kernel/umip.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kernel/umip.c b/arch/x86/kernel/umip.c
> index 576b47e7523d..5a4b21389b1d 100644
> --- a/arch/x86/kernel/umip.c
> +++ b/arch/x86/kernel/umip.c
> @@ -92,8 +92,8 @@ static const char * const umip_insns[5] = {
>  
>  #define umip_pr_err(regs, fmt, ...) \
>  	umip_printk(regs, KERN_ERR, fmt, ##__VA_ARGS__)
> -#define umip_pr_warn(regs, fmt, ...) \
> -	umip_printk(regs, KERN_WARNING, fmt,  ##__VA_ARGS__)
> +#define umip_pr_debug(regs, fmt, ...) \
> +	umip_printk(regs, KERN_DEBUG, fmt,  ##__VA_ARGS__)
>  
>  /**
>   * umip_printk() - Print a rate-limited message
> @@ -361,10 +361,10 @@ bool fixup_umip_exception(struct pt_regs *regs)
>  	if (umip_inst < 0)
>  		return false;
>  
> -	umip_pr_warn(regs, "%s instruction cannot be used by applications.\n",
> +	umip_pr_debug(regs, "%s instruction cannot be used by applications.\n",
>  			umip_insns[umip_inst]);
>  
> -	umip_pr_warn(regs, "For now, expensive software emulation returns the result.\n");
> +	umip_pr_debug(regs, "For now, expensive software emulation returns the result.\n");
>  
>  	if (emulate_umip_insn(&insn, umip_inst, dummy_data, &dummy_data_size,
>  			      user_64bit_mode(regs)))

FWIW, Reviewed-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>

Thanks and BR,
Ricardo

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] x86/umip: Downgrade warning messages to debug loglevel
  2021-09-16  0:27             ` Ricardo Neri
@ 2021-09-23 14:59               ` Borislav Petkov
  0 siblings, 0 replies; 16+ messages in thread
From: Borislav Petkov @ 2021-09-23 14:59 UTC (permalink / raw)
  To: Ricardo Neri; +Cc: X86 ML, LKML, Marcus Rückert

On Wed, Sep 15, 2021 at 05:27:35PM -0700, Ricardo Neri wrote:
> FWIW, Reviewed-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>

Thx.

So this thing is not completely quiet:

# dmesg -l err,warn,notice -x
...
kern  :notice: [   17.974749] random: dd: uninitialized urandom read (512 bytes read)
kern  :notice: [   18.063596] random: dbus-daemon: uninitialized urandom read (12 bytes read)
kern  :notice: [   18.065212] random: dbus-daemon: uninitialized urandom read (12 bytes read)
kern  :notice: [   20.161303] random: crng init done
kern  :warn  : [  144.505690] umip_printk: 403267 callbacks suppressed
^^^^^^^^^^^^^^^^

so at least it says that umip is being busy logging stuff and if one
wants to see them, one can do:

# dmesg -l err,warn,notice,debug -x
kern  :notice: [   17.974749] random: dd: uninitialized urandom read (512 bytes read)
kern  :notice: [   18.063596] random: dbus-daemon: uninitialized urandom read (12 bytes read)
kern  :notice: [   18.065212] random: dbus-daemon: uninitialized urandom read (12 bytes read)
kern  :notice: [   20.161303] random: crng init done
kern  :debug : [   24.501738] umip: sidt[1882] ip:5570e245a083 sp:7ffdbe080390: SIDT instruction cannot be used by applications.
kern  :debug : [   24.501743] umip: sidt[1882] ip:5570e245a083 sp:7ffdbe080390: For now, expensive software emulation returns the result.
kern  :debug : [   24.502566] umip: sidt[1883] ip:55c63b5ff083 sp:7fffde333160: SIDT instruction cannot be used by applications.
kern  :debug : [   24.502569] umip: sidt[1883] ip:55c63b5ff083 sp:7fffde333160: For now, expensive software emulation returns the result.
kern  :debug : [   24.503259] umip: sidt[1884] ip:55e4bff9e083 sp:7ffecb324530: SIDT instruction cannot be used by applications.
kern  :warn  : [  144.505690] umip_printk: 403267 callbacks suppressed
kern  :debug : [  144.505692] umip: sidt[9170] ip:560d1457f083 sp:7fff1d5d9df0: SIDT instruction cannot be used by applications.
kern  :debug : [  144.506484] umip: sidt[9170] ip:560d1457f083 sp:7fff1d5d9df0: For now, expensive software emulation returns the result.
kern  :debug : [  144.507218] umip: sidt[9171] ip:556f4549c083 sp:7ffc146ab2a0: SIDT instruction cannot be used by applications.
kern  :debug : [  144.507221] umip: sidt[9171] ip:556f4549c083 sp:7ffc146ab2a0: For now, expensive software emulation returns the result.
kern  :debug : [  144.507951] umip: sidt[9172] ip:55662fdd8083 sp:7ffefcdbc0d0: SIDT instruction cannot be used by applications.

ok, lemme queue it.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [tip: x86/cpu] x86/umip: Downgrade warning messages to debug loglevel
  2021-09-07 20:04 [PATCH] x86/umip: Add a umip= cmdline switch Borislav Petkov
  2021-09-11  1:14 ` Ricardo Neri
  2021-09-13 21:45 ` [PATCH] x86/umip: Add a umip= cmdline switch Ricardo Neri
@ 2021-09-23 15:03 ` tip-bot2 for Borislav Petkov
  2021-09-25 11:31 ` tip-bot2 for Borislav Petkov
  3 siblings, 0 replies; 16+ messages in thread
From: tip-bot2 for Borislav Petkov @ 2021-09-23 15:03 UTC (permalink / raw)
  To: linux-tip-commits; +Cc: mrueckert, Borislav Petkov, x86, linux-kernel

The following commit has been merged into the x86/cpu branch of tip:

Commit-ID:     1eefe7a85678a056951cd9efb09820c1b0a1b4da
Gitweb:        https://git.kernel.org/tip/1eefe7a85678a056951cd9efb09820c1b0a1b4da
Author:        Borislav Petkov <bp@suse.de>
AuthorDate:    Wed, 15 Sep 2021 16:39:18 +02:00
Committer:     Borislav Petkov <bp@suse.de>
CommitterDate: Thu, 23 Sep 2021 16:30:11 +02:00

x86/umip: Downgrade warning messages to debug loglevel

After four years in the wild, those have not fullfilled their
initial purpose of pushing people to fix their software to not use
UMIP-emulated instructions, and to warn users about the degraded
emulation performance.

Yet, the only thing that "degrades" performance is overflowing dmesg
with those:

  [Di Sep  7 00:24:05 2021] umip_printk: 1345 callbacks suppressed
  [Di Sep  7 00:24:05 2021] umip: someapp.exe[29231] ip:14064cdba sp:11b7c0: SIDT instruction cannot be used by applications.
  [Di Sep  7 00:24:05 2021] umip: someapp.exe[29231] ip:14064cdba sp:11b7c0: For now, expensive software emulation returns the result.
  ...
  [Di Sep  7 00:26:06 2021] umip_printk: 2227 callbacks suppressed
  [Di Sep  7 00:26:06 2021] umip: someapp.exe[29231] ip:14064cdba sp:11b940: SIDT instruction cannot be used by applications.

and users don't really care about that - they just want to play their
games in wine.

So convert those to debug loglevel - in case someone is still interested
in them, someone can boot with "debug" on the kernel cmdline.

Reported-by: Marcus Rückert <mrueckert@suse.com>
Signed-off-by: Borislav Petkov <bp@suse.de>
Link: https://lkml.kernel.org/r/20210907200454.30458-1-bp@alien8.de
---
 arch/x86/kernel/umip.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/umip.c b/arch/x86/kernel/umip.c
index 576b47e..5a4b213 100644
--- a/arch/x86/kernel/umip.c
+++ b/arch/x86/kernel/umip.c
@@ -92,8 +92,8 @@ static const char * const umip_insns[5] = {
 
 #define umip_pr_err(regs, fmt, ...) \
 	umip_printk(regs, KERN_ERR, fmt, ##__VA_ARGS__)
-#define umip_pr_warn(regs, fmt, ...) \
-	umip_printk(regs, KERN_WARNING, fmt,  ##__VA_ARGS__)
+#define umip_pr_debug(regs, fmt, ...) \
+	umip_printk(regs, KERN_DEBUG, fmt,  ##__VA_ARGS__)
 
 /**
  * umip_printk() - Print a rate-limited message
@@ -361,10 +361,10 @@ bool fixup_umip_exception(struct pt_regs *regs)
 	if (umip_inst < 0)
 		return false;
 
-	umip_pr_warn(regs, "%s instruction cannot be used by applications.\n",
+	umip_pr_debug(regs, "%s instruction cannot be used by applications.\n",
 			umip_insns[umip_inst]);
 
-	umip_pr_warn(regs, "For now, expensive software emulation returns the result.\n");
+	umip_pr_debug(regs, "For now, expensive software emulation returns the result.\n");
 
 	if (emulate_umip_insn(&insn, umip_inst, dummy_data, &dummy_data_size,
 			      user_64bit_mode(regs)))

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [tip: x86/cpu] x86/umip: Downgrade warning messages to debug loglevel
  2021-09-07 20:04 [PATCH] x86/umip: Add a umip= cmdline switch Borislav Petkov
                   ` (2 preceding siblings ...)
  2021-09-23 15:03 ` [tip: x86/cpu] x86/umip: Downgrade warning messages to debug loglevel tip-bot2 for Borislav Petkov
@ 2021-09-25 11:31 ` tip-bot2 for Borislav Petkov
  3 siblings, 0 replies; 16+ messages in thread
From: tip-bot2 for Borislav Petkov @ 2021-09-25 11:31 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: mrueckert, Borislav Petkov, Ricardo Neri, x86, linux-kernel

The following commit has been merged into the x86/cpu branch of tip:

Commit-ID:     f3f07ae425bc09039d9e0c73c86b76f95d9d5cd6
Gitweb:        https://git.kernel.org/tip/f3f07ae425bc09039d9e0c73c86b76f95d9d5cd6
Author:        Borislav Petkov <bp@suse.de>
AuthorDate:    Wed, 15 Sep 2021 16:39:18 +02:00
Committer:     Borislav Petkov <bp@suse.de>
CommitterDate: Sat, 25 Sep 2021 13:23:28 +02:00

x86/umip: Downgrade warning messages to debug loglevel

After four years in the wild, those have not fullfilled their
initial purpose of pushing people to fix their software to not use
UMIP-emulated instructions, and to warn users about the degraded
emulation performance.

Yet, the only thing that "degrades" performance is overflowing dmesg
with those:

  [Di Sep  7 00:24:05 2021] umip_printk: 1345 callbacks suppressed
  [Di Sep  7 00:24:05 2021] umip: someapp.exe[29231] ip:14064cdba sp:11b7c0: SIDT instruction cannot be used by applications.
  [Di Sep  7 00:24:05 2021] umip: someapp.exe[29231] ip:14064cdba sp:11b7c0: For now, expensive software emulation returns the result.
  ...
  [Di Sep  7 00:26:06 2021] umip_printk: 2227 callbacks suppressed
  [Di Sep  7 00:26:06 2021] umip: someapp.exe[29231] ip:14064cdba sp:11b940: SIDT instruction cannot be used by applications.

and users don't really care about that - they just want to play their
games in wine.

So convert those to debug loglevel - in case someone is still interested
in them, someone can boot with "debug" on the kernel cmdline.

Reported-by: Marcus Rückert <mrueckert@suse.com>
Signed-off-by: Borislav Petkov <bp@suse.de>
Reviewed-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
Link: https://lkml.kernel.org/r/20210907200454.30458-1-bp@alien8.de
---
 arch/x86/kernel/umip.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/umip.c b/arch/x86/kernel/umip.c
index 576b47e..5a4b213 100644
--- a/arch/x86/kernel/umip.c
+++ b/arch/x86/kernel/umip.c
@@ -92,8 +92,8 @@ static const char * const umip_insns[5] = {
 
 #define umip_pr_err(regs, fmt, ...) \
 	umip_printk(regs, KERN_ERR, fmt, ##__VA_ARGS__)
-#define umip_pr_warn(regs, fmt, ...) \
-	umip_printk(regs, KERN_WARNING, fmt,  ##__VA_ARGS__)
+#define umip_pr_debug(regs, fmt, ...) \
+	umip_printk(regs, KERN_DEBUG, fmt,  ##__VA_ARGS__)
 
 /**
  * umip_printk() - Print a rate-limited message
@@ -361,10 +361,10 @@ bool fixup_umip_exception(struct pt_regs *regs)
 	if (umip_inst < 0)
 		return false;
 
-	umip_pr_warn(regs, "%s instruction cannot be used by applications.\n",
+	umip_pr_debug(regs, "%s instruction cannot be used by applications.\n",
 			umip_insns[umip_inst]);
 
-	umip_pr_warn(regs, "For now, expensive software emulation returns the result.\n");
+	umip_pr_debug(regs, "For now, expensive software emulation returns the result.\n");
 
 	if (emulate_umip_insn(&insn, umip_inst, dummy_data, &dummy_data_size,
 			      user_64bit_mode(regs)))

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2021-09-25 11:31 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-07 20:04 [PATCH] x86/umip: Add a umip= cmdline switch Borislav Petkov
2021-09-11  1:14 ` Ricardo Neri
2021-09-11  9:20   ` Borislav Petkov
2021-09-13 21:38     ` Ricardo Neri
2021-09-14 16:51       ` Borislav Petkov
2021-09-15 11:34         ` Ricardo Neri
2021-09-15 12:21           ` Marcus Rückert
2021-09-15 13:00             ` Ricardo Neri
2021-09-15 13:14               ` Borislav Petkov
2021-09-15 14:46           ` [PATCH] x86/umip: Downgrade warning messages to debug loglevel Borislav Petkov
2021-09-16  0:27             ` Ricardo Neri
2021-09-23 14:59               ` Borislav Petkov
2021-09-13 21:45 ` [PATCH] x86/umip: Add a umip= cmdline switch Ricardo Neri
2021-09-14 16:52   ` Borislav Petkov
2021-09-23 15:03 ` [tip: x86/cpu] x86/umip: Downgrade warning messages to debug loglevel tip-bot2 for Borislav Petkov
2021-09-25 11:31 ` tip-bot2 for Borislav Petkov

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.