* [PATCH] FIXUP: proc: Provide details on speculation flaw mitigations @ 2019-01-12 0:32 Gwendal Grignou 2019-01-12 8:01 ` Greg KH 0 siblings, 1 reply; 14+ messages in thread From: Gwendal Grignou @ 2019-01-12 0:32 UTC (permalink / raw) To: gregkh; +Cc: stable, groeck, keescook Prevent an empty line in /proc/self/status, allow iotop to work. iotop does not like empty lines, fails with: File "/usr/local/lib64/python2.7/site-packages/iotop/data.py", line 196, in parse_proc_pid_status key, value = line.split(':\t', 1) ValueError: need more than 1 value to unpack [reading /proc/self/status] Signed-off-by: Gwendal Grignou <gwendal@chromium.org> --- fs/proc/array.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/proc/array.c b/fs/proc/array.c index 0c142916a8c7d..f11df9ab4256e 100644 --- a/fs/proc/array.c +++ b/fs/proc/array.c @@ -334,7 +334,7 @@ static inline void task_seccomp(struct seq_file *m, struct task_struct *p) #ifdef CONFIG_SECCOMP seq_printf(m, "Seccomp:\t%d\n", p->seccomp.mode); #endif - seq_printf(m, "\nSpeculation_Store_Bypass:\t"); + seq_printf(m, "Speculation_Store_Bypass:\t"); switch (arch_prctl_spec_ctrl_get(p, PR_SPEC_STORE_BYPASS)) { case -EINVAL: seq_printf(m, "unknown"); -- 2.20.1.97.g81188d93c3-goog ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] FIXUP: proc: Provide details on speculation flaw mitigations 2019-01-12 0:32 [PATCH] FIXUP: proc: Provide details on speculation flaw mitigations Gwendal Grignou @ 2019-01-12 8:01 ` Greg KH 2019-01-14 18:00 ` Gwendal Grignou 0 siblings, 1 reply; 14+ messages in thread From: Greg KH @ 2019-01-12 8:01 UTC (permalink / raw) To: Gwendal Grignou; +Cc: stable, groeck, keescook On Fri, Jan 11, 2019 at 04:32:12PM -0800, Gwendal Grignou wrote: > Prevent an empty line in /proc/self/status, allow iotop to work. > > iotop does not like empty lines, fails with: > File "/usr/local/lib64/python2.7/site-packages/iotop/data.py", line > 196, in parse_proc_pid_status > key, value = line.split(':\t', 1) > ValueError: need more than 1 value to unpack > > [reading /proc/self/status] > > Signed-off-by: Gwendal Grignou <gwendal@chromium.org> > --- > fs/proc/array.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > Why send this to me? Always use scripts/get_maintainer.pl on a patch to determine who and what lists to send patches to. And is this a new bug? What commit caused this? thanks, greg k-h ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] FIXUP: proc: Provide details on speculation flaw mitigations 2019-01-12 8:01 ` Greg KH @ 2019-01-14 18:00 ` Gwendal Grignou 2019-01-14 18:13 ` [PATCH v2] proc: Remove empty line in /proc/self/status Gwendal Grignou 2019-01-14 18:49 ` [PATCH] FIXUP: proc: Provide details on speculation flaw mitigations Greg KH 0 siblings, 2 replies; 14+ messages in thread From: Gwendal Grignou @ 2019-01-14 18:00 UTC (permalink / raw) To: Greg KH; +Cc: stable, groeck, keescook On Sat, Jan 12, 2019 at 12:01 AM Greg KH <gregkh@linuxfoundation.org> wrote: > > On Fri, Jan 11, 2019 at 04:32:12PM -0800, Gwendal Grignou wrote: > > Prevent an empty line in /proc/self/status, allow iotop to work. > > > > iotop does not like empty lines, fails with: > > File "/usr/local/lib64/python2.7/site-packages/iotop/data.py", line > > 196, in parse_proc_pid_status > > key, value = line.split(':\t', 1) > > ValueError: need more than 1 value to unpack > > > > [reading /proc/self/status] > > > > Signed-off-by: Gwendal Grignou <gwendal@chromium.org> > > --- > > fs/proc/array.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > Why send this to me? Always use scripts/get_maintainer.pl on a patch to > determine who and what lists to send patches to. I did, your email address is on the first line: ./scripts/get_maintainer.pl 0001-CHROMIUM-FIXUP-proc-Provide-details-on-speculation-f.patch Greg Kroah-Hartman <gregkh@linuxfoundation.org> (commit_signer:6/4=100%) "Srivatsa S. Bhat" <srivatsa@csail.mit.edu> (commit_signer:3/4=75%) Thomas Gleixner <tglx@linutronix.de> (commit_signer:3/4=75%,authored:1/4=25%,added_lines:3/28=11%) David Woodhouse <dwmw@amazon.co.uk> (commit_signer:3/4=75%) Bo Gan <ganb@vmware.com> (commit_signer:3/4=75%) Kees Cook <keescook@chromium.org> (authored:1/4=25%,added_lines:23/28=82%) Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> (authored:1/4=25%,removed_lines:1/2=50%) Gwendal Grignou <gwendal@chromium.org> (authored:1/4=25%,removed_lines:1/2=50%) linux-kernel@vger.kernel.org (open list) > > And is this a new bug? What commit caused this? It is only in 4.4 stable. It has been introduced by: "484964fa3e5a0 proc: Provide details on speculation flaw mitigations" That patch adds an extra \n in front of "Speculation Store Bypass" that breaks iotop processing of /proc/../status. Regards, Gwendal. > > thanks, > > greg k-h ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2] proc: Remove empty line in /proc/self/status 2019-01-14 18:00 ` Gwendal Grignou @ 2019-01-14 18:13 ` Gwendal Grignou 2019-01-14 18:21 ` Guenter Roeck 2019-01-18 16:10 ` Greg KH 2019-01-14 18:49 ` [PATCH] FIXUP: proc: Provide details on speculation flaw mitigations Greg KH 1 sibling, 2 replies; 14+ messages in thread From: Gwendal Grignou @ 2019-01-14 18:13 UTC (permalink / raw) To: gregkh; +Cc: stable, groeck, keescook Prevent an empty line in /proc/self/status, allow iotop to work. iotop does not like empty lines, fails with: File "/usr/local/lib64/python2.7/site-packages/iotop/data.py", line 196, in parse_proc_pid_status key, value = line.split(':\t', 1) ValueError: need more than 1 value to unpack [reading /proc/self/status] Fixes: 84964fa3e5a0 ("proc: Provide details on speculation flaw mitigations") Signed-off-by: Gwendal Grignou <gwendal@chromium.org> --- v2: Format commit message properly with proper subject and fixes keyword. fs/proc/array.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/proc/array.c b/fs/proc/array.c index 0c142916a8c7d..f11df9ab4256e 100644 --- a/fs/proc/array.c +++ b/fs/proc/array.c @@ -334,7 +334,7 @@ static inline void task_seccomp(struct seq_file *m, struct task_struct *p) #ifdef CONFIG_SECCOMP seq_printf(m, "Seccomp:\t%d\n", p->seccomp.mode); #endif - seq_printf(m, "\nSpeculation_Store_Bypass:\t"); + seq_printf(m, "Speculation_Store_Bypass:\t"); switch (arch_prctl_spec_ctrl_get(p, PR_SPEC_STORE_BYPASS)) { case -EINVAL: seq_printf(m, "unknown"); -- 2.20.1.97.g81188d93c3-goog ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v2] proc: Remove empty line in /proc/self/status 2019-01-14 18:13 ` [PATCH v2] proc: Remove empty line in /proc/self/status Gwendal Grignou @ 2019-01-14 18:21 ` Guenter Roeck 2019-01-14 18:50 ` Greg Kroah-Hartman 2019-01-18 16:10 ` Greg KH 1 sibling, 1 reply; 14+ messages in thread From: Guenter Roeck @ 2019-01-14 18:21 UTC (permalink / raw) To: Gwendal Grignou; +Cc: Greg Kroah-Hartman, # v4 . 10+, Guenter Roeck, Kees Cook On Mon, Jan 14, 2019 at 10:13 AM Gwendal Grignou <gwendal@chromium.org> wrote: > > Prevent an empty line in /proc/self/status, allow iotop to work. > > iotop does not like empty lines, fails with: > File "/usr/local/lib64/python2.7/site-packages/iotop/data.py", line > 196, in parse_proc_pid_status > key, value = line.split(':\t', 1) > ValueError: need more than 1 value to unpack > > [reading /proc/self/status] > > Fixes: 84964fa3e5a0 ("proc: Provide details on speculation flaw mitigations") > > Signed-off-by: Gwendal Grignou <gwendal@chromium.org> > --- > v2: Format commit message properly with proper subject and fixes > keyword. > You might want to mention that this patch only applies to v4.4.y. v4.9.y has a similar problem, but only if CONFIG_SECCOMP=n, and would require a slightly different patch to fix. Other releases are, as far as I can see, not affected. Guenter > fs/proc/array.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/proc/array.c b/fs/proc/array.c > index 0c142916a8c7d..f11df9ab4256e 100644 > --- a/fs/proc/array.c > +++ b/fs/proc/array.c > @@ -334,7 +334,7 @@ static inline void task_seccomp(struct seq_file *m, struct task_struct *p) > #ifdef CONFIG_SECCOMP > seq_printf(m, "Seccomp:\t%d\n", p->seccomp.mode); > #endif > - seq_printf(m, "\nSpeculation_Store_Bypass:\t"); > + seq_printf(m, "Speculation_Store_Bypass:\t"); > switch (arch_prctl_spec_ctrl_get(p, PR_SPEC_STORE_BYPASS)) { > case -EINVAL: > seq_printf(m, "unknown"); > -- > 2.20.1.97.g81188d93c3-goog > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] proc: Remove empty line in /proc/self/status 2019-01-14 18:21 ` Guenter Roeck @ 2019-01-14 18:50 ` Greg Kroah-Hartman 2019-01-14 19:01 ` Guenter Roeck 0 siblings, 1 reply; 14+ messages in thread From: Greg Kroah-Hartman @ 2019-01-14 18:50 UTC (permalink / raw) To: Guenter Roeck; +Cc: Gwendal Grignou, # v4 . 10+, Guenter Roeck, Kees Cook On Mon, Jan 14, 2019 at 10:21:45AM -0800, Guenter Roeck wrote: > On Mon, Jan 14, 2019 at 10:13 AM Gwendal Grignou <gwendal@chromium.org> wrote: > > > > Prevent an empty line in /proc/self/status, allow iotop to work. > > > > iotop does not like empty lines, fails with: > > File "/usr/local/lib64/python2.7/site-packages/iotop/data.py", line > > 196, in parse_proc_pid_status > > key, value = line.split(':\t', 1) > > ValueError: need more than 1 value to unpack > > > > [reading /proc/self/status] > > > > Fixes: 84964fa3e5a0 ("proc: Provide details on speculation flaw mitigations") > > > > Signed-off-by: Gwendal Grignou <gwendal@chromium.org> > > --- > > v2: Format commit message properly with proper subject and fixes > > keyword. > > > You might want to mention that this patch only applies to v4.4.y. > v4.9.y has a similar problem, but only if CONFIG_SECCOMP=n, and would > require a slightly different patch to fix. Other releases are, as far > as I can see, not affected. > > Guenter > > > fs/proc/array.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/fs/proc/array.c b/fs/proc/array.c > > index 0c142916a8c7d..f11df9ab4256e 100644 > > --- a/fs/proc/array.c > > +++ b/fs/proc/array.c > > @@ -334,7 +334,7 @@ static inline void task_seccomp(struct seq_file *m, struct task_struct *p) > > #ifdef CONFIG_SECCOMP > > seq_printf(m, "Seccomp:\t%d\n", p->seccomp.mode); > > #endif > > - seq_printf(m, "\nSpeculation_Store_Bypass:\t"); > > + seq_printf(m, "Speculation_Store_Bypass:\t"); Why isn't this issue showing up in all kernel releases, as this line is still the same in 5.0-rc2? What makes the 4.4.y and 4.9.y trees so special here? confused, greg k-h ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] proc: Remove empty line in /proc/self/status 2019-01-14 18:50 ` Greg Kroah-Hartman @ 2019-01-14 19:01 ` Guenter Roeck 2019-01-14 19:05 ` Kees Cook 0 siblings, 1 reply; 14+ messages in thread From: Guenter Roeck @ 2019-01-14 19:01 UTC (permalink / raw) To: Greg Kroah-Hartman; +Cc: Gwendal Grignou, # v4 . 10+, Guenter Roeck, Kees Cook On Mon, Jan 14, 2019 at 10:50 AM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Mon, Jan 14, 2019 at 10:21:45AM -0800, Guenter Roeck wrote: > > On Mon, Jan 14, 2019 at 10:13 AM Gwendal Grignou <gwendal@chromium.org> wrote: > > > > > > Prevent an empty line in /proc/self/status, allow iotop to work. > > > > > > iotop does not like empty lines, fails with: > > > File "/usr/local/lib64/python2.7/site-packages/iotop/data.py", line > > > 196, in parse_proc_pid_status > > > key, value = line.split(':\t', 1) > > > ValueError: need more than 1 value to unpack > > > > > > [reading /proc/self/status] > > > > > > Fixes: 84964fa3e5a0 ("proc: Provide details on speculation flaw mitigations") > > > > > > Signed-off-by: Gwendal Grignou <gwendal@chromium.org> > > > --- > > > v2: Format commit message properly with proper subject and fixes > > > keyword. > > > > > You might want to mention that this patch only applies to v4.4.y. > > v4.9.y has a similar problem, but only if CONFIG_SECCOMP=n, and would > > require a slightly different patch to fix. Other releases are, as far > > as I can see, not affected. > > > > Guenter > > > > > fs/proc/array.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/fs/proc/array.c b/fs/proc/array.c > > > index 0c142916a8c7d..f11df9ab4256e 100644 > > > --- a/fs/proc/array.c > > > +++ b/fs/proc/array.c > > > @@ -334,7 +334,7 @@ static inline void task_seccomp(struct seq_file *m, struct task_struct *p) > > > #ifdef CONFIG_SECCOMP > > > seq_printf(m, "Seccomp:\t%d\n", p->seccomp.mode); > > > #endif > > > - seq_printf(m, "\nSpeculation_Store_Bypass:\t"); > > > + seq_printf(m, "Speculation_Store_Bypass:\t"); > > Why isn't this issue showing up in all kernel releases, as this line is > still the same in 5.0-rc2? > > What makes the 4.4.y and 4.9.y trees so special here? > v4.14 and later: { seq_put_decimal_ull(m, "NoNewPrivs:\t", task_no_new_privs(p)); #ifdef CONFIG_SECCOMP seq_put_decimal_ull(m, "\nSeccomp:\t", p->seccomp.mode); #endif seq_printf(m, "\nSpeculation_Store_Bypass:\t"); --- v4.9: { #ifdef CONFIG_SECCOMP seq_put_decimal_ull(m, "Seccomp:\t", p->seccomp.mode); ^^^ #endif seq_printf(m, "\nSpeculation_Store_Bypass:\t"); ^^^ -> extra newline if CONFIG_SECCOMP=n --- v4.4: { #ifdef CONFIG_SECCOMP seq_printf(m, "Seccomp:\t%d\n", p->seccomp.mode); ^^^ #endif seq_printf(m, "\nSpeculation_Store_Bypass:\t"); ^^^ -> always extra newline Guenter ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] proc: Remove empty line in /proc/self/status 2019-01-14 19:01 ` Guenter Roeck @ 2019-01-14 19:05 ` Kees Cook 2019-01-16 16:06 ` Guenter Roeck 0 siblings, 1 reply; 14+ messages in thread From: Kees Cook @ 2019-01-14 19:05 UTC (permalink / raw) To: Guenter Roeck Cc: Greg Kroah-Hartman, Gwendal Grignou, # v4 . 10+, Guenter Roeck On Mon, Jan 14, 2019 at 11:01 AM Guenter Roeck <groeck@google.com> wrote: > > On Mon, Jan 14, 2019 at 10:50 AM Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > > > On Mon, Jan 14, 2019 at 10:21:45AM -0800, Guenter Roeck wrote: > > > On Mon, Jan 14, 2019 at 10:13 AM Gwendal Grignou <gwendal@chromium.org> wrote: > > > > > > > > Prevent an empty line in /proc/self/status, allow iotop to work. > > > > > > > > iotop does not like empty lines, fails with: > > > > File "/usr/local/lib64/python2.7/site-packages/iotop/data.py", line > > > > 196, in parse_proc_pid_status > > > > key, value = line.split(':\t', 1) > > > > ValueError: need more than 1 value to unpack > > > > > > > > [reading /proc/self/status] > > > > > > > > Fixes: 84964fa3e5a0 ("proc: Provide details on speculation flaw mitigations") > > > > > > > > Signed-off-by: Gwendal Grignou <gwendal@chromium.org> > > > > --- > > > > v2: Format commit message properly with proper subject and fixes > > > > keyword. > > > > > > > You might want to mention that this patch only applies to v4.4.y. > > > v4.9.y has a similar problem, but only if CONFIG_SECCOMP=n, and would > > > require a slightly different patch to fix. Other releases are, as far > > > as I can see, not affected. > > > > > > Guenter > > > > > > > fs/proc/array.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/fs/proc/array.c b/fs/proc/array.c > > > > index 0c142916a8c7d..f11df9ab4256e 100644 > > > > --- a/fs/proc/array.c > > > > +++ b/fs/proc/array.c > > > > @@ -334,7 +334,7 @@ static inline void task_seccomp(struct seq_file *m, struct task_struct *p) > > > > #ifdef CONFIG_SECCOMP > > > > seq_printf(m, "Seccomp:\t%d\n", p->seccomp.mode); > > > > #endif > > > > - seq_printf(m, "\nSpeculation_Store_Bypass:\t"); > > > > + seq_printf(m, "Speculation_Store_Bypass:\t"); > > > > Why isn't this issue showing up in all kernel releases, as this line is > > still the same in 5.0-rc2? > > > > What makes the 4.4.y and 4.9.y trees so special here? > > > > v4.14 and later: > > { > seq_put_decimal_ull(m, "NoNewPrivs:\t", task_no_new_privs(p)); > #ifdef CONFIG_SECCOMP > seq_put_decimal_ull(m, "\nSeccomp:\t", p->seccomp.mode); > #endif > seq_printf(m, "\nSpeculation_Store_Bypass:\t"); > > --- > v4.9: > > { > #ifdef CONFIG_SECCOMP > seq_put_decimal_ull(m, "Seccomp:\t", p->seccomp.mode); > ^^^ > #endif > seq_printf(m, "\nSpeculation_Store_Bypass:\t"); > ^^^ > > -> extra newline if CONFIG_SECCOMP=n > > --- > v4.4: > > { > #ifdef CONFIG_SECCOMP > seq_printf(m, "Seccomp:\t%d\n", p->seccomp.mode); > ^^^ > #endif > seq_printf(m, "\nSpeculation_Store_Bypass:\t"); > ^^^ > > -> always extra newline > > Guenter Yeah, this grew out of odd placement of the trailing "\n". I agree it needs fixing universally. -- Kees Cook ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] proc: Remove empty line in /proc/self/status 2019-01-14 19:05 ` Kees Cook @ 2019-01-16 16:06 ` Guenter Roeck 2019-01-16 17:00 ` Kees Cook 0 siblings, 1 reply; 14+ messages in thread From: Guenter Roeck @ 2019-01-16 16:06 UTC (permalink / raw) To: Kees Cook; +Cc: Greg Kroah-Hartman, Gwendal Grignou, # v4 . 10+, Guenter Roeck On Mon, Jan 14, 2019 at 11:05 AM Kees Cook <keescook@chromium.org> wrote: > > On Mon, Jan 14, 2019 at 11:01 AM Guenter Roeck <groeck@google.com> wrote: > > > > On Mon, Jan 14, 2019 at 10:50 AM Greg Kroah-Hartman > > <gregkh@linuxfoundation.org> wrote: > > > > > > On Mon, Jan 14, 2019 at 10:21:45AM -0800, Guenter Roeck wrote: > > > > On Mon, Jan 14, 2019 at 10:13 AM Gwendal Grignou <gwendal@chromium.org> wrote: > > > > > > > > > > Prevent an empty line in /proc/self/status, allow iotop to work. > > > > > > > > > > iotop does not like empty lines, fails with: > > > > > File "/usr/local/lib64/python2.7/site-packages/iotop/data.py", line > > > > > 196, in parse_proc_pid_status > > > > > key, value = line.split(':\t', 1) > > > > > ValueError: need more than 1 value to unpack > > > > > > > > > > [reading /proc/self/status] > > > > > > > > > > Fixes: 84964fa3e5a0 ("proc: Provide details on speculation flaw mitigations") > > > > > > > > > > Signed-off-by: Gwendal Grignou <gwendal@chromium.org> > > > > > --- > > > > > v2: Format commit message properly with proper subject and fixes > > > > > keyword. > > > > > > > > > You might want to mention that this patch only applies to v4.4.y. > > > > v4.9.y has a similar problem, but only if CONFIG_SECCOMP=n, and would > > > > require a slightly different patch to fix. Other releases are, as far > > > > as I can see, not affected. > > > > > > > > Guenter > > > > > > > > > fs/proc/array.c | 2 +- > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > diff --git a/fs/proc/array.c b/fs/proc/array.c > > > > > index 0c142916a8c7d..f11df9ab4256e 100644 > > > > > --- a/fs/proc/array.c > > > > > +++ b/fs/proc/array.c > > > > > @@ -334,7 +334,7 @@ static inline void task_seccomp(struct seq_file *m, struct task_struct *p) > > > > > #ifdef CONFIG_SECCOMP > > > > > seq_printf(m, "Seccomp:\t%d\n", p->seccomp.mode); > > > > > #endif > > > > > - seq_printf(m, "\nSpeculation_Store_Bypass:\t"); > > > > > + seq_printf(m, "Speculation_Store_Bypass:\t"); > > > > > > Why isn't this issue showing up in all kernel releases, as this line is > > > still the same in 5.0-rc2? > > > > > > What makes the 4.4.y and 4.9.y trees so special here? > > > > > > > v4.14 and later: > > > > { > > seq_put_decimal_ull(m, "NoNewPrivs:\t", task_no_new_privs(p)); > > #ifdef CONFIG_SECCOMP > > seq_put_decimal_ull(m, "\nSeccomp:\t", p->seccomp.mode); > > #endif > > seq_printf(m, "\nSpeculation_Store_Bypass:\t"); > > > > --- > > v4.9: > > > > { > > #ifdef CONFIG_SECCOMP > > seq_put_decimal_ull(m, "Seccomp:\t", p->seccomp.mode); > > ^^^ > > #endif > > seq_printf(m, "\nSpeculation_Store_Bypass:\t"); > > ^^^ > > > > -> extra newline if CONFIG_SECCOMP=n > > > > --- > > v4.4: > > > > { > > #ifdef CONFIG_SECCOMP > > seq_printf(m, "Seccomp:\t%d\n", p->seccomp.mode); > > ^^^ > > #endif > > seq_printf(m, "\nSpeculation_Store_Bypass:\t"); > > ^^^ > > > > -> always extra newline > > > > Guenter > > Yeah, this grew out of odd placement of the trailing "\n". I agree it > needs fixing universally. > I think we need some guidance on how to fix this problem in 4.4.y and 4.9.y. Backport more of the context patches or stable-release-only patches, possibly with more context explaining the reason ? Guenter ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] proc: Remove empty line in /proc/self/status 2019-01-16 16:06 ` Guenter Roeck @ 2019-01-16 17:00 ` Kees Cook 2019-01-16 17:16 ` Guenter Roeck 0 siblings, 1 reply; 14+ messages in thread From: Kees Cook @ 2019-01-16 17:00 UTC (permalink / raw) To: Guenter Roeck Cc: Greg Kroah-Hartman, Gwendal Grignou, # v4 . 10+, Guenter Roeck On Wed, Jan 16, 2019 at 8:06 AM Guenter Roeck <groeck@google.com> wrote: > > On Mon, Jan 14, 2019 at 11:05 AM Kees Cook <keescook@chromium.org> wrote: > > > > On Mon, Jan 14, 2019 at 11:01 AM Guenter Roeck <groeck@google.com> wrote: > > > > > > On Mon, Jan 14, 2019 at 10:50 AM Greg Kroah-Hartman > > > <gregkh@linuxfoundation.org> wrote: > > > > > > > > On Mon, Jan 14, 2019 at 10:21:45AM -0800, Guenter Roeck wrote: > > > > > On Mon, Jan 14, 2019 at 10:13 AM Gwendal Grignou <gwendal@chromium.org> wrote: > > > > > > > > > > > > Prevent an empty line in /proc/self/status, allow iotop to work. > > > > > > > > > > > > iotop does not like empty lines, fails with: > > > > > > File "/usr/local/lib64/python2.7/site-packages/iotop/data.py", line > > > > > > 196, in parse_proc_pid_status > > > > > > key, value = line.split(':\t', 1) > > > > > > ValueError: need more than 1 value to unpack > > > > > > > > > > > > [reading /proc/self/status] > > > > > > > > > > > > Fixes: 84964fa3e5a0 ("proc: Provide details on speculation flaw mitigations") > > > > > > > > > > > > Signed-off-by: Gwendal Grignou <gwendal@chromium.org> > > > > > > --- > > > > > > v2: Format commit message properly with proper subject and fixes > > > > > > keyword. > > > > > > > > > > > You might want to mention that this patch only applies to v4.4.y. > > > > > v4.9.y has a similar problem, but only if CONFIG_SECCOMP=n, and would > > > > > require a slightly different patch to fix. Other releases are, as far > > > > > as I can see, not affected. > > > > > > > > > > Guenter > > > > > > > > > > > fs/proc/array.c | 2 +- > > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > > > diff --git a/fs/proc/array.c b/fs/proc/array.c > > > > > > index 0c142916a8c7d..f11df9ab4256e 100644 > > > > > > --- a/fs/proc/array.c > > > > > > +++ b/fs/proc/array.c > > > > > > @@ -334,7 +334,7 @@ static inline void task_seccomp(struct seq_file *m, struct task_struct *p) > > > > > > #ifdef CONFIG_SECCOMP > > > > > > seq_printf(m, "Seccomp:\t%d\n", p->seccomp.mode); > > > > > > #endif > > > > > > - seq_printf(m, "\nSpeculation_Store_Bypass:\t"); > > > > > > + seq_printf(m, "Speculation_Store_Bypass:\t"); > > > > > > > > Why isn't this issue showing up in all kernel releases, as this line is > > > > still the same in 5.0-rc2? > > > > > > > > What makes the 4.4.y and 4.9.y trees so special here? > > > > > > > > > > v4.14 and later: > > > > > > { > > > seq_put_decimal_ull(m, "NoNewPrivs:\t", task_no_new_privs(p)); > > > #ifdef CONFIG_SECCOMP > > > seq_put_decimal_ull(m, "\nSeccomp:\t", p->seccomp.mode); > > > #endif > > > seq_printf(m, "\nSpeculation_Store_Bypass:\t"); > > > > > > --- > > > v4.9: > > > > > > { > > > #ifdef CONFIG_SECCOMP > > > seq_put_decimal_ull(m, "Seccomp:\t", p->seccomp.mode); > > > ^^^ > > > #endif > > > seq_printf(m, "\nSpeculation_Store_Bypass:\t"); > > > ^^^ > > > > > > -> extra newline if CONFIG_SECCOMP=n > > > > > > --- > > > v4.4: > > > > > > { > > > #ifdef CONFIG_SECCOMP > > > seq_printf(m, "Seccomp:\t%d\n", p->seccomp.mode); > > > ^^^ > > > #endif > > > seq_printf(m, "\nSpeculation_Store_Bypass:\t"); > > > ^^^ > > > > > > -> always extra newline > > > > > > Guenter > > > > Yeah, this grew out of odd placement of the trailing "\n". I agree it > > needs fixing universally. > > > > I think we need some guidance on how to fix this problem in 4.4.y and > 4.9.y. Backport more of the context patches or stable-release-only > patches, possibly with more context explaining the reason ? I think we need 4.4 and 4.9 specific patches that fix it up, and a patch to Linus that regularizes the "always have a trailing \n" style (since the mixture of newlines was what causes this problem in the first place). -- Kees Cook ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] proc: Remove empty line in /proc/self/status 2019-01-16 17:00 ` Kees Cook @ 2019-01-16 17:16 ` Guenter Roeck 0 siblings, 0 replies; 14+ messages in thread From: Guenter Roeck @ 2019-01-16 17:16 UTC (permalink / raw) To: Kees Cook; +Cc: Greg Kroah-Hartman, Gwendal Grignou, # v4 . 10+, Guenter Roeck On Wed, Jan 16, 2019 at 9:00 AM Kees Cook <keescook@chromium.org> wrote: > > On Wed, Jan 16, 2019 at 8:06 AM Guenter Roeck <groeck@google.com> wrote: > > > > On Mon, Jan 14, 2019 at 11:05 AM Kees Cook <keescook@chromium.org> wrote: > > > > > > On Mon, Jan 14, 2019 at 11:01 AM Guenter Roeck <groeck@google.com> wrote: > > > > > > > > On Mon, Jan 14, 2019 at 10:50 AM Greg Kroah-Hartman > > > > <gregkh@linuxfoundation.org> wrote: > > > > > > > > > > On Mon, Jan 14, 2019 at 10:21:45AM -0800, Guenter Roeck wrote: > > > > > > On Mon, Jan 14, 2019 at 10:13 AM Gwendal Grignou <gwendal@chromium.org> wrote: > > > > > > > > > > > > > > Prevent an empty line in /proc/self/status, allow iotop to work. > > > > > > > > > > > > > > iotop does not like empty lines, fails with: > > > > > > > File "/usr/local/lib64/python2.7/site-packages/iotop/data.py", line > > > > > > > 196, in parse_proc_pid_status > > > > > > > key, value = line.split(':\t', 1) > > > > > > > ValueError: need more than 1 value to unpack > > > > > > > > > > > > > > [reading /proc/self/status] > > > > > > > > > > > > > > Fixes: 84964fa3e5a0 ("proc: Provide details on speculation flaw mitigations") > > > > > > > > > > > > > > Signed-off-by: Gwendal Grignou <gwendal@chromium.org> > > > > > > > --- > > > > > > > v2: Format commit message properly with proper subject and fixes > > > > > > > keyword. > > > > > > > > > > > > > You might want to mention that this patch only applies to v4.4.y. > > > > > > v4.9.y has a similar problem, but only if CONFIG_SECCOMP=n, and would > > > > > > require a slightly different patch to fix. Other releases are, as far > > > > > > as I can see, not affected. > > > > > > > > > > > > Guenter > > > > > > > > > > > > > fs/proc/array.c | 2 +- > > > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > > > > > diff --git a/fs/proc/array.c b/fs/proc/array.c > > > > > > > index 0c142916a8c7d..f11df9ab4256e 100644 > > > > > > > --- a/fs/proc/array.c > > > > > > > +++ b/fs/proc/array.c > > > > > > > @@ -334,7 +334,7 @@ static inline void task_seccomp(struct seq_file *m, struct task_struct *p) > > > > > > > #ifdef CONFIG_SECCOMP > > > > > > > seq_printf(m, "Seccomp:\t%d\n", p->seccomp.mode); > > > > > > > #endif > > > > > > > - seq_printf(m, "\nSpeculation_Store_Bypass:\t"); > > > > > > > + seq_printf(m, "Speculation_Store_Bypass:\t"); > > > > > > > > > > Why isn't this issue showing up in all kernel releases, as this line is > > > > > still the same in 5.0-rc2? > > > > > > > > > > What makes the 4.4.y and 4.9.y trees so special here? > > > > > > > > > > > > > v4.14 and later: > > > > > > > > { > > > > seq_put_decimal_ull(m, "NoNewPrivs:\t", task_no_new_privs(p)); > > > > #ifdef CONFIG_SECCOMP > > > > seq_put_decimal_ull(m, "\nSeccomp:\t", p->seccomp.mode); > > > > #endif > > > > seq_printf(m, "\nSpeculation_Store_Bypass:\t"); > > > > > > > > --- > > > > v4.9: > > > > > > > > { > > > > #ifdef CONFIG_SECCOMP > > > > seq_put_decimal_ull(m, "Seccomp:\t", p->seccomp.mode); > > > > ^^^ > > > > #endif > > > > seq_printf(m, "\nSpeculation_Store_Bypass:\t"); > > > > ^^^ > > > > > > > > -> extra newline if CONFIG_SECCOMP=n > > > > > > > > --- > > > > v4.4: > > > > > > > > { > > > > #ifdef CONFIG_SECCOMP > > > > seq_printf(m, "Seccomp:\t%d\n", p->seccomp.mode); > > > > ^^^ > > > > #endif > > > > seq_printf(m, "\nSpeculation_Store_Bypass:\t"); > > > > ^^^ > > > > > > > > -> always extra newline > > > > > > > > Guenter > > > > > > Yeah, this grew out of odd placement of the trailing "\n". I agree it > > > needs fixing universally. > > > > > > > I think we need some guidance on how to fix this problem in 4.4.y and > > 4.9.y. Backport more of the context patches or stable-release-only > > patches, possibly with more context explaining the reason ? > > I think we need 4.4 and 4.9 specific patches that fix it up, and a > patch to Linus that regularizes the "always have a trailing \n" style > (since the mixture of newlines was what causes this problem in the > first place). > Unfortunately, the lack of trailing newlines is triggered by the use of seq_put_{decimal,hex}_{ull,ll,...}(), which do not add a trailing newline, are used all over the place, and pretty much mandate the use of "\n" at the beginning of the next print statement. I don't think there is an easy way to fix that. Guenter > -- > Kees Cook ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] proc: Remove empty line in /proc/self/status 2019-01-14 18:13 ` [PATCH v2] proc: Remove empty line in /proc/self/status Gwendal Grignou 2019-01-14 18:21 ` Guenter Roeck @ 2019-01-18 16:10 ` Greg KH 1 sibling, 0 replies; 14+ messages in thread From: Greg KH @ 2019-01-18 16:10 UTC (permalink / raw) To: Gwendal Grignou; +Cc: stable, groeck, keescook On Mon, Jan 14, 2019 at 10:13:36AM -0800, Gwendal Grignou wrote: > Prevent an empty line in /proc/self/status, allow iotop to work. > > iotop does not like empty lines, fails with: > File "/usr/local/lib64/python2.7/site-packages/iotop/data.py", line > 196, in parse_proc_pid_status > key, value = line.split(':\t', 1) > ValueError: need more than 1 value to unpack > > [reading /proc/self/status] > > Fixes: 84964fa3e5a0 ("proc: Provide details on speculation flaw mitigations") > > Signed-off-by: Gwendal Grignou <gwendal@chromium.org> > --- > v2: Format commit message properly with proper subject and fixes > keyword. > > fs/proc/array.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Now applied, thanks. greg k-h ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] FIXUP: proc: Provide details on speculation flaw mitigations 2019-01-14 18:00 ` Gwendal Grignou 2019-01-14 18:13 ` [PATCH v2] proc: Remove empty line in /proc/self/status Gwendal Grignou @ 2019-01-14 18:49 ` Greg KH 2019-01-14 21:36 ` Gwendal Grignou 1 sibling, 1 reply; 14+ messages in thread From: Greg KH @ 2019-01-14 18:49 UTC (permalink / raw) To: Gwendal Grignou; +Cc: stable, groeck, keescook On Mon, Jan 14, 2019 at 10:00:54AM -0800, Gwendal Grignou wrote: > On Sat, Jan 12, 2019 at 12:01 AM Greg KH <gregkh@linuxfoundation.org> wrote: > > > > On Fri, Jan 11, 2019 at 04:32:12PM -0800, Gwendal Grignou wrote: > > > Prevent an empty line in /proc/self/status, allow iotop to work. > > > > > > iotop does not like empty lines, fails with: > > > File "/usr/local/lib64/python2.7/site-packages/iotop/data.py", line > > > 196, in parse_proc_pid_status > > > key, value = line.split(':\t', 1) > > > ValueError: need more than 1 value to unpack > > > > > > [reading /proc/self/status] > > > > > > Signed-off-by: Gwendal Grignou <gwendal@chromium.org> > > > --- > > > fs/proc/array.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > Why send this to me? Always use scripts/get_maintainer.pl on a patch to > > determine who and what lists to send patches to. > I did, your email address is on the first line: > ./scripts/get_maintainer.pl > 0001-CHROMIUM-FIXUP-proc-Provide-details-on-speculation-f.patch > Greg Kroah-Hartman <gregkh@linuxfoundation.org> (commit_signer:6/4=100%) Ah, wait, you are making a patch against the stable kernel tree? We can't go back in time, you need to work against Linus's latest kernel tree, that's why I am showing up in this list. I'm not the upstream developer for this file. > "Srivatsa S. Bhat" <srivatsa@csail.mit.edu> (commit_signer:3/4=75%) > Thomas Gleixner <tglx@linutronix.de> > (commit_signer:3/4=75%,authored:1/4=25%,added_lines:3/28=11%) > David Woodhouse <dwmw@amazon.co.uk> (commit_signer:3/4=75%) > Bo Gan <ganb@vmware.com> (commit_signer:3/4=75%) > Kees Cook <keescook@chromium.org> (authored:1/4=25%,added_lines:23/28=82%) > Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > (authored:1/4=25%,removed_lines:1/2=50%) > Gwendal Grignou <gwendal@chromium.org> (authored:1/4=25%,removed_lines:1/2=50%) > linux-kernel@vger.kernel.org (open list) > > > > > And is this a new bug? What commit caused this? > It is only in 4.4 stable. It has been introduced by: > "484964fa3e5a0 proc: Provide details on speculation flaw mitigations" That really is commit fae1fa0fc6cca8beee3ab8ed71d54f9a78fa3f64 in Linus's tree, and is in the 4.4, 4.9, 4.14, 4.16, and 4.17 kernel releases. So it also is included in 5.0-rc1, if this is still an issue there, please submit the patch to the correct developers and then the patch will be backported to the needed stable kernel trees if you add the correct "Fixes:" and "cc: stable@stable.kernel.org" lines to the patch. Documentation/SubmittingPatches will tell you all about how to do this. > That patch adds an extra \n in front of "Speculation Store Bypass" > that breaks iotop processing of /proc/../status. Why isn't iotop broken in the latest kernel release? It seems to work for me here. thanks, greg k-h ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] FIXUP: proc: Provide details on speculation flaw mitigations 2019-01-14 18:49 ` [PATCH] FIXUP: proc: Provide details on speculation flaw mitigations Greg KH @ 2019-01-14 21:36 ` Gwendal Grignou 0 siblings, 0 replies; 14+ messages in thread From: Gwendal Grignou @ 2019-01-14 21:36 UTC (permalink / raw) To: Greg KH; +Cc: stable, groeck, keescook On Mon, Jan 14, 2019 at 10:49 AM Greg KH <gregkh@linuxfoundation.org> wrote: > > On Mon, Jan 14, 2019 at 10:00:54AM -0800, Gwendal Grignou wrote: > > On Sat, Jan 12, 2019 at 12:01 AM Greg KH <gregkh@linuxfoundation.org> wrote: > > > > > > On Fri, Jan 11, 2019 at 04:32:12PM -0800, Gwendal Grignou wrote: > > > > Prevent an empty line in /proc/self/status, allow iotop to work. > > > > > > > > iotop does not like empty lines, fails with: > > > > File "/usr/local/lib64/python2.7/site-packages/iotop/data.py", line > > > > 196, in parse_proc_pid_status > > > > key, value = line.split(':\t', 1) > > > > ValueError: need more than 1 value to unpack > > > > > > > > [reading /proc/self/status] > > > > > > > > Signed-off-by: Gwendal Grignou <gwendal@chromium.org> > > > > --- > > > > fs/proc/array.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > Why send this to me? Always use scripts/get_maintainer.pl on a patch to > > > determine who and what lists to send patches to. > > I did, your email address is on the first line: > > ./scripts/get_maintainer.pl > > 0001-CHROMIUM-FIXUP-proc-Provide-details-on-speculation-f.patch > > Greg Kroah-Hartman <gregkh@linuxfoundation.org> (commit_signer:6/4=100%) > > Ah, wait, you are making a patch against the stable kernel tree? > > We can't go back in time, you need to work against Linus's latest kernel > tree, that's why I am showing up in this list. I'm not the upstream > developer for this file. > > > "Srivatsa S. Bhat" <srivatsa@csail.mit.edu> (commit_signer:3/4=75%) > > Thomas Gleixner <tglx@linutronix.de> > > (commit_signer:3/4=75%,authored:1/4=25%,added_lines:3/28=11%) > > David Woodhouse <dwmw@amazon.co.uk> (commit_signer:3/4=75%) > > Bo Gan <ganb@vmware.com> (commit_signer:3/4=75%) > > Kees Cook <keescook@chromium.org> (authored:1/4=25%,added_lines:23/28=82%) > > Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > > (authored:1/4=25%,removed_lines:1/2=50%) > > Gwendal Grignou <gwendal@chromium.org> (authored:1/4=25%,removed_lines:1/2=50%) > > linux-kernel@vger.kernel.org (open list) > > > > > > > > And is this a new bug? What commit caused this? > > It is only in 4.4 stable. It has been introduced by: > > "484964fa3e5a0 proc: Provide details on speculation flaw mitigations" > > That really is commit fae1fa0fc6cca8beee3ab8ed71d54f9a78fa3f64 in > Linus's tree, and is in the 4.4, 4.9, 4.14, 4.16, and 4.17 kernel > releases. > > So it also is included in 5.0-rc1, if this is still an issue there, > please submit the patch to the correct developers and then the patch > will be backported to the needed stable kernel trees if you add the > correct "Fixes:" and "cc: stable@stable.kernel.org" lines to the patch. > Documentation/SubmittingPatches will tell you all about how to do this. > > > That patch adds an extra \n in front of "Speculation Store Bypass" > > that breaks iotop processing of /proc/../status. > > Why isn't iotop broken in the latest kernel release? It seems to work > for me here. iotop is not broken in ToT, only on 4.4 stable: fae1fa0fc6cca8beee3ab8ed71d54f9a78fa3f64 assumes f7a5f132b447c ("proc: faster /proc/*/status") has been applied. That patch breaks seq_printf(m, "Seccomp:\t%d\n", p->seccomp.mode); in 2: seq_puts(m, "Seccomp:\t"); seq_put_decimal_ull(m, 0, p->seccomp.mode); and seq_putc(m, '\n'); f7a5f132b447c is not applied to 4.4 stable. Comparing fae1fa0fc6 with 484964fa3e5a0, the latter adds a 'seq_putc(m, '\n');' whereas it was already there when fae1fa0fc6 was applied. 484964fa3e5a0 is adding an empty line, the patch I propose removes it. Regards, Gwendal. > > thanks, > > greg k-h ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2019-01-18 16:11 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-01-12 0:32 [PATCH] FIXUP: proc: Provide details on speculation flaw mitigations Gwendal Grignou 2019-01-12 8:01 ` Greg KH 2019-01-14 18:00 ` Gwendal Grignou 2019-01-14 18:13 ` [PATCH v2] proc: Remove empty line in /proc/self/status Gwendal Grignou 2019-01-14 18:21 ` Guenter Roeck 2019-01-14 18:50 ` Greg Kroah-Hartman 2019-01-14 19:01 ` Guenter Roeck 2019-01-14 19:05 ` Kees Cook 2019-01-16 16:06 ` Guenter Roeck 2019-01-16 17:00 ` Kees Cook 2019-01-16 17:16 ` Guenter Roeck 2019-01-18 16:10 ` Greg KH 2019-01-14 18:49 ` [PATCH] FIXUP: proc: Provide details on speculation flaw mitigations Greg KH 2019-01-14 21:36 ` Gwendal Grignou
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).