Commit 5e7fa023 authored by Tim Chen's avatar Tim Chen Committed by Marcelo Henrique Cerri

x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature

CVE-2017-5753
CVE-2017-5715

There are 2 ways to control IBPB and IBRS

1. At boot time
	noibrs kernel boot parameter will disable IBRS usage
	noibpb kernel boot parameter will disable IBPB usage
Otherwise if the above parameters are not specified, the system
will enable ibrs and ibpb usage if the cpu supports it.

2. At run time
	echo 0 > /proc/sys/kernel/ibrs_enabled will turn off IBRS
	echo 1 > /proc/sys/kernel/ibrs_enabled will turn on IBRS in kernel
	echo 2 > /proc/sys/kernel/ibrs_enabled will turn on IBRS in both userspace and kernel
Signed-off-by: default avatarTim Chen <tim.c.chen@linux.intel.com>
Signed-off-by: default avatarAndy Whitcroft <apw@canonical.com>
(backported from commit 50169d8fada2532084c9f8ccde51c6c9211603d5)
Signed-off-by: default avatarAndy Whitcroft <apw@canonical.com>
parent ba5013cf
This source diff could not be displayed because it is too large. You can view the blob instead.
...@@ -106,14 +106,14 @@ static inline void mwait_idle_with_hints(unsigned long eax, unsigned long ecx) ...@@ -106,14 +106,14 @@ static inline void mwait_idle_with_hints(unsigned long eax, unsigned long ecx)
mb(); mb();
} }
if (boot_cpu_has(X86_FEATURE_SPEC_CTRL)) if (ibrs_inuse)
native_wrmsrl(MSR_IA32_SPEC_CTRL, 0); native_wrmsrl(MSR_IA32_SPEC_CTRL, 0);
__monitor((void *)&current_thread_info()->flags, 0, 0); __monitor((void *)&current_thread_info()->flags, 0, 0);
if (!need_resched()) if (!need_resched())
__mwait(eax, ecx); __mwait(eax, ecx);
if (boot_cpu_has(X86_FEATURE_SPEC_CTRL)) if (ibrs_inuse)
native_wrmsrl(MSR_IA32_SPEC_CTRL, FEATURE_ENABLE_IBRS); native_wrmsrl(MSR_IA32_SPEC_CTRL, FEATURE_ENABLE_IBRS);
} }
current_clr_polling(); current_clr_polling();
......
...@@ -8,6 +8,9 @@ ...@@ -8,6 +8,9 @@
#ifdef __ASSEMBLY__ #ifdef __ASSEMBLY__
.extern use_ibrs
.extern use_ibpb
#define __ASM_ENABLE_IBRS \ #define __ASM_ENABLE_IBRS \
pushq %rax; \ pushq %rax; \
pushq %rcx; \ pushq %rcx; \
...@@ -104,15 +107,30 @@ ...@@ -104,15 +107,30 @@
add $(32*8), %rsp; add $(32*8), %rsp;
.macro ENABLE_IBRS .macro ENABLE_IBRS
ALTERNATIVE "", __stringify(__ASM_ENABLE_IBRS), X86_FEATURE_SPEC_CTRL testl $1, use_ibrs
jz 10f
__ASM_ENABLE_IBRS
jmp 20f
10:
lfence
20:
.endm .endm
.macro ENABLE_IBRS_CLOBBER .macro ENABLE_IBRS_CLOBBER
ALTERNATIVE "", __stringify(__ASM_ENABLE_IBRS_CLOBBER), X86_FEATURE_SPEC_CTRL testl $1, use_ibrs
jz 11f
__ASM_ENABLE_IBRS_CLOBBER
jmp 21f
11:
lfence
21:
.endm .endm
.macro DISABLE_IBRS .macro DISABLE_IBRS
ALTERNATIVE "", __stringify(__ASM_DISABLE_IBRS), X86_FEATURE_SPEC_CTRL testl $1, use_ibrs
jz 9f
__ASM_DISABLE_IBRS
9:
.endm .endm
.macro STUFF_RSB .macro STUFF_RSB
......
...@@ -529,10 +529,17 @@ static void init_intel(struct cpuinfo_x86 *c) ...@@ -529,10 +529,17 @@ static void init_intel(struct cpuinfo_x86 *c)
init_intel_energy_perf(c); init_intel_energy_perf(c);
if (boot_cpu_has(X86_FEATURE_SPEC_CTRL)) if (boot_cpu_has(X86_FEATURE_SPEC_CTRL)) {
printk_once(KERN_INFO "FEATURE SPEC_CTRL Present\n"); printk_once(KERN_INFO "FEATURE SPEC_CTRL Present\n");
else set_ibrs_supported();
set_ibpb_supported();
if (ibrs_inuse)
sysctl_ibrs_enabled = 1;
if (ibpb_inuse)
sysctl_ibpb_enabled = 1;
} else {
printk_once(KERN_INFO "FEATURE SPEC_CTRL Not Present\n"); printk_once(KERN_INFO "FEATURE SPEC_CTRL Not Present\n");
}
} }
#ifdef CONFIG_X86_32 #ifdef CONFIG_X86_32
......
...@@ -419,6 +419,17 @@ static ssize_t reload_store(struct device *dev, ...@@ -419,6 +419,17 @@ static ssize_t reload_store(struct device *dev,
} }
if (!ret) if (!ret)
perf_check_microcode(); perf_check_microcode();
if (boot_cpu_has(X86_FEATURE_SPEC_CTRL)) {
printk_once(KERN_INFO "FEATURE SPEC_CTRL Present\n");
set_ibrs_supported();
set_ibpb_supported();
if (ibrs_inuse)
sysctl_ibrs_enabled = 1;
if (ibpb_inuse)
sysctl_ibpb_enabled = 1;
}
mutex_unlock(&microcode_mutex); mutex_unlock(&microcode_mutex);
put_online_cpus(); put_online_cpus();
......
...@@ -424,16 +424,16 @@ static void mwait_idle(void) ...@@ -424,16 +424,16 @@ static void mwait_idle(void)
smp_mb(); /* quirk */ smp_mb(); /* quirk */
} }
if (boot_cpu_has(X86_FEATURE_SPEC_CTRL)) if (ibrs_inuse)
native_wrmsrl(MSR_IA32_SPEC_CTRL, 0); native_wrmsrl(MSR_IA32_SPEC_CTRL, 0);
__monitor((void *)&current_thread_info()->flags, 0, 0); __monitor((void *)&current_thread_info()->flags, 0, 0);
if (!need_resched()) { if (!need_resched()) {
__sti_mwait(0, 0); __sti_mwait(0, 0);
if (boot_cpu_has(X86_FEATURE_SPEC_CTRL)) if (ibrs_inuse)
native_wrmsrl(MSR_IA32_SPEC_CTRL, FEATURE_ENABLE_IBRS); native_wrmsrl(MSR_IA32_SPEC_CTRL, FEATURE_ENABLE_IBRS);
} else { } else {
if (boot_cpu_has(X86_FEATURE_SPEC_CTRL)) if (ibrs_inuse)
native_wrmsrl(MSR_IA32_SPEC_CTRL, FEATURE_ENABLE_IBRS); native_wrmsrl(MSR_IA32_SPEC_CTRL, FEATURE_ENABLE_IBRS);
local_irq_enable(); local_irq_enable();
} }
......
...@@ -1657,14 +1657,14 @@ void native_play_dead(void) ...@@ -1657,14 +1657,14 @@ void native_play_dead(void)
play_dead_common(); play_dead_common();
tboot_shutdown(TB_SHUTDOWN_WFS); tboot_shutdown(TB_SHUTDOWN_WFS);
if (boot_cpu_has(X86_FEATURE_SPEC_CTRL)) if (ibrs_inuse)
native_wrmsrl(MSR_IA32_SPEC_CTRL, 0); native_wrmsrl(MSR_IA32_SPEC_CTRL, 0);
mwait_play_dead(); /* Only returns on failure */ mwait_play_dead(); /* Only returns on failure */
if (cpuidle_play_dead()) if (cpuidle_play_dead())
hlt_play_dead(); hlt_play_dead();
if (boot_cpu_has(X86_FEATURE_SPEC_CTRL)) if (ibrs_inuse)
native_wrmsrl(MSR_IA32_SPEC_CTRL, FEATURE_ENABLE_IBRS); native_wrmsrl(MSR_IA32_SPEC_CTRL, FEATURE_ENABLE_IBRS);
} }
......
...@@ -2050,7 +2050,7 @@ static void vmx_vcpu_load(struct kvm_vcpu *vcpu, int cpu) ...@@ -2050,7 +2050,7 @@ static void vmx_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
if (per_cpu(current_vmcs, cpu) != vmx->loaded_vmcs->vmcs) { if (per_cpu(current_vmcs, cpu) != vmx->loaded_vmcs->vmcs) {
per_cpu(current_vmcs, cpu) = vmx->loaded_vmcs->vmcs; per_cpu(current_vmcs, cpu) = vmx->loaded_vmcs->vmcs;
vmcs_load(vmx->loaded_vmcs->vmcs); vmcs_load(vmx->loaded_vmcs->vmcs);
if (boot_cpu_has(X86_FEATURE_SPEC_CTRL)) if (ibpb_inuse)
native_wrmsrl(MSR_IA32_PRED_CMD, FEATURE_SET_IBPB); native_wrmsrl(MSR_IA32_PRED_CMD, FEATURE_SET_IBPB);
} }
...@@ -8600,7 +8600,7 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu *vcpu) ...@@ -8600,7 +8600,7 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu *vcpu)
atomic_switch_perf_msrs(vmx); atomic_switch_perf_msrs(vmx);
if (boot_cpu_has(X86_FEATURE_SPEC_CTRL)) if (ibrs_inuse)
add_atomic_switch_msr(vmx, MSR_IA32_SPEC_CTRL, add_atomic_switch_msr(vmx, MSR_IA32_SPEC_CTRL,
vcpu->arch.spec_ctrl, FEATURE_ENABLE_IBRS); vcpu->arch.spec_ctrl, FEATURE_ENABLE_IBRS);
......
...@@ -100,8 +100,7 @@ static void delay_mwaitx(unsigned long __loops) ...@@ -100,8 +100,7 @@ static void delay_mwaitx(unsigned long __loops)
for (;;) { for (;;) {
delay = min_t(u64, MWAITX_MAX_LOOPS, loops); delay = min_t(u64, MWAITX_MAX_LOOPS, loops);
if (boot_cpu_has(X86_FEATURE_SPEC_CTRL) && if (ibrs_inuse && (delay > IBRS_DISABLE_THRESHOLD))
(delay > IBRS_DISABLE_THRESHOLD))
native_wrmsrl(MSR_IA32_SPEC_CTRL, 0); native_wrmsrl(MSR_IA32_SPEC_CTRL, 0);
/* /*
...@@ -117,8 +116,7 @@ static void delay_mwaitx(unsigned long __loops) ...@@ -117,8 +116,7 @@ static void delay_mwaitx(unsigned long __loops)
*/ */
__mwaitx(MWAITX_DISABLE_CSTATES, delay, MWAITX_ECX_TIMER_ENABLE); __mwaitx(MWAITX_DISABLE_CSTATES, delay, MWAITX_ECX_TIMER_ENABLE);
if (boot_cpu_has(X86_FEATURE_SPEC_CTRL) && if (ibrs_inuse && (delay > IBRS_DISABLE_THRESHOLD))
(delay > IBRS_DISABLE_THRESHOLD))
native_wrmsrl(MSR_IA32_SPEC_CTRL, FEATURE_ENABLE_IBRS); native_wrmsrl(MSR_IA32_SPEC_CTRL, FEATURE_ENABLE_IBRS);
end = rdtsc_ordered(); end = rdtsc_ordered();
......
...@@ -142,7 +142,7 @@ void switch_mm(struct mm_struct *prev, struct mm_struct *next, ...@@ -142,7 +142,7 @@ void switch_mm(struct mm_struct *prev, struct mm_struct *next,
cpumask_clear_cpu(cpu, mm_cpumask(prev)); cpumask_clear_cpu(cpu, mm_cpumask(prev));
/* Null tsk means switching to kernel, so that's safe */ /* Null tsk means switching to kernel, so that's safe */
if (boot_cpu_has(X86_FEATURE_SPEC_CTRL) && tsk && if (ibpb_inuse && tsk &&
___ptrace_may_access(tsk, current, PTRACE_MODE_IBPB)) ___ptrace_may_access(tsk, current, PTRACE_MODE_IBPB))
native_wrmsrl(MSR_IA32_PRED_CMD, FEATURE_SET_IBPB); native_wrmsrl(MSR_IA32_PRED_CMD, FEATURE_SET_IBPB);
......
...@@ -50,6 +50,93 @@ void on_each_cpu_cond(bool (*cond_func)(int cpu, void *info), ...@@ -50,6 +50,93 @@ void on_each_cpu_cond(bool (*cond_func)(int cpu, void *info),
int smp_call_function_single_async(int cpu, struct call_single_data *csd); int smp_call_function_single_async(int cpu, struct call_single_data *csd);
#ifdef CONFIG_X86
/* indicate usage of IBRS to control execution speculation */
extern int use_ibrs;
extern u32 sysctl_ibrs_enabled;
extern struct mutex spec_ctrl_mutex;
#define ibrs_supported (use_ibrs & 0x2)
#define ibrs_disabled (use_ibrs & 0x4)
static inline void set_ibrs_inuse(void)
{
if (ibrs_supported)
use_ibrs |= 0x1;
}
static inline void clear_ibrs_inuse(void)
{
use_ibrs &= ~0x1;
}
static inline int check_ibrs_inuse(void)
{
if (use_ibrs & 0x1)
return 1;
else
/* rmb to prevent wrong speculation for security */
rmb();
return 0;
}
static inline void set_ibrs_supported(void)
{
use_ibrs |= 0x2;
if (!ibrs_disabled)
set_ibrs_inuse();
}
static inline void set_ibrs_disabled(void)
{
use_ibrs |= 0x4;
if (check_ibrs_inuse())
clear_ibrs_inuse();
}
static inline void clear_ibrs_disabled(void)
{
use_ibrs &= ~0x4;
set_ibrs_inuse();
}
#define ibrs_inuse (check_ibrs_inuse())
/* indicate usage of IBPB to control execution speculation */
extern int use_ibpb;
extern u32 sysctl_ibpb_enabled;
#define ibpb_supported (use_ibpb & 0x2)
#define ibpb_disabled (use_ibpb & 0x4)
static inline void set_ibpb_inuse(void)
{
if (ibpb_supported)
use_ibpb |= 0x1;
}
static inline void clear_ibpb_inuse(void)
{
use_ibpb &= ~0x1;
}
static inline int check_ibpb_inuse(void)
{
if (use_ibpb & 0x1)
return 1;
else
/* rmb to prevent wrong speculation for security */
rmb();
return 0;
}
static inline void set_ibpb_supported(void)
{
use_ibpb |= 0x2;
if (!ibpb_disabled)
set_ibpb_inuse();
}
static inline void set_ibpb_disabled(void)
{
use_ibpb |= 0x4;
if (check_ibpb_inuse())
clear_ibpb_inuse();
}
static inline void clear_ibpb_disabled(void)
{
use_ibpb &= ~0x4;
set_ibpb_inuse();
}
#define ibpb_inuse (check_ibpb_inuse())
#endif
#ifdef CONFIG_SMP #ifdef CONFIG_SMP
#include <linux/preempt.h> #include <linux/preempt.h>
......
...@@ -499,6 +499,26 @@ EXPORT_SYMBOL(smp_call_function); ...@@ -499,6 +499,26 @@ EXPORT_SYMBOL(smp_call_function);
unsigned int setup_max_cpus = NR_CPUS; unsigned int setup_max_cpus = NR_CPUS;
EXPORT_SYMBOL(setup_max_cpus); EXPORT_SYMBOL(setup_max_cpus);
#ifdef CONFIG_X86
/*
* use IBRS
* bit 0 = indicate if ibrs is currently in use
* bit 1 = indicate if system supports ibrs
* bit 2 = indicate if admin disables ibrs
*/
int use_ibrs;
EXPORT_SYMBOL(use_ibrs);
/*
* use IBRS
* bit 0 = indicate if ibpb is currently in use
* bit 1 = indicate if system supports ibpb
* bit 2 = indicate if admin disables ibpb
*/
int use_ibpb;
EXPORT_SYMBOL(use_ibpb);
#endif
/* /*
* Setup routine for controlling SMP activation * Setup routine for controlling SMP activation
...@@ -523,6 +543,27 @@ static int __init nosmp(char *str) ...@@ -523,6 +543,27 @@ static int __init nosmp(char *str)
early_param("nosmp", nosmp); early_param("nosmp", nosmp);
#ifdef CONFIG_X86
static int __init noibrs(char *str)
{
set_ibrs_disabled();
return 0;
}
early_param("noibrs", noibrs);
static int __init noibpb(char *str)
{
set_ibpb_disabled();
return 0;
}
early_param("noibpb", noibpb);
#endif
/* this is hard limit */ /* this is hard limit */
static int __init nrcpus(char *str) static int __init nrcpus(char *str)
{ {
......
...@@ -72,6 +72,7 @@ ...@@ -72,6 +72,7 @@
#include <asm/processor.h> #include <asm/processor.h>
#ifdef CONFIG_X86 #ifdef CONFIG_X86
#include <asm/msr.h>
#include <asm/nmi.h> #include <asm/nmi.h>
#include <asm/stacktrace.h> #include <asm/stacktrace.h>
#include <asm/io.h> #include <asm/io.h>
...@@ -198,6 +199,15 @@ static int proc_dostring_coredump(struct ctl_table *table, int write, ...@@ -198,6 +199,15 @@ static int proc_dostring_coredump(struct ctl_table *table, int write,
void __user *buffer, size_t *lenp, loff_t *ppos); void __user *buffer, size_t *lenp, loff_t *ppos);
#endif #endif
#ifdef CONFIG_X86
int proc_dointvec_ibrs_ctrl(struct ctl_table *table, int write,
void __user *buffer, size_t *lenp, loff_t *ppos);
int proc_dointvec_ibpb_ctrl(struct ctl_table *table, int write,
void __user *buffer, size_t *lenp, loff_t *ppos);
int proc_dointvec_ibrs_dump(struct ctl_table *table, int write,
void __user *buffer, size_t *lenp, loff_t *ppos);
#endif
#ifdef CONFIG_MAGIC_SYSRQ #ifdef CONFIG_MAGIC_SYSRQ
/* Note: sysrq code uses it's own private copy */ /* Note: sysrq code uses it's own private copy */
static int __sysrq_enabled = CONFIG_MAGIC_SYSRQ_DEFAULT_ENABLE; static int __sysrq_enabled = CONFIG_MAGIC_SYSRQ_DEFAULT_ENABLE;
...@@ -234,6 +244,12 @@ extern struct ctl_table epoll_table[]; ...@@ -234,6 +244,12 @@ extern struct ctl_table epoll_table[];
int sysctl_legacy_va_layout; int sysctl_legacy_va_layout;
#endif #endif
u32 sysctl_ibrs_dump = 0;
u32 sysctl_ibrs_enabled = 0;
EXPORT_SYMBOL(sysctl_ibrs_enabled);
u32 sysctl_ibpb_enabled = 0;
EXPORT_SYMBOL(sysctl_ibpb_enabled);
/* The default sysctl tables: */ /* The default sysctl tables: */
static struct ctl_table sysctl_base_table[] = { static struct ctl_table sysctl_base_table[] = {
...@@ -1217,6 +1233,35 @@ static struct ctl_table kern_table[] = { ...@@ -1217,6 +1233,35 @@ static struct ctl_table kern_table[] = {
.extra1 = &one, .extra1 = &one,
.extra2 = &one, .extra2 = &one,
}, },
#endif
#ifdef CONFIG_X86
{
.procname = "ibrs_enabled",
.data = &sysctl_ibrs_enabled,
.maxlen = sizeof(unsigned int),
.mode = 0644,
.proc_handler = proc_dointvec_ibrs_ctrl,
.extra1 = &zero,
.extra2 = &two,
},
{
.procname = "ibpb_enabled",
.data = &sysctl_ibpb_enabled,
.maxlen = sizeof(unsigned int),
.mode = 0644,
.proc_handler = proc_dointvec_ibpb_ctrl,
.extra1 = &zero,
.extra2 = &one,
},
{
.procname = "ibrs_dump",
.data = &sysctl_ibrs_dump,
.maxlen = sizeof(unsigned int),
.mode = 0644,
.proc_handler = proc_dointvec_ibrs_dump,
.extra1 = &zero,
.extra2 = &one,
},
#endif #endif
{ } { }
}; };
...@@ -2375,6 +2420,85 @@ int proc_dointvec_minmax(struct ctl_table *table, int write, ...@@ -2375,6 +2420,85 @@ int proc_dointvec_minmax(struct ctl_table *table, int write,
do_proc_dointvec_minmax_conv, &param); do_proc_dointvec_minmax_conv, &param);
} }
#ifdef CONFIG_X86
int proc_dointvec_ibrs_dump(struct ctl_table *table, int write,
void __user *buffer, size_t *lenp, loff_t *ppos)
{
int ret;
unsigned int cpu;
ret = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
printk("sysctl_ibrs_enabled = %u, sysctl_ibpb_enabled = %u\n", sysctl_ibrs_enabled, sysctl_ibpb_enabled);
printk("use_ibrs = %d, use_ibpb = %d\n", use_ibrs, use_ibpb);
for_each_online_cpu(cpu) {
u64 val;
if (boot_cpu_has(X86_FEATURE_SPEC_CTRL))
rdmsrl_on_cpu(cpu, MSR_IA32_SPEC_CTRL, &val);
else
val = 0;
printk("read cpu %d ibrs val %lu\n", cpu, (unsigned long) val);
}
return ret;
}
int proc_dointvec_ibrs_ctrl(struct ctl_table *table, int write,
void __user *buffer, size_t *lenp, loff_t *ppos)
{
int ret;
unsigned int cpu;
ret = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
pr_debug("sysctl_ibrs_enabled = %u, sysctl_ibpb_enabled = %u\n", sysctl_ibrs_enabled, sysctl_ibpb_enabled);
pr_debug("before:use_ibrs = %d, use_ibpb = %d\n", use_ibrs, use_ibpb);
if (sysctl_ibrs_enabled == 0) {
/* always set IBRS off */
set_ibrs_disabled();
if (ibrs_supported) {
for_each_online_cpu(cpu)
wrmsrl_on_cpu(cpu, MSR_IA32_SPEC_CTRL, 0x0);
}
} else if (sysctl_ibrs_enabled == 2) {
/* always set IBRS on, even in user space */
clear_ibrs_disabled();
if (ibrs_supported) {
for_each_online_cpu(cpu)
wrmsrl_on_cpu(cpu, MSR_IA32_SPEC_CTRL, FEATURE_ENABLE_IBRS);
} else {
sysctl_ibrs_enabled = 0;
}
} else if (sysctl_ibrs_enabled == 1) {
/* use IBRS in kernel */
clear_ibrs_disabled();
if (!ibrs_inuse)
/* platform don't support ibrs */
sysctl_ibrs_enabled = 0;
}
pr_debug("after:use_ibrs = %d, use_ibpb = %d\n", use_ibrs, use_ibpb);
return ret;
}
int proc_dointvec_ibpb_ctrl(struct ctl_table *table, int write,
void __user *buffer, size_t *lenp, loff_t *ppos)
{
int ret;
ret = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
pr_debug("sysctl_ibrs_enabled = %u, sysctl_ibpb_enabled = %u\n", sysctl_ibrs_enabled, sysctl_ibpb_enabled);
pr_debug("before:use_ibrs = %d, use_ibpb = %d\n", use_ibrs, use_ibpb);
if (sysctl_ibpb_enabled == 0)
set_ibpb_disabled();
else if (sysctl_ibpb_enabled == 1) {
clear_ibpb_disabled();
if (!ibpb_inuse)
/* platform don't support ibpb */
sysctl_ibpb_enabled = 0;
}
pr_debug("after:use_ibrs = %d, use_ibpb = %d\n", use_ibrs, use_ibpb);
return ret;
}
#endif
static void validate_coredump_safety(void) static void validate_coredump_safety(void)
{ {
#ifdef CONFIG_COREDUMP #ifdef CONFIG_COREDUMP
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment