Linux-Fsdevel Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] fanotify: Avoid softlockups when reading many events
@ 2020-07-15 12:29 Jan Kara
  2020-07-15 12:34 ` Amir Goldstein
  0 siblings, 1 reply; 3+ messages in thread
From: Jan Kara @ 2020-07-15 12:29 UTC (permalink / raw)
  To: linux-fsdevel; +Cc: Amir Goldstein, Francesco Ruggeri, Jan Kara

When user provides large buffer for events and there are lots of events
available, we can try to copy them all to userspace without scheduling
which can softlockup the kernel (furthermore exacerbated by the
contention on notification_lock). Add a scheduling point after copying
each event.

Note that usually the real underlying problem is the cost of fanotify
event merging and the resulting contention on notification_lock but this
is a cheap way to somewhat reduce the problem until we can properly
address that.

Reported-by: Francesco Ruggeri <fruggeri@arista.com>
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/notify/fanotify/fanotify_user.c | 5 +++++
 1 file changed, 5 insertions(+)

This is a quick mending we can do immediately and is probably a good idea
nevertheless... I'll queue it up if Amir agrees.

diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index 63b5dffdca9e..d7f63aeca992 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -412,6 +412,11 @@ static ssize_t fanotify_read(struct file *file, char __user *buf,
 
 	add_wait_queue(&group->notification_waitq, &wait);
 	while (1) {
+		/*
+		 * User can supply arbitrarily large buffer. Avoid softlockups
+		 * in case there are lots of available events.
+		 */
+		cond_resched();
 		event = get_one_event(group, count);
 		if (IS_ERR(event)) {
 			ret = PTR_ERR(event);
-- 
2.16.4


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

* Re: [PATCH] fanotify: Avoid softlockups when reading many events
  2020-07-15 12:29 [PATCH] fanotify: Avoid softlockups when reading many events Jan Kara
@ 2020-07-15 12:34 ` Amir Goldstein
  2020-07-15 13:24   ` Jan Kara
  0 siblings, 1 reply; 3+ messages in thread
From: Amir Goldstein @ 2020-07-15 12:34 UTC (permalink / raw)
  To: Jan Kara; +Cc: linux-fsdevel, Francesco Ruggeri

On Wed, Jul 15, 2020 at 3:29 PM Jan Kara <jack@suse.cz> wrote:
>
> When user provides large buffer for events and there are lots of events
> available, we can try to copy them all to userspace without scheduling
> which can softlockup the kernel (furthermore exacerbated by the
> contention on notification_lock). Add a scheduling point after copying
> each event.
>
> Note that usually the real underlying problem is the cost of fanotify
> event merging and the resulting contention on notification_lock but this
> is a cheap way to somewhat reduce the problem until we can properly
> address that.
>
> Reported-by: Francesco Ruggeri <fruggeri@arista.com>
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  fs/notify/fanotify/fanotify_user.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> This is a quick mending we can do immediately and is probably a good idea
> nevertheless... I'll queue it up if Amir agrees.
>

Sure. fine by me.
Maybe add a lore link to the issue report.

Reviewed-by: Amir Goldstein <amir73il@gmail.com>

> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> index 63b5dffdca9e..d7f63aeca992 100644
> --- a/fs/notify/fanotify/fanotify_user.c
> +++ b/fs/notify/fanotify/fanotify_user.c
> @@ -412,6 +412,11 @@ static ssize_t fanotify_read(struct file *file, char __user *buf,
>
>         add_wait_queue(&group->notification_waitq, &wait);
>         while (1) {
> +               /*
> +                * User can supply arbitrarily large buffer. Avoid softlockups
> +                * in case there are lots of available events.
> +                */
> +               cond_resched();
>                 event = get_one_event(group, count);
>                 if (IS_ERR(event)) {
>                         ret = PTR_ERR(event);
> --
> 2.16.4
>

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

* Re: [PATCH] fanotify: Avoid softlockups when reading many events
  2020-07-15 12:34 ` Amir Goldstein
@ 2020-07-15 13:24   ` Jan Kara
  0 siblings, 0 replies; 3+ messages in thread
From: Jan Kara @ 2020-07-15 13:24 UTC (permalink / raw)
  To: Amir Goldstein; +Cc: Jan Kara, linux-fsdevel, Francesco Ruggeri

On Wed 15-07-20 15:34:48, Amir Goldstein wrote:
> On Wed, Jul 15, 2020 at 3:29 PM Jan Kara <jack@suse.cz> wrote:
> >
> > When user provides large buffer for events and there are lots of events
> > available, we can try to copy them all to userspace without scheduling
> > which can softlockup the kernel (furthermore exacerbated by the
> > contention on notification_lock). Add a scheduling point after copying
> > each event.
> >
> > Note that usually the real underlying problem is the cost of fanotify
> > event merging and the resulting contention on notification_lock but this
> > is a cheap way to somewhat reduce the problem until we can properly
> > address that.
> >
> > Reported-by: Francesco Ruggeri <fruggeri@arista.com>
> > Signed-off-by: Jan Kara <jack@suse.cz>
> > ---
> >  fs/notify/fanotify/fanotify_user.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > This is a quick mending we can do immediately and is probably a good idea
> > nevertheless... I'll queue it up if Amir agrees.
> >
> 
> Sure. fine by me.
> Maybe add a lore link to the issue report.
> 
> Reviewed-by: Amir Goldstein <amir73il@gmail.com>

Thanks. I've added the link and pushed out the patch to my tree.

									Honza

> 
> > diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> > index 63b5dffdca9e..d7f63aeca992 100644
> > --- a/fs/notify/fanotify/fanotify_user.c
> > +++ b/fs/notify/fanotify/fanotify_user.c
> > @@ -412,6 +412,11 @@ static ssize_t fanotify_read(struct file *file, char __user *buf,
> >
> >         add_wait_queue(&group->notification_waitq, &wait);
> >         while (1) {
> > +               /*
> > +                * User can supply arbitrarily large buffer. Avoid softlockups
> > +                * in case there are lots of available events.
> > +                */
> > +               cond_resched();
> >                 event = get_one_event(group, count);
> >                 if (IS_ERR(event)) {
> >                         ret = PTR_ERR(event);
> > --
> > 2.16.4
> >
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

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

end of thread, other threads:[~2020-07-15 13:24 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-15 12:29 [PATCH] fanotify: Avoid softlockups when reading many events Jan Kara
2020-07-15 12:34 ` Amir Goldstein
2020-07-15 13:24   ` Jan Kara

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).