All of lore.kernel.org
 help / color / mirror / Atom feed
* /proc/timer_list and weird behavior with dropbear
@ 2013-07-19 15:28 Holger Hans Peter Freyther
  2013-07-19 15:45 ` Nathan Zimmer
  0 siblings, 1 reply; 11+ messages in thread
From: Holger Hans Peter Freyther @ 2013-07-19 15:28 UTC (permalink / raw)
  To: Nathan Zimmer; +Cc: linux-kernel

[-- Attachment #1: Type: text/plain, Size: 738 bytes --]

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


[-- Attachment #2: proc_test.c --]
[-- Type: text/x-csrc, Size: 722 bytes --]

#include <sys/stat.h>
#include <fcntl.h>

       #include <stdio.h>
       #include <stdlib.h>
       #include <sys/time.h>
       #include <sys/types.h>
       #include <unistd.h>


int main(int argc, char **argv)
{
	fd_set rfds;
	struct timeval tv;
	char buf[2048];

	int fd = open("/proc/timer_list", O_RDONLY, O_NONBLOCK);
	if (fd < 0) {
		perror("Failed to open");
		abort();
	}

	FD_ZERO(&rfds);
	FD_SET(fd, &rfds);


	while (1) {
		tv.tv_sec = 2;
		tv.tv_usec = 0;

		int retval = select(fd + 1, &rfds, NULL, NULL, &tv);
		if (retval == -1)
			perror("select()");
		else if (retval) {
			int ret = read(fd, buf, sizeof(buf));
			if (ret == 0)
				return EXIT_SUCCESS;
		} else {
			printf("TIMEOUT...\n");
		}
	}
}

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: /proc/timer_list and weird behavior with dropbear
  2013-07-19 15:28 /proc/timer_list and weird behavior with dropbear Holger Hans Peter Freyther
@ 2013-07-19 15:45 ` Nathan Zimmer
  2013-07-19 17:03   ` Holger Hans Peter Freyther
  0 siblings, 1 reply; 11+ messages in thread
From: Nathan Zimmer @ 2013-07-19 15:45 UTC (permalink / raw)
  To: Holger Hans Peter Freyther; +Cc: linux-kernel

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.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: /proc/timer_list and weird behavior with dropbear
  2013-07-19 15:45 ` Nathan Zimmer
@ 2013-07-19 17:03   ` Holger Hans Peter Freyther
  2013-07-19 19:05     ` Nathan Zimmer
  2013-07-19 20:33     ` Nathan Zimmer
  0 siblings, 2 replies; 11+ messages in thread
From: Holger Hans Peter Freyther @ 2013-07-19 17:03 UTC (permalink / raw)
  To: Nathan Zimmer; +Cc: linux-kernel

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 = {

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: /proc/timer_list and weird behavior with dropbear
  2013-07-19 17:03   ` Holger Hans Peter Freyther
@ 2013-07-19 19:05     ` Nathan Zimmer
  2013-07-19 20:33     ` Nathan Zimmer
  1 sibling, 0 replies; 11+ messages in thread
From: Nathan Zimmer @ 2013-07-19 19:05 UTC (permalink / raw)
  To: Holger Hans Peter Freyther; +Cc: linux-kernel

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.


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: /proc/timer_list and weird behavior with dropbear
  2013-07-19 17:03   ` Holger Hans Peter Freyther
  2013-07-19 19:05     ` Nathan Zimmer
@ 2013-07-19 20:33     ` Nathan Zimmer
  2013-07-19 20:37       ` Nathan Zimmer
  1 sibling, 1 reply; 11+ messages in thread
From: Nathan Zimmer @ 2013-07-19 20:33 UTC (permalink / raw)
  To: Holger Hans Peter Freyther; +Cc: Nathan Zimmer, linux-kernel

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: /proc/timer_list and weird behavior with dropbear
  2013-07-19 20:33     ` Nathan Zimmer
@ 2013-07-19 20:37       ` Nathan Zimmer
  2013-07-20  5:43         ` Holger Hans Peter Freyther
  0 siblings, 1 reply; 11+ messages in thread
From: Nathan Zimmer @ 2013-07-19 20:37 UTC (permalink / raw)
  To: Nathan Zimmer; +Cc: Holger Hans Peter Freyther, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 1537 bytes --]

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


[-- Attachment #2: tlfix.patch --]
[-- Type: text/x-patch, Size: 1041 bytes --]

diff --git a/kernel/time/timer_list.c b/kernel/time/timer_list.c
index 3bdf283..a3620ec 100644
--- a/kernel/time/timer_list.c
+++ b/kernel/time/timer_list.c
@@ -305,24 +305,26 @@ static void *timer_list_start(struct seq_file *file, loff_t *offset)
 	if (!*offset) {
 		iter->cpu = -1;
 		iter->now = ktime_to_ns(ktime_get());
-	} else if (iter->cpu >= nr_cpu_ids) {
+	} else {
+		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_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);
 }

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: /proc/timer_list and weird behavior with dropbear
  2013-07-19 20:37       ` Nathan Zimmer
