* [PATCH] add lock proctect to __handle_sysrq in write_sysrq_trigger @ 2020-02-07 7:56 Shen Kai 2020-02-07 8:10 ` Greg KH 0 siblings, 1 reply; 4+ messages in thread From: Shen Kai @ 2020-02-07 7:56 UTC (permalink / raw) To: gregkh, jslaby; +Cc: linux-kernel, hushiyuan, linfeilong From: Feilong Lin <linfeilong@huawei.com> Add lock protect to __handle_sysrq to avoid race condition. __handle_sysrq will change console_loglevel without lock protect which can lead to console_loglevel to be set as an unexpected value. Problem may occur when "echo t > /proc/sysrq-trigger" is called on multiple cpus concurrently. In this case in __handle_sysrq, console_loglevel is set to 7 to print some head info to the console then restore it. But without lock protect in parallel execution situation, restoring may go wrong. The new loglevel may be taken as the previous loglevel incorrectly. Console_loglevel can be 7 at last, which causes the terminal to output info in most log levels. This bug was found on linux 4.19 Signed-off-by: Feilong Lin <linfeilong@huawei.com> Reported-by: Kai Shen <shenkai8@huawei.com> --- drivers/tty/sysrq.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c index f724962..cbb48a9 100644 --- a/drivers/tty/sysrq.c +++ b/drivers/tty/sysrq.c @@ -1087,6 +1087,8 @@ EXPORT_SYMBOL(unregister_sysrq_key); /* * writing 'C' to /proc/sysrq-trigger is like sysrq-C */ +static DEFINE_MUTEX(sysrq_mutex); + static ssize_t write_sysrq_trigger(struct file *file, const char __user *buf, size_t count, loff_t *ppos) { @@ -1095,7 +1097,9 @@ static ssize_t write_sysrq_trigger(struct file *file, const char __user *buf, if (get_user(c, buf)) return -EFAULT; + mutex_lock(&sysrq_mutex); __handle_sysrq(c, false); + mutex_unlock(&sysrq_mutex); } return count; -- 2.6.4.windows.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] add lock proctect to __handle_sysrq in write_sysrq_trigger 2020-02-07 7:56 [PATCH] add lock proctect to __handle_sysrq in write_sysrq_trigger Shen Kai @ 2020-02-07 8:10 ` Greg KH 2020-02-07 9:13 ` shenkai 0 siblings, 1 reply; 4+ messages in thread From: Greg KH @ 2020-02-07 8:10 UTC (permalink / raw) To: Shen Kai; +Cc: jslaby, linux-kernel, hushiyuan, linfeilong On Fri, Feb 07, 2020 at 07:56:06AM +0000, Shen Kai wrote: > From: Feilong Lin <linfeilong@huawei.com> > > Add lock protect to __handle_sysrq to avoid race condition. > __handle_sysrq will change console_loglevel without lock protect > which can lead to console_loglevel to be set as an unexpected value. > > Problem may occur when "echo t > /proc/sysrq-trigger" is called on > multiple cpus concurrently. > > In this case in __handle_sysrq, console_loglevel is set to 7 to print > some head info to the console then restore it. But without lock protect > in parallel execution situation, restoring may go wrong. The new > loglevel may be taken as the previous loglevel incorrectly. > Console_loglevel can be 7 at last, which causes the terminal to output > info in most log levels. > > This bug was found on linux 4.19 > > Signed-off-by: Feilong Lin <linfeilong@huawei.com> > Reported-by: Kai Shen <shenkai8@huawei.com> > --- > drivers/tty/sysrq.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c > index f724962..cbb48a9 100644 > --- a/drivers/tty/sysrq.c > +++ b/drivers/tty/sysrq.c > @@ -1087,6 +1087,8 @@ EXPORT_SYMBOL(unregister_sysrq_key); > /* > * writing 'C' to /proc/sysrq-trigger is like sysrq-C > */ > +static DEFINE_MUTEX(sysrq_mutex); > + > static ssize_t write_sysrq_trigger(struct file *file, const char __user *buf, > size_t count, loff_t *ppos) > { > @@ -1095,7 +1097,9 @@ static ssize_t write_sysrq_trigger(struct file *file, const char __user *buf, > > if (get_user(c, buf)) > return -EFAULT; > + mutex_lock(&sysrq_mutex); > __handle_sysrq(c, false); > + mutex_unlock(&sysrq_mutex); What exactly are you protecting here? What other task is doing this at the same exact time? You mention different tasks hitting this sysrq-trigger at the same time, but really, "just do not do that" should be the real answer, as even with this lock, you don't know what the end result will be as the "last" one in will have the last word, right? thanks, greg k-h ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] add lock proctect to __handle_sysrq in write_sysrq_trigger 2020-02-07 8:10 ` Greg KH @ 2020-02-07 9:13 ` shenkai 2020-02-07 9:53 ` Greg KH 0 siblings, 1 reply; 4+ messages in thread From: shenkai @ 2020-02-07 9:13 UTC (permalink / raw) To: Greg KH; +Cc: jslaby, linux-kernel, hushiyuan, linfeilong On 2020/2/7 16:10, Greg KH wrote: > On Fri, Feb 07, 2020 at 07:56:06AM +0000, Shen Kai wrote: >> From: Feilong Lin <linfeilong@huawei.com> >> >> Add lock protect to __handle_sysrq to avoid race condition. >> __handle_sysrq will change console_loglevel without lock protect >> which can lead to console_loglevel to be set as an unexpected value. >> >> Problem may occur when "echo t > /proc/sysrq-trigger" is called on >> multiple cpus concurrently. >> >> In this case in __handle_sysrq, console_loglevel is set to 7 to print >> some head info to the console then restore it. But without lock protect >> in parallel execution situation, restoring may go wrong. The new >> loglevel may be taken as the previous loglevel incorrectly. >> Console_loglevel can be 7 at last, which causes the terminal to output >> info in most log levels. >> >> This bug was found on linux 4.19 >> >> Signed-off-by: Feilong Lin <linfeilong@huawei.com> >> Reported-by: Kai Shen <shenkai8@huawei.com> >> --- >> drivers/tty/sysrq.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c >> index f724962..cbb48a9 100644 >> --- a/drivers/tty/sysrq.c >> +++ b/drivers/tty/sysrq.c >> @@ -1087,6 +1087,8 @@ EXPORT_SYMBOL(unregister_sysrq_key); >> /* >> * writing 'C' to /proc/sysrq-trigger is like sysrq-C >> */ >> +static DEFINE_MUTEX(sysrq_mutex); >> + >> static ssize_t write_sysrq_trigger(struct file *file, const char __user *buf, >> size_t count, loff_t *ppos) >> { >> @@ -1095,7 +1097,9 @@ static ssize_t write_sysrq_trigger(struct file *file, const char __user *buf, >> >> if (get_user(c, buf)) >> return -EFAULT; >> + mutex_lock(&sysrq_mutex); >> __handle_sysrq(c, false); >> + mutex_unlock(&sysrq_mutex); > > What exactly are you protecting here? What other task is doing this at > the same exact time? > > You mention different tasks hitting this sysrq-trigger at the same time, > but really, "just do not do that" should be the real answer, as even > with this lock, you don't know what the end result will be as the "last" > one in will have the last word, right? > > thanks, > > greg k-h > > . > Here we want to protect the global variable console_loglevel (console_printk[0]). Problem may occur when run shell programs like: echo t > /proc/sysrq-trigger & echo t > /proc/sysrq-trigger & echo t > /proc/sysrq-trigger & .. After above operations are done, console_loglevel may be 7 instead of the original log level. I doubt this is what we expect though those operations may not be meaningful. In this case, much info may be output to the terminal for stack info of all threads is a lot to print which may cause soft lockup on a non-preempt kernel. Best regards Kai ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] add lock proctect to __handle_sysrq in write_sysrq_trigger 2020-02-07 9:13 ` shenkai @ 2020-02-07 9:53 ` Greg KH 0 siblings, 0 replies; 4+ messages in thread From: Greg KH @ 2020-02-07 9:53 UTC (permalink / raw) To: shenkai; +Cc: jslaby, linux-kernel, hushiyuan, linfeilong On Fri, Feb 07, 2020 at 05:13:57PM +0800, shenkai wrote: > > On 2020/2/7 16:10, Greg KH wrote: > > On Fri, Feb 07, 2020 at 07:56:06AM +0000, Shen Kai wrote: > > > From: Feilong Lin <linfeilong@huawei.com> > > > > > > Add lock protect to __handle_sysrq to avoid race condition. > > > __handle_sysrq will change console_loglevel without lock protect > > > which can lead to console_loglevel to be set as an unexpected value. > > > > > > Problem may occur when "echo t > /proc/sysrq-trigger" is called on > > > multiple cpus concurrently. > > > > > > In this case in __handle_sysrq, console_loglevel is set to 7 to print > > > some head info to the console then restore it. But without lock protect > > > in parallel execution situation, restoring may go wrong. The new > > > loglevel may be taken as the previous loglevel incorrectly. > > > Console_loglevel can be 7 at last, which causes the terminal to output > > > info in most log levels. > > > > > > This bug was found on linux 4.19 > > > > > > Signed-off-by: Feilong Lin <linfeilong@huawei.com> > > > Reported-by: Kai Shen <shenkai8@huawei.com> > > > --- > > > drivers/tty/sysrq.c | 4 ++++ > > > 1 file changed, 4 insertions(+) > > > > > > diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c > > > index f724962..cbb48a9 100644 > > > --- a/drivers/tty/sysrq.c > > > +++ b/drivers/tty/sysrq.c > > > @@ -1087,6 +1087,8 @@ EXPORT_SYMBOL(unregister_sysrq_key); > > > /* > > > * writing 'C' to /proc/sysrq-trigger is like sysrq-C > > > */ > > > +static DEFINE_MUTEX(sysrq_mutex); > > > + > > > static ssize_t write_sysrq_trigger(struct file *file, const char __user *buf, > > > size_t count, loff_t *ppos) > > > { > > > @@ -1095,7 +1097,9 @@ static ssize_t write_sysrq_trigger(struct file *file, const char __user *buf, > > > if (get_user(c, buf)) > > > return -EFAULT; > > > + mutex_lock(&sysrq_mutex); > > > __handle_sysrq(c, false); > > > + mutex_unlock(&sysrq_mutex); > > > > What exactly are you protecting here? What other task is doing this at > > the same exact time? > > > > You mention different tasks hitting this sysrq-trigger at the same time, > > but really, "just do not do that" should be the real answer, as even > > with this lock, you don't know what the end result will be as the "last" > > one in will have the last word, right? > > > > thanks, > > > > greg k-h > > > > . > > > > Here we want to protect the global variable console_loglevel > (console_printk[0]). But how is this single lock protecting it? > Problem may occur when run shell programs like: > > echo t > /proc/sysrq-trigger & > echo t > /proc/sysrq-trigger & > echo t > /proc/sysrq-trigger & > .. Don't do that :) > After above operations are done, console_loglevel may be 7 instead of the > original log level. I doubt this is what we expect though those operations > may not be meaningful. > > In this case, much info may be output to the terminal for stack info of all > threads is a lot to print which may cause soft lockup on a non-preempt > kernel. Dumping loads of stuff to the console is what you asked the above things to do. And why would you run non-preempt? Anyway, this feels like you are not addressing the real issue and instead papering over it by just trying to serialize the sysrq trigger, which is not something that we always need to do. thanks, greg k-h ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-02-07 9:53 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-02-07 7:56 [PATCH] add lock proctect to __handle_sysrq in write_sysrq_trigger Shen Kai 2020-02-07 8:10 ` Greg KH 2020-02-07 9:13 ` shenkai 2020-02-07 9:53 ` Greg KH
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).