* [PATCH] Input: mousedev - add a schedule point in mousedev_write() @ 2018-10-04 15:47 Eric Dumazet 2018-10-04 18:59 ` Dmitry Torokhov 0 siblings, 1 reply; 9+ messages in thread From: Eric Dumazet @ 2018-10-04 15:47 UTC (permalink / raw) To: linux-kernel; +Cc: Eric Dumazet, Eric Dumazet, Dmitry Torokhov, linux-input syzbot was able to trigger rcu stalls by calling write() with large number of bytes. Add a cond_resched() in the loop to avoid this. Link: https://lkml.org/lkml/2018/8/23/1106 Signed-off-by: Eric Dumazet <edumazet@google.com> Reported-by: syzbot+9436b02171ac0894d33e@syzkaller.appspotmail.com Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> Cc: linux-input@vger.kernel.org --- drivers/input/mousedev.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/input/mousedev.c b/drivers/input/mousedev.c index e08228061bcdd2f97aaadece31d6c83eb7539ae5..412fa71245afe26a7a8ad75705566f83633ba347 100644 --- a/drivers/input/mousedev.c +++ b/drivers/input/mousedev.c @@ -707,6 +707,7 @@ static ssize_t mousedev_write(struct file *file, const char __user *buffer, mousedev_generate_response(client, c); spin_unlock_irq(&client->packet_lock); + cond_resched(); } kill_fasync(&client->fasync, SIGIO, POLL_IN); -- 2.19.0.605.g01d371f741-goog ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] Input: mousedev - add a schedule point in mousedev_write() 2018-10-04 15:47 [PATCH] Input: mousedev - add a schedule point in mousedev_write() Eric Dumazet @ 2018-10-04 18:59 ` Dmitry Torokhov 2018-10-04 19:28 ` Eric Dumazet 2018-10-04 19:34 ` Paul E. McKenney 0 siblings, 2 replies; 9+ messages in thread From: Dmitry Torokhov @ 2018-10-04 18:59 UTC (permalink / raw) To: Eric Dumazet; +Cc: linux-kernel, Eric Dumazet, linux-input, Paul E. McKenney Hi Eric, On Thu, Oct 04, 2018 at 08:47:49AM -0700, Eric Dumazet wrote: > syzbot was able to trigger rcu stalls by calling write() > with large number of bytes. > > Add a cond_resched() in the loop to avoid this. I think this simply masks a deeper issue. The code fetches characters from userspace in a loop, takes a lock, quickly places response in an output buffer, and releases interrupt. I do not see why this should cause stalls as we do not hold spinlock/interrupts off for extended period of time. Adding Paul so he can straighten me out... > > Link: https://lkml.org/lkml/2018/8/23/1106 > Signed-off-by: Eric Dumazet <edumazet@google.com> > Reported-by: syzbot+9436b02171ac0894d33e@syzkaller.appspotmail.com > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> > Cc: linux-input@vger.kernel.org > --- > drivers/input/mousedev.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/input/mousedev.c b/drivers/input/mousedev.c > index e08228061bcdd2f97aaadece31d6c83eb7539ae5..412fa71245afe26a7a8ad75705566f83633ba347 100644 > --- a/drivers/input/mousedev.c > +++ b/drivers/input/mousedev.c > @@ -707,6 +707,7 @@ static ssize_t mousedev_write(struct file *file, const char __user *buffer, > mousedev_generate_response(client, c); > > spin_unlock_irq(&client->packet_lock); > + cond_resched(); > } > > kill_fasync(&client->fasync, SIGIO, POLL_IN); > -- > 2.19.0.605.g01d371f741-goog > Thanks. -- Dmitry ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Input: mousedev - add a schedule point in mousedev_write() 2018-10-04 18:59 ` Dmitry Torokhov @ 2018-10-04 19:28 ` Eric Dumazet 2018-10-04 19:36 ` Paul E. McKenney 2018-10-04 19:38 ` Dmitry Torokhov 2018-10-04 19:34 ` Paul E. McKenney 1 sibling, 2 replies; 9+ messages in thread From: Eric Dumazet @ 2018-10-04 19:28 UTC (permalink / raw) To: Dmitry Torokhov; +Cc: LKML, Eric Dumazet, linux-input, Paul E. McKenney On Thu, Oct 4, 2018 at 11:59 AM Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > > Hi Eric, > > On Thu, Oct 04, 2018 at 08:47:49AM -0700, Eric Dumazet wrote: > > syzbot was able to trigger rcu stalls by calling write() > > with large number of bytes. > > > > Add a cond_resched() in the loop to avoid this. > > I think this simply masks a deeper issue. The code fetches characters > from userspace in a loop, takes a lock, quickly places response in an > output buffer, and releases interrupt. I do not see why this should > cause stalls as we do not hold spinlock/interrupts off for extended > period of time. > > Adding Paul so he can straighten me out... > Well... write(fd, buffer, 0x7FFF0000); Takes between 20 seconds and 2 minutes depending on CONFIG options .... So either apply my patch, or add a limit on the max count, and possibly break legitimate user space ? I dunno... > > > > Link: https://lkml.org/lkml/2018/8/23/1106 > > Signed-off-by: Eric Dumazet <edumazet@google.com> > > Reported-by: syzbot+9436b02171ac0894d33e@syzkaller.appspotmail.com > > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > Cc: linux-input@vger.kernel.org > > --- > > drivers/input/mousedev.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/input/mousedev.c b/drivers/input/mousedev.c > > index e08228061bcdd2f97aaadece31d6c83eb7539ae5..412fa71245afe26a7a8ad75705566f83633ba347 100644 > > --- a/drivers/input/mousedev.c > > +++ b/drivers/input/mousedev.c > > @@ -707,6 +707,7 @@ static ssize_t mousedev_write(struct file *file, const char __user *buffer, > > mousedev_generate_response(client, c); > > > > spin_unlock_irq(&client->packet_lock); > > + cond_resched(); > > } > > > > kill_fasync(&client->fasync, SIGIO, POLL_IN); > > -- > > 2.19.0.605.g01d371f741-goog > > > > Thanks. > > -- > Dmitry ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Input: mousedev - add a schedule point in mousedev_write() 2018-10-04 19:28 ` Eric Dumazet @ 2018-10-04 19:36 ` Paul E. McKenney 2018-10-04 19:38 ` Dmitry Torokhov 1 sibling, 0 replies; 9+ messages in thread From: Paul E. McKenney @ 2018-10-04 19:36 UTC (permalink / raw) To: Eric Dumazet; +Cc: Dmitry Torokhov, LKML, Eric Dumazet, linux-input On Thu, Oct 04, 2018 at 12:28:56PM -0700, Eric Dumazet wrote: > On Thu, Oct 4, 2018 at 11:59 AM Dmitry Torokhov > <dmitry.torokhov@gmail.com> wrote: > > > > Hi Eric, > > > > On Thu, Oct 04, 2018 at 08:47:49AM -0700, Eric Dumazet wrote: > > > syzbot was able to trigger rcu stalls by calling write() > > > with large number of bytes. > > > > > > Add a cond_resched() in the loop to avoid this. > > > > I think this simply masks a deeper issue. The code fetches characters > > from userspace in a loop, takes a lock, quickly places response in an > > output buffer, and releases interrupt. I do not see why this should > > cause stalls as we do not hold spinlock/interrupts off for extended > > period of time. > > > > Adding Paul so he can straighten me out... > > > > Well... > > write(fd, buffer, 0x7FFF0000); > > Takes between 20 seconds and 2 minutes depending on CONFIG options .... And two minutes would get you an RCU CPU stall warning, even on distro kernels that set the stall-warning time to a full minute (as opposed to 21 seconds in mainline). > So either apply my patch, or add a limit on the max count, and > possibly break legitimate user space ? > > I dunno... I vote for Eric's patch. In fact: Reviewed-by: Paul E. McKenney <paulmck@linux.ibm.com> > > > Link: https://lkml.org/lkml/2018/8/23/1106 > > > Signed-off-by: Eric Dumazet <edumazet@google.com> > > > Reported-by: syzbot+9436b02171ac0894d33e@syzkaller.appspotmail.com > > > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > > Cc: linux-input@vger.kernel.org > > > --- > > > drivers/input/mousedev.c | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/drivers/input/mousedev.c b/drivers/input/mousedev.c > > > index e08228061bcdd2f97aaadece31d6c83eb7539ae5..412fa71245afe26a7a8ad75705566f83633ba347 100644 > > > --- a/drivers/input/mousedev.c > > > +++ b/drivers/input/mousedev.c > > > @@ -707,6 +707,7 @@ static ssize_t mousedev_write(struct file *file, const char __user *buffer, > > > mousedev_generate_response(client, c); > > > > > > spin_unlock_irq(&client->packet_lock); > > > + cond_resched(); > > > } > > > > > > kill_fasync(&client->fasync, SIGIO, POLL_IN); > > > -- > > > 2.19.0.605.g01d371f741-goog > > > > > > > Thanks. > > > > -- > > Dmitry > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Input: mousedev - add a schedule point in mousedev_write() 2018-10-04 19:28 ` Eric Dumazet 2018-10-04 19:36 ` Paul E. McKenney @ 2018-10-04 19:38 ` Dmitry Torokhov 2018-10-04 19:45 ` Eric Dumazet 1 sibling, 1 reply; 9+ messages in thread From: Dmitry Torokhov @ 2018-10-04 19:38 UTC (permalink / raw) To: Eric Dumazet; +Cc: LKML, Eric Dumazet, linux-input, Paul E. McKenney On October 4, 2018 12:28:56 PM PDT, Eric Dumazet <edumazet@google.com> wrote: >On Thu, Oct 4, 2018 at 11:59 AM Dmitry Torokhov ><dmitry.torokhov@gmail.com> wrote: >> >> Hi Eric, >> >> On Thu, Oct 04, 2018 at 08:47:49AM -0700, Eric Dumazet wrote: >> > syzbot was able to trigger rcu stalls by calling write() >> > with large number of bytes. >> > >> > Add a cond_resched() in the loop to avoid this. >> >> I think this simply masks a deeper issue. The code fetches characters >> from userspace in a loop, takes a lock, quickly places response in an >> output buffer, and releases interrupt. I do not see why this should >> cause stalls as we do not hold spinlock/interrupts off for extended >> period of time. >> >> Adding Paul so he can straighten me out... >> > >Well... > >write(fd, buffer, 0x7FFF0000); > >Takes between 20 seconds and 2 minutes depending on CONFIG options .... That's fine even if it takes a couple of years. We are not holding spinlock for the entirety of this time, so we should get bumped off CPU at some point. > >So either apply my patch, or add a limit on the max count, and >possibly break legitimate user space ? Legitimate users write a single character at a time and read response, so exciting after, let's say, 32 bytes would be fine. But I still want to understand why we have to do that. > >I dunno... > >> > >> > Link: https://lkml.org/lkml/2018/8/23/1106 >> > Signed-off-by: Eric Dumazet <edumazet@google.com> >> > Reported-by: syzbot+9436b02171ac0894d33e@syzkaller.appspotmail.com >> > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> >> > Cc: linux-input@vger.kernel.org >> > --- >> > drivers/input/mousedev.c | 1 + >> > 1 file changed, 1 insertion(+) >> > >> > diff --git a/drivers/input/mousedev.c b/drivers/input/mousedev.c >> > index >e08228061bcdd2f97aaadece31d6c83eb7539ae5..412fa71245afe26a7a8ad75705566f83633ba347 >100644 >> > --- a/drivers/input/mousedev.c >> > +++ b/drivers/input/mousedev.c >> > @@ -707,6 +707,7 @@ static ssize_t mousedev_write(struct file >*file, const char __user *buffer, >> > mousedev_generate_response(client, c); >> > >> > spin_unlock_irq(&client->packet_lock); >> > + cond_resched(); >> > } >> > >> > kill_fasync(&client->fasync, SIGIO, POLL_IN); >> > -- >> > 2.19.0.605.g01d371f741-goog >> > >> >> Thanks. >> >> -- >> Dmitry Thanks. -- Dmitry ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Input: mousedev - add a schedule point in mousedev_write() 2018-10-04 19:38 ` Dmitry Torokhov @ 2018-10-04 19:45 ` Eric Dumazet 0 siblings, 0 replies; 9+ messages in thread From: Eric Dumazet @ 2018-10-04 19:45 UTC (permalink / raw) To: Dmitry Torokhov; +Cc: LKML, Eric Dumazet, linux-input, Paul E. McKenney On Thu, Oct 4, 2018 at 12:38 PM Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > > On October 4, 2018 12:28:56 PM PDT, Eric Dumazet <edumazet@google.com> wrote: > >On Thu, Oct 4, 2018 at 11:59 AM Dmitry Torokhov > ><dmitry.torokhov@gmail.com> wrote: > >> > >> Hi Eric, > >> > >> On Thu, Oct 04, 2018 at 08:47:49AM -0700, Eric Dumazet wrote: > >> > syzbot was able to trigger rcu stalls by calling write() > >> > with large number of bytes. > >> > > >> > Add a cond_resched() in the loop to avoid this. > >> > >> I think this simply masks a deeper issue. The code fetches characters > >> from userspace in a loop, takes a lock, quickly places response in an > >> output buffer, and releases interrupt. I do not see why this should > >> cause stalls as we do not hold spinlock/interrupts off for extended > >> period of time. > >> > >> Adding Paul so he can straighten me out... > >> > > > >Well... > > > >write(fd, buffer, 0x7FFF0000); > > > >Takes between 20 seconds and 2 minutes depending on CONFIG options .... > > That's fine even if it takes a couple of years. We are not holding spinlock for the entirety of this time, so we should get bumped off CPU at some point. Well, you are saying that we could get rid of all cond_resched() calls in the kernel. You should send patches asap ;) > > > > >So either apply my patch, or add a limit on the max count, and > >possibly break legitimate user space ? > > Legitimate users write a single character at a time and read response, so exciting after, let's say, 32 bytes would be fine. But I still want to understand why we have to do that. > > > > >I dunno... > > > >> > > >> > Link: https://lkml.org/lkml/2018/8/23/1106 > >> > Signed-off-by: Eric Dumazet <edumazet@google.com> > >> > Reported-by: syzbot+9436b02171ac0894d33e@syzkaller.appspotmail.com > >> > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> > >> > Cc: linux-input@vger.kernel.org > >> > --- > >> > drivers/input/mousedev.c | 1 + > >> > 1 file changed, 1 insertion(+) > >> > > >> > diff --git a/drivers/input/mousedev.c b/drivers/input/mousedev.c > >> > index > >e08228061bcdd2f97aaadece31d6c83eb7539ae5..412fa71245afe26a7a8ad75705566f83633ba347 > >100644 > >> > --- a/drivers/input/mousedev.c > >> > +++ b/drivers/input/mousedev.c > >> > @@ -707,6 +707,7 @@ static ssize_t mousedev_write(struct file > >*file, const char __user *buffer, > >> > mousedev_generate_response(client, c); > >> > > >> > spin_unlock_irq(&client->packet_lock); > >> > + cond_resched(); > >> > } > >> > > >> > kill_fasync(&client->fasync, SIGIO, POLL_IN); > >> > -- > >> > 2.19.0.605.g01d371f741-goog > >> > > >> > >> Thanks. > >> > >> -- > >> Dmitry > > > Thanks. > > -- > Dmitry ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Input: mousedev - add a schedule point in mousedev_write() 2018-10-04 18:59 ` Dmitry Torokhov 2018-10-04 19:28 ` Eric Dumazet @ 2018-10-04 19:34 ` Paul E. McKenney 2018-10-04 22:54 ` Dmitry Torokhov 1 sibling, 1 reply; 9+ messages in thread From: Paul E. McKenney @ 2018-10-04 19:34 UTC (permalink / raw) To: Dmitry Torokhov; +Cc: Eric Dumazet, linux-kernel, Eric Dumazet, linux-input On Thu, Oct 04, 2018 at 11:59:49AM -0700, Dmitry Torokhov wrote: > Hi Eric, > > On Thu, Oct 04, 2018 at 08:47:49AM -0700, Eric Dumazet wrote: > > syzbot was able to trigger rcu stalls by calling write() > > with large number of bytes. > > > > Add a cond_resched() in the loop to avoid this. > > I think this simply masks a deeper issue. The code fetches characters > from userspace in a loop, takes a lock, quickly places response in an > output buffer, and releases interrupt. I do not see why this should > cause stalls as we do not hold spinlock/interrupts off for extended > period of time. > > Adding Paul so he can straighten me out... If you are running a !PREEMPT kernel, then you need the cond_resched() to allow the scheduler to choose someone else to run if needed and to let RCU know that grace periods can end. Without the cond_resched(), if you stay in that loop long enough you will get excessive scheduling latencies and eventually even RCU CPU stall warning splats. In a PREEMPT (instead of !PREEMPT) kernel, you would be right. When preemption is enabled, the scheduler can preempt and RCU can sense lack of readers from the scheduling-clock interrupt handler. Which is why cond_resched() is nothingness in a PREEMPT kernel. But because people run !PREEMPT as well as PREEMPT kernels, if that loop can run for a long time, you need that cond_resched(). Thanx, Paul > > Link: https://lkml.org/lkml/2018/8/23/1106 > > Signed-off-by: Eric Dumazet <edumazet@google.com> > > Reported-by: syzbot+9436b02171ac0894d33e@syzkaller.appspotmail.com > > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > Cc: linux-input@vger.kernel.org > > --- > > drivers/input/mousedev.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/input/mousedev.c b/drivers/input/mousedev.c > > index e08228061bcdd2f97aaadece31d6c83eb7539ae5..412fa71245afe26a7a8ad75705566f83633ba347 100644 > > --- a/drivers/input/mousedev.c > > +++ b/drivers/input/mousedev.c > > @@ -707,6 +707,7 @@ static ssize_t mousedev_write(struct file *file, const char __user *buffer, > > mousedev_generate_response(client, c); > > > > spin_unlock_irq(&client->packet_lock); > > + cond_resched(); > > } > > > > kill_fasync(&client->fasync, SIGIO, POLL_IN); > > -- > > 2.19.0.605.g01d371f741-goog > > > > Thanks. > > -- > Dmitry > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Input: mousedev - add a schedule point in mousedev_write() 2018-10-04 19:34 ` Paul E. McKenney @ 2018-10-04 22:54 ` Dmitry Torokhov 2018-10-04 23:01 ` Eric Dumazet 0 siblings, 1 reply; 9+ messages in thread From: Dmitry Torokhov @ 2018-10-04 22:54 UTC (permalink / raw) To: Paul E. McKenney; +Cc: Eric Dumazet, linux-kernel, Eric Dumazet, linux-input On Thu, Oct 04, 2018 at 12:34:07PM -0700, Paul E. McKenney wrote: > On Thu, Oct 04, 2018 at 11:59:49AM -0700, Dmitry Torokhov wrote: > > Hi Eric, > > > > On Thu, Oct 04, 2018 at 08:47:49AM -0700, Eric Dumazet wrote: > > > syzbot was able to trigger rcu stalls by calling write() > > > with large number of bytes. > > > > > > Add a cond_resched() in the loop to avoid this. > > > > I think this simply masks a deeper issue. The code fetches characters > > from userspace in a loop, takes a lock, quickly places response in an > > output buffer, and releases interrupt. I do not see why this should > > cause stalls as we do not hold spinlock/interrupts off for extended > > period of time. > > > > Adding Paul so he can straighten me out... > > If you are running a !PREEMPT kernel, then you need the cond_resched() > to allow the scheduler to choose someone else to run if needed and > to let RCU know that grace periods can end. Without the cond_resched(), > if you stay in that loop long enough you will get excessive scheduling > latencies and eventually even RCU CPU stall warning splats. > > In a PREEMPT (instead of !PREEMPT) kernel, you would be right. When > preemption is enabled, the scheduler can preempt and RCU can sense > lack of readers from the scheduling-clock interrupt handler. Which > is why cond_resched() is nothingness in a PREEMPT kernel. > > But because people run !PREEMPT as well as PREEMPT kernels, if that loop > can run for a long time, you need that cond_resched(). OK, I see. I'll apply the patch then. I think evdev.c needs similar treatment as it will keep looping while there is data... Thanks. -- Dmitry ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Input: mousedev - add a schedule point in mousedev_write() 2018-10-04 22:54 ` Dmitry Torokhov @ 2018-10-04 23:01 ` Eric Dumazet 0 siblings, 0 replies; 9+ messages in thread From: Eric Dumazet @ 2018-10-04 23:01 UTC (permalink / raw) To: Dmitry Torokhov, Paul E. McKenney Cc: Eric Dumazet, linux-kernel, Eric Dumazet, linux-input On 10/04/2018 03:54 PM, Dmitry Torokhov wrote: > OK, I see. I'll apply the patch then. Thanks ! > > I think evdev.c needs similar treatment as it will keep looping while > there is data... Yeah, presumably other drivers need care as well :/ ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2018-10-04 23:01 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-10-04 15:47 [PATCH] Input: mousedev - add a schedule point in mousedev_write() Eric Dumazet 2018-10-04 18:59 ` Dmitry Torokhov 2018-10-04 19:28 ` Eric Dumazet 2018-10-04 19:36 ` Paul E. McKenney 2018-10-04 19:38 ` Dmitry Torokhov 2018-10-04 19:45 ` Eric Dumazet 2018-10-04 19:34 ` Paul E. McKenney 2018-10-04 22:54 ` Dmitry Torokhov 2018-10-04 23:01 ` Eric Dumazet
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).