Subject: /proc/timer_list and weird behavior with dropbear

Dear Nathan,

I am currently upgrading a kernel for a TI Davinci DM644x based design
from Linux 3.2 to Linux 3.10.1. I have a funny re-producible issue that
goes away by reverting b3956a896ea57f25cacd74708b8fab611543a81d.

The dropbear sshd will open several files after accepting a connection to
generate some entropy. The "/proc/timer_list" file is part of that[1].
It will open that file and will try to read until the end of the file, but
most of the time this end never comes. According to strace the read on the
fd will always return 2048. I have attached the testcase I am using to
re-produce the issue.

Can this be re-produced by anyone else?

holger



[1] https://secure.ucc.asn.au/hg/dropbear/file/028fa77f952f/random.c#l204


Attachments:
(No filename) (738.00 B)
proc_test.c (722.00 B)
Download all attachments

2013-07-19 15:45:17

by Nathan Zimmer

[permalink] [raw]
Subject: Re: /proc/timer_list and weird behavior with dropbear

On 07/19/2013 10:28 AM, Holger Hans Peter Freyther wrote:
> Dear Nathan,
>
> I am currently upgrading a kernel for a TI Davinci DM644x based design
> from Linux 3.2 to Linux 3.10.1. I have a funny re-producible issue that
> goes away by reverting b3956a896ea57f25cacd74708b8fab611543a81d.
>
> The dropbear sshd will open several files after accepting a connection to
> generate some entropy. The "/proc/timer_list" file is part of that[1].
> It will open that file and will try to read until the end of the file, but
> most of the time this end never comes. According to strace the read on the
> fd will always return 2048. I have attached the testcase I am using to
> re-produce the issue.
>
> Can this be re-produced by anyone else?
>
> holger
>
>
>
> [1] https://secure.ucc.asn.au/hg/dropbear/file/028fa77f952f/random.c#l204
>
I hadn't noticed anything.
Let me try your program and see what I may have missed.

Subject: Re: /proc/timer_list and weird behavior with dropbear

On Fri, Jul 19, 2013 at 10:45:15AM -0500, Nathan Zimmer wrote:

> I hadn't noticed anything.
> Let me try your program and see what I may have missed.

Hi,

I neither know the semantics of the timer_list nor how to use
seq_file correctly. What happens is that timer_list_next will only
be called once. This means that iter->cpu will never be increased.

This just moves to the next CPU when stop is called (e.g. nothing
was added once the print_tickdevice was printed). Do you think
this could be correct?



diff --git a/kernel/time/timer_list.c b/kernel/time/timer_list.c
index 3bdf283..8d36a3d 100644
--- a/kernel/time/timer_list.c
+++ b/kernel/time/timer_list.c
@@ -327,8 +327,10 @@ static void *timer_list_next(struct seq_file *file, void *v, loff_t *offset)
return timer_list_start(file, offset);
}

-static void timer_list_stop(struct seq_file *seq, void *v)
+static void timer_list_stop(struct seq_file *file, void *v)
{
+ struct timer_list_iter *iter = file->private;
+ iter->cpu = cpumask_next(iter->cpu, cpu_online_mask);
}

