[PATCH] FIXUP: proc: Provide details on speculation flaw mitigations

[PATCH] FIXUP: proc: Provide details on speculation flaw mitigations

[Gwendal Grignou]

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

Re: [PATCH] FIXUP: proc: Provide details on speculation flaw mitigations

[Greg KH]

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

Re: [PATCH] FIXUP: proc: Provide details on speculation flaw mitigations

[Gwendal Grignou]

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

[PATCH v2] proc: Remove empty line in /proc/self/status

[Gwendal Grignou]

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

Re: [PATCH v2] proc: Remove empty line in /proc/self/status

[Guenter Roeck]

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
>

Re: [PATCH v2] proc: Remove empty line in /proc/self/status

[Greg Kroah-Hartman]

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

Re: [PATCH v2] proc: Remove empty line in /proc/self/status

[Guenter Roeck]

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

Re: [PATCH v2] proc: Remove empty line in /proc/self/status

[Kees Cook]

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

Re: [PATCH v2] proc: Remove empty line in /proc/self/status

[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

Re: [PATCH v2] proc: Remove empty line in /proc/self/status

[Kees Cook]

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

Re: [PATCH v2] proc: Remove empty line in /proc/self/status

[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

Re: [PATCH v2] proc: Remove empty line in /proc/self/status

[Greg KH]

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

Re: [PATCH] FIXUP: proc: Provide details on speculation flaw mitigations

[Greg KH]

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

Re: [PATCH] FIXUP: proc: Provide details on speculation flaw mitigations

[Gwendal Grignou]

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