@ 2013-07-20  5:43         ` Holger Hans Peter Freyther
  2013-07-22 21:18           ` [PATCH] timer_list: Correct the show function for timer_list by using iter->now Nathan Zimmer
  0 siblings, 1 reply; 11+ messages in thread
From: Holger Hans Peter Freyther @ 2013-07-20  5:43 UTC (permalink / raw)
  To: Nathan Zimmer; +Cc: linux-kernel

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH] timer_list: Correct the show function for timer_list by using iter->now
  2013-07-20  5:43         ` Holger Hans Peter Freyther
@ 2013-07-22 21:18           ` Nathan Zimmer
  2013-07-23  7:18             ` Holger Hans Peter Freyther
  0 siblings, 1 reply; 11+ messages in thread
From: Nathan Zimmer @ 2013-07-22 21:18 UTC (permalink / raw)
  To: linux-kernel, holger; +Cc: Nathan Zimmer, stable, John Stultz, Thomas Gleixner

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 <nzimmer@sgi.com>
Reported-by: Holger Hans Peter Freyther <holger@freyther.de>
Cc: <stable@vger.kernel.org> # 3.10.x
Cc: John Stultz <john.stultz@linaro.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
---
 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


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] timer_list: Correct the show function for timer_list by using iter->now
  2013-07-22 21:18           ` [PATCH] timer_list: Correct the show function for timer_list by using iter->now Nathan Zimmer
@ 2013-07-23  7:18             ` Holger Hans Peter Freyther
  2013-07-23 22:50               ` Nathan Zimmer
  2013-07-24 14:31               ` [PATCH] timer_list: Correct the iterator for timer_list Nathan Zimmer
  0 siblings, 2 replies; 11+ messages in thread
From: Holger Hans Peter Freyther @ 2013-07-23  7:18 UTC (permalink / raw)
  To: Nathan Zimmer; +Cc: linux-kernel, stable, John Stultz, Thomas Gleixner

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] timer_list: Correct the show function for timer_list by using iter->now
  2013-07-23  7:18             ` Holger Hans Peter Freyther
@ 2013-07-23 22:50               ` Nathan Zimmer
  2013-07-24 14:31               ` [PATCH] timer_list: Correct the iterator for timer_list Nathan Zimmer
  1 sibling, 0 replies; 11+ messages in thread
From: Nathan Zimmer @ 2013-07-23 22:50 UTC (permalink / raw)
  To: Holger Hans Peter Freyther
  Cc: linux-kernel, stable, John Stultz, Thomas Gleixner

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH] timer_list: Correct the iterator for timer_list
  2013-07-23  7:18             ` Holger Hans Peter Freyther
  2013-07-23 22:50               ` Nathan Zimmer
@ 2013-07-24 14:31               ` Nathan Zimmer
  1 sibling, 0 replies; 11+ messages in thread
From: Nathan Zimmer @ 2013-07-24 14:31 UTC (permalink / raw)
  To: linux-kernel, holger
  Cc: mgalbraith, jkosina, Nathan Zimmer, stable, John Stultz, Thomas Gleixner

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 <nzimmer@sgi.com>
Reported-by: Holger Hans Peter Freyther <holger@freyther.de>
Cc: <stable@vger.kernel.org> # 3.10.x
Cc: John Stultz <john.stultz@linaro.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
---
 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


^ permalink raw reply related	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2013-07-24 14:31 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-07-19 15:28 /proc/timer_list and weird behavior with dropbear Holger Hans Peter Freyther
2013-07-19 15:45 ` Nathan Zimmer
2013-07-19 17:03   ` Holger Hans Peter Freyther
2013-07-19 19:05     ` Nathan Zimmer
2013-07-19 20:33     ` Nathan Zimmer
2013-07-19 20:37       ` Nathan Zimmer
2013-07-20  5:43         ` Holger Hans Peter Freyther
2013-07-22 21:18           ` [PATCH] timer_list: Correct the show function for timer_list by using iter->now Nathan Zimmer
2013-07-23  7:18             ` Holger Hans Peter Freyther
2013-07-23 22:50               ` Nathan Zimmer
2013-07-24 14:31               ` [PATCH] timer_list: Correct the iterator for timer_list Nathan Zimmer

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.