static const struct seq_operations timer_list_sops = {

2013-07-19 19:05:24

by Nathan Zimmer

[permalink] [raw]
Subject: Re: /proc/timer_list and weird behavior with dropbear

On 07/19/2013 12:03 PM, Holger Hans Peter Freyther wrote:
> On Fri, Jul 19, 2013 at 10:45:15AM -0500, Nathan Zimmer wrote:
>
>> I hadn't noticed anything.
>> Let me try your program and see what I may have missed.
> Hi,
>
> I neither know the semantics of the timer_list nor how to use
> seq_file correctly. What happens is that timer_list_next will only
> be called once. This means that iter->cpu will never be increased.
>
> This just moves to the next CPU when stop is called (e.g. nothing
> was added once the print_tickdevice was printed). Do you think
> this could be correct?
>
>
>
> diff --git a/kernel/time/timer_list.c b/kernel/time/timer_list.c
> index 3bdf283..8d36a3d 100644
> --- a/kernel/time/timer_list.c
> +++ b/kernel/time/timer_list.c
> @@ -327,8 +327,10 @@ static void *timer_list_next(struct seq_file *file, void *v, loff_t *offset)
> return timer_list_start(file, offset);
> }
>
> -static void timer_list_stop(struct seq_file *seq, void *v)
> +static void timer_list_stop(struct seq_file *file, void *v)
> {
> + struct timer_list_iter *iter = file->private;
> + iter->cpu = cpumask_next(iter->cpu, cpu_online_mask);
> }
>
> static const struct seq_operations timer_list_sops = {

That certainly does make the issue go away.
I think a better solution would be to have an increment in the
timer_list_start.

2013-07-19 20:33:26

by Nathan Zimmer

[permalink] [raw]
Subject: Re: /proc/timer_list and weird behavior with dropbear

On Fri, Jul 19, 2013 at 07:03:54PM +0200, Holger Hans Peter Freyther wrote:
> On Fri, Jul 19, 2013 at 10:45:15AM -0500, Nathan Zimmer wrote:
>
> > I hadn't noticed anything.
> > Let me try your program and see what I may have missed.
>
> Hi,
>
> I neither know the semantics of the timer_list nor how to use
> seq_file correctly. What happens is that timer_list_next will only
> be called once. This means that iter->cpu will never be increased.
>
> This just moves to the next CPU when stop is called (e.g. nothing
> was added once the print_tickdevice was printed). Do you think
> this could be correct?
>
>
>
> diff --git a/kernel/time/timer_list.c b/kernel/time/timer_list.c
> index 3bdf283..8d36a3d 100644
> --- a/kernel/time/timer_list.c
> +++ b/kernel/time/timer_list.c
> @@ -327,8 +327,10 @@ static void *timer_list_next(struct seq_file *file, void *v, loff_t *offset)
> return timer_list_start(file, offset);
> }
>
> -static void timer_list_stop(struct seq_file *seq, void *v)
> +static void timer_list_stop(struct seq_file *file, void *v)
> {
> + struct timer_list_iter *iter = file->private;
> + iter->cpu = cpumask_next(iter->cpu, cpu_online_mask);
> }
>
> static const struct seq_operations timer_list_sops = {


I think this would be an acceptable fix.
It work file locally. Could you check it out to see if it behaves?

Nate

2013-07-19 20:37:10

by Nathan Zimmer

[permalink] [raw]
Subject: Re: /proc/timer_list and weird behavior with dropbear

On Fri, Jul 19, 2013 at 03:33:24PM -0500, Nathan Zimmer wrote:
> On Fri, Jul 19, 2013 at 07:03:54PM +0200, Holger Hans Peter Freyther wrote:
> > On Fri, Jul 19, 2013 at 10:45:15AM -0500, Nathan Zimmer wrote:
> >
> > > I hadn't noticed anything.
> > > Let me try your program and see what I may have missed.
> >
> > Hi,
> >
> > I neither know the semantics of the timer_list nor how to use
> > seq_file correctly. What happens is that timer_list_next will only
> > be called once. This means that iter->cpu will never be increased.
> >
> > This just moves to the next CPU when stop is called (e.g. nothing
> > was added once the print_tickdevice was printed). Do you think
> > this could be correct?
> >
> >
> >
> > diff --git a/kernel/time/timer_list.c b/kernel/time/timer_list.c
> > index 3bdf283..8d36a3d 100644
> > --- a/kernel/time/timer_list.c
> > +++ b/kernel/time/timer_list.c
> > @@ -327,8 +327,10 @@ static void *timer_list_next(struct seq_file *file, void *v, loff_t *offset)
> > return timer_list_start(file, offset);
> > }
> >
> > -static void timer_list_stop(struct seq_file *seq, void *v)
> > +static void timer_list_stop(struct seq_file *file, void *v)
> > {
> > + struct timer_list_iter *iter = file->private;
> > + iter->cpu = cpumask_next(iter->cpu, cpu_online_mask);
> > }
> >
> > static const struct seq_operations timer_list_sops = {
>
>
> I think this would be an acceptable fix.
> It work file locally. Could you check it out to see if it behaves?
>
> Nate

Forgot the patch last time.
Sorry


Attachments:
(No filename) (1.50 kB)
tlfix.patch (1.02 kB)
Download all attachments
Subject: Re: /proc/timer_list and weird behavior with dropbear

On Fri, Jul 19, 2013 at 03:37:07PM -0500, Nathan Zimmer wrote:

> Forgot the patch last time.
> Sorry

yes, this appears to fix the problem I experience.


thanks
holger

2013-07-22 21:18:41

by Nathan Zimmer

[permalink] [raw]
Subject: [PATCH] timer_list: Correct the show function for timer_list by using iter->now

This patch corrects the issue with /proc/timer_list reported by Holger.
When reading from the proc file with a sufficently small buffer, 2k so not
really that small, there was one could get hung trying to read the file a
chunk at a time.

Signed-off-by Nathan Zimmer <[email protected]>
Reported-by: Holger Hans Peter Freyther <[email protected]>
Cc: <[email protected]> # 3.10.x
Cc: John Stultz <[email protected]>
Cc: Thomas Gleixner <[email protected]>
---
kernel/time/timer_list.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/kernel/time/timer_list.c b/kernel/time/timer_list.c
index 3bdf283..08d7fbd 100644
--- a/kernel/time/timer_list.c
+++ b/kernel/time/timer_list.c
@@ -265,10 +265,9 @@ static inline void timer_list_header(struct seq_file *m, u64 now)
static int timer_list_show(struct seq_file *m, void *v)
{
struct timer_list_iter *iter = v;
- u64 now = ktime_to_ns(ktime_get());

if (iter->cpu == -1 && !iter->second_pass)
- timer_list_header(m, now);
+ timer_list_header(m, iter->now);
else if (!iter->second_pass)
print_cpu(m, iter->cpu, iter->now);
#ifdef CONFIG_GENERIC_CLOCKEVENTS
--
1.8.2.1

Subject: Re: [PATCH] timer_list: Correct the show function for timer_list by using iter->now

On Mon, Jul 22, 2013 at 04:18:31PM -0500, Nathan Zimmer wrote:
> This patch corrects the issue with /proc/timer_list reported by Holger.
> When reading from the proc file with a sufficently small buffer, 2k so not
> really that small, there was one could get hung trying to read the file a
> chunk at a time.

I think it makes sense to always use iter->now but it does not solve
the issue with dropbear/my test case. Or is this meant to be applied
on top of the previous patch?


holger

2013-07-23 22:50:49

by Nathan Zimmer

[permalink] [raw]
Subject: Re: [PATCH] timer_list: Correct the show function for timer_list by using iter->now

On 07/23/2013 02:18 AM, Holger Hans Peter Freyther wrote:
> On Mon, Jul 22, 2013 at 04:18:31PM -0500, Nathan Zimmer wrote:
>> This patch corrects the issue with /proc/timer_list reported by Holger.
>> When reading from the proc file with a sufficently small buffer, 2k so not
>> really that small, there was one could get hung trying to read the file a
>> chunk at a time.
> I think it makes sense to always use iter->now but it does not solve
> the issue with dropbear/my test case. Or is this meant to be applied
> on top of the previous patch?
>
>
> holger
>
Ah sorry I networked booted the wrong kernel and got excited.


However after hitting my head against this for the better part of the
day I found
the root cause of my issue.

In seq_read about halfway down when we flush the buffer to userland.
m->index is advanced without calling m->op->next().
My iterator doesn't take notice of that and thus fails to advance.

/* if not empty - flush it first */
if (m->count) {
n = min(m->count, size);
err = copy_to_user(buf, m->buf + m->from, n);
if (err)
goto Efault;
m->count -= n;
m->from += n;
size -= n;
buf += n;
copied += n;
if (!m->count)
m->index++;
....


I'll need to think about how to correct it.

For the moment probably the right thing would be to revert b3956a896ea5

2013-07-24 14:31:26

by Nathan Zimmer

[permalink] [raw]
Subject: [PATCH] timer_list: Correct the iterator for timer_list

This patch corrects the issue with /proc/timer_list reported by Holger.
When reading from the proc file with a sufficiently small buffer, 2k so not
really that small, there was one could get hung trying to read the file a
chunk at a time.

The timer_list_start function failed to account for the possibility that the
offset was adjusted outside the timer_list_next.

Signed-off-by: Nathan Zimmer <[email protected]>
Reported-by: Holger Hans Peter Freyther <[email protected]>
Cc: <[email protected]> # 3.10.x
Cc: John Stultz <[email protected]>
Cc: Thomas Gleixner <[email protected]>
---
kernel/time/timer_list.c | 41 ++++++++++++++++++++++++-----------------
1 file changed, 24 insertions(+), 17 deletions(-)

diff --git a/kernel/time/timer_list.c b/kernel/time/timer_list.c
index 3bdf283..61ed862 100644
--- a/kernel/time/timer_list.c
+++ b/kernel/time/timer_list.c
@@ -265,10 +265,9 @@ static inline void timer_list_header(struct seq_file *m, u64 now)
static int timer_list_show(struct seq_file *m, void *v)
{
struct timer_list_iter *iter = v;
- u64 now = ktime_to_ns(ktime_get());

if (iter->cpu == -1 && !iter->second_pass)
- timer_list_header(m, now);
+ timer_list_header(m, iter->now);
else if (!iter->second_pass)
print_cpu(m, iter->cpu, iter->now);
#ifdef CONFIG_GENERIC_CLOCKEVENTS
@@ -298,33 +297,41 @@ void sysrq_timer_list_show(void)
return;
}

-static void *timer_list_start(struct seq_file *file, loff_t *offset)
+static void *move_iter(struct timer_list_iter *iter, loff_t offset)
{
- struct timer_list_iter *iter = file->private;
-
- if (!*offset) {
- iter->cpu = -1;
- iter->now = ktime_to_ns(ktime_get());
- } else if (iter->cpu >= nr_cpu_ids) {
+ for (; offset; offset--) {
+ iter->cpu = cpumask_next(iter->cpu, cpu_online_mask);
+ if (iter->cpu >= nr_cpu_ids) {
#ifdef CONFIG_GENERIC_CLOCKEVENTS
- if (!iter->second_pass) {
- iter->cpu = -1;
- iter->second_pass = true;
- } else
- return NULL;
+ if (!iter->second_pass) {
+ iter->cpu = -1;
+ iter->second_pass = true;
+ } else
+ return NULL;
#else
- return NULL;
+ return NULL;
#endif
+ }
}
return iter;
}

+static void *timer_list_start(struct seq_file *file, loff_t *offset)
+{
+ struct timer_list_iter *iter = file->private;
+
+ if (!*offset)
+ iter->now = ktime_to_ns(ktime_get());
+ iter->cpu = -1;
+ iter->second_pass = false;
+ return move_iter(iter, *offset);
+}
+
static void *timer_list_next(struct seq_file *file, void *v, loff_t *offset)
{
struct timer_list_iter *iter = file->private;
- iter->cpu = cpumask_next(iter->cpu, cpu_online_mask);
++*offset;
- return timer_list_start(file, offset);
+ return move_iter(iter, 1);
}

static void timer_list_stop(struct seq_file *seq, void *v)
--
1.8.2.1