[1/2] fanotify: Avoid lost events due to ENOMEM for unlimited queues
diff mbox

Message ID 20180221154430.19195-2-jack@suse.cz
State New
Headers show

Commit Message

Jan Kara Feb. 21, 2018, 3:44 p.m. UTC
Fanotify queues of unlimited length do not expect events can be lost.
Since these queues are used for system auditing and other security
related tasks, loosing events can even have security implications. So
avoid loosing events due to failure to allocate memory by making event
allocation use __GFP_NOFAIL. Since the allocation is small (32-bytes),
currently there is no practical difference of this change but still it
is good to have the expectation explicitely documented.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/notify/fanotify/fanotify.c      | 19 ++++++++++++++-----
 fs/notify/fanotify/fanotify.h      |  3 ++-
 fs/notify/fanotify/fanotify_user.c |  2 +-
 3 files changed, 17 insertions(+), 7 deletions(-)

Comments

Amir Goldstein Feb. 22, 2018, 4:04 p.m. UTC | #1
On Wed, Feb 21, 2018 at 5:44 PM, Jan Kara <jack@suse.cz> wrote:
> Fanotify queues of unlimited length do not expect events can be lost.
> Since these queues are used for system auditing and other security

Change looks good to me, but the reasoning is going backwards.
IMO, you should mention that we are going to change -ENOMEM
behavior to result in Q_OVERFLOW and user does not expect
Q_OVERFLOW from an UNLIMITED queue.

> related tasks, loosing events can even have security implications. So
> avoid loosing events due to failure to allocate memory by making event
> allocation use __GFP_NOFAIL. Since the allocation is small (32-bytes),
> currently there is no practical difference of this change but still it
> is good to have the expectation explicitely documented.
>

But if currently allocations cannot fail, then why do we need patch
2/2 (queue overflow event). It is because small allocations can fail when
accounted to non-root memcg?

Thanks,
Amir.



> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  fs/notify/fanotify/fanotify.c      | 19 ++++++++++++++-----
>  fs/notify/fanotify/fanotify.h      |  3 ++-
>  fs/notify/fanotify/fanotify_user.c |  2 +-
>  3 files changed, 17 insertions(+), 7 deletions(-)
>
> diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> index 6702a6a0bbb5..928f2a5eedb7 100644
> --- a/fs/notify/fanotify/fanotify.c
> +++ b/fs/notify/fanotify/fanotify.c
> @@ -139,23 +139,32 @@ static bool fanotify_should_send_event(struct fsnotify_mark *inode_mark,
>         return false;
>  }
>
> -struct fanotify_event_info *fanotify_alloc_event(struct inode *inode, u32 mask,
> +struct fanotify_event_info *fanotify_alloc_event(struct fsnotify_group *group,
> +                                                struct inode *inode, u32 mask,
>                                                  const struct path *path)
>  {
>         struct fanotify_event_info *event;
> +       gfp_t gfp = GFP_KERNEL;
> +
> +       /*
> +        * For queues with unlimited length lost events are not expected and
> +        * can possibly have security implications. Avoid losing events when
> +        * memory is short.
> +        */
> +       if (group->max_events == UINT_MAX)
> +               gfp |= __GFP_NOFAIL;
>
>         if (fanotify_is_perm_event(mask)) {
>                 struct fanotify_perm_event_info *pevent;
>
> -               pevent = kmem_cache_alloc(fanotify_perm_event_cachep,
> -                                         GFP_KERNEL);
> +               pevent = kmem_cache_alloc(fanotify_perm_event_cachep, gfp);
>                 if (!pevent)
>                         return NULL;
>                 event = &pevent->fae;
>                 pevent->response = 0;
>                 goto init;
>         }
> -       event = kmem_cache_alloc(fanotify_event_cachep, GFP_KERNEL);
> +       event = kmem_cache_alloc(fanotify_event_cachep, gfp);
>         if (!event)
>                 return NULL;
>  init: __maybe_unused
> @@ -210,7 +219,7 @@ static int fanotify_handle_event(struct fsnotify_group *group,
>                         return 0;
>         }
>
> -       event = fanotify_alloc_event(inode, mask, data);
> +       event = fanotify_alloc_event(group, inode, mask, data);
>         ret = -ENOMEM;
>         if (unlikely(!event))
>                 goto finish;
> diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h
> index 256d9d1ddea9..8609ba06f474 100644
> --- a/fs/notify/fanotify/fanotify.h
> +++ b/fs/notify/fanotify/fanotify.h
> @@ -52,5 +52,6 @@ static inline struct fanotify_event_info *FANOTIFY_E(struct fsnotify_event *fse)
>         return container_of(fse, struct fanotify_event_info, fse);
>  }
>
> -struct fanotify_event_info *fanotify_alloc_event(struct inode *inode, u32 mask,
> +struct fanotify_event_info *fanotify_alloc_event(struct fsnotify_group *group,
> +                                                struct inode *inode, u32 mask,
>                                                  const struct path *path);
> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> index c07eb3d655ea..72e367822efb 100644
> --- a/fs/notify/fanotify/fanotify_user.c
> +++ b/fs/notify/fanotify/fanotify_user.c
> @@ -757,7 +757,7 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int, flags, unsigned int, event_f_flags)
>         group->fanotify_data.user = user;
>         atomic_inc(&user->fanotify_listeners);
>
> -       oevent = fanotify_alloc_event(NULL, FS_Q_OVERFLOW, NULL);
> +       oevent = fanotify_alloc_event(group, NULL, FS_Q_OVERFLOW, NULL);
>         if (unlikely(!oevent)) {
>                 fd = -ENOMEM;
>                 goto out_destroy_group;
> --
> 2.13.6
>
Jan Kara Feb. 22, 2018, 4:22 p.m. UTC | #2
On Thu 22-02-18 18:04:36, Amir Goldstein wrote:
> On Wed, Feb 21, 2018 at 5:44 PM, Jan Kara <jack@suse.cz> wrote:
> > Fanotify queues of unlimited length do not expect events can be lost.
> > Since these queues are used for system auditing and other security
> 
> Change looks good to me, but the reasoning is going backwards.
> IMO, you should mention that we are going to change -ENOMEM
> behavior to result in Q_OVERFLOW and user does not expect
> Q_OVERFLOW from an UNLIMITED queue.

See below.

> > related tasks, loosing events can even have security implications. So
> > avoid loosing events due to failure to allocate memory by making event
> > allocation use __GFP_NOFAIL. Since the allocation is small (32-bytes),
> > currently there is no practical difference of this change but still it
> > is good to have the expectation explicitely documented.
> >
> 
> But if currently allocations cannot fail, then why do we need patch
> 2/2 (queue overflow event). It is because small allocations can fail when
> accounted to non-root memcg?

Yes. So how about changelog like:

Fanotify queues of unlimited length do not expect events can be lost.
Since these queues are used for system auditing and other security
related tasks, loosing events can even have security implications.
Currently, since the allocation is small (32-bytes), it cannot fail
however when we start accounting events in memcgs, allocation can start 
failing. So avoid loosing events due to failure to allocate memory by 
making event allocation use __GFP_NOFAIL.

Thanks for review!

								Honza
Amir Goldstein Feb. 22, 2018, 6:34 p.m. UTC | #3
On Thu, Feb 22, 2018 at 6:22 PM, Jan Kara <jack@suse.cz> wrote:
> On Thu 22-02-18 18:04:36, Amir Goldstein wrote:
>> On Wed, Feb 21, 2018 at 5:44 PM, Jan Kara <jack@suse.cz> wrote:
>> > Fanotify queues of unlimited length do not expect events can be lost.
>> > Since these queues are used for system auditing and other security
>>
>> Change looks good to me, but the reasoning is going backwards.
>> IMO, you should mention that we are going to change -ENOMEM
>> behavior to result in Q_OVERFLOW and user does not expect
>> Q_OVERFLOW from an UNLIMITED queue.
>
> See below.
>
>> > related tasks, loosing events can even have security implications. So
>> > avoid loosing events due to failure to allocate memory by making event
>> > allocation use __GFP_NOFAIL. Since the allocation is small (32-bytes),
>> > currently there is no practical difference of this change but still it
>> > is good to have the expectation explicitely documented.
>> >
>>
>> But if currently allocations cannot fail, then why do we need patch
>> 2/2 (queue overflow event). It is because small allocations can fail when
>> accounted to non-root memcg?
>
> Yes. So how about changelog like:
>
> Fanotify queues of unlimited length do not expect events can be lost.
> Since these queues are used for system auditing and other security
> related tasks, loosing events can even have security implications.
> Currently, since the allocation is small (32-bytes), it cannot fail
> however when we start accounting events in memcgs, allocation can start
> failing. So avoid loosing events due to failure to allocate memory by
> making event allocation use __GFP_NOFAIL.
>

Very good.
Thanks,
Amir.
Jan Kara Feb. 26, 2018, 1:06 p.m. UTC | #4
On Thu 22-02-18 20:34:48, Amir Goldstein wrote:
> On Thu, Feb 22, 2018 at 6:22 PM, Jan Kara <jack@suse.cz> wrote:
> > On Thu 22-02-18 18:04:36, Amir Goldstein wrote:
> >> On Wed, Feb 21, 2018 at 5:44 PM, Jan Kara <jack@suse.cz> wrote:
> >> > Fanotify queues of unlimited length do not expect events can be lost.
> >> > Since these queues are used for system auditing and other security
> >>
> >> Change looks good to me, but the reasoning is going backwards.
> >> IMO, you should mention that we are going to change -ENOMEM
> >> behavior to result in Q_OVERFLOW and user does not expect
> >> Q_OVERFLOW from an UNLIMITED queue.
> >
> > See below.
> >
> >> > related tasks, loosing events can even have security implications. So
> >> > avoid loosing events due to failure to allocate memory by making event
> >> > allocation use __GFP_NOFAIL. Since the allocation is small (32-bytes),
> >> > currently there is no practical difference of this change but still it
> >> > is good to have the expectation explicitely documented.
> >> >
> >>
> >> But if currently allocations cannot fail, then why do we need patch
> >> 2/2 (queue overflow event). It is because small allocations can fail when
> >> accounted to non-root memcg?
> >
> > Yes. So how about changelog like:
> >
> > Fanotify queues of unlimited length do not expect events can be lost.
> > Since these queues are used for system auditing and other security
> > related tasks, loosing events can even have security implications.
> > Currently, since the allocation is small (32-bytes), it cannot fail
> > however when we start accounting events in memcgs, allocation can start
> > failing. So avoid loosing events due to failure to allocate memory by
> > making event allocation use __GFP_NOFAIL.
> >
> 
> Very good.

Can I add your reviewed-by to the patch then?

								Honza
Amir Goldstein Feb. 26, 2018, 1:17 p.m. UTC | #5
On Mon, Feb 26, 2018 at 3:06 PM, Jan Kara <jack@suse.cz> wrote:
> On Thu 22-02-18 20:34:48, Amir Goldstein wrote:
>> On Thu, Feb 22, 2018 at 6:22 PM, Jan Kara <jack@suse.cz> wrote:
>> > On Thu 22-02-18 18:04:36, Amir Goldstein wrote:
>> >> On Wed, Feb 21, 2018 at 5:44 PM, Jan Kara <jack@suse.cz> wrote:
>> >> > Fanotify queues of unlimited length do not expect events can be lost.
>> >> > Since these queues are used for system auditing and other security
>> >>
>> >> Change looks good to me, but the reasoning is going backwards.
>> >> IMO, you should mention that we are going to change -ENOMEM
>> >> behavior to result in Q_OVERFLOW and user does not expect
>> >> Q_OVERFLOW from an UNLIMITED queue.
>> >
>> > See below.
>> >
>> >> > related tasks, loosing events can even have security implications. So
>> >> > avoid loosing events due to failure to allocate memory by making event
>> >> > allocation use __GFP_NOFAIL. Since the allocation is small (32-bytes),
>> >> > currently there is no practical difference of this change but still it
>> >> > is good to have the expectation explicitely documented.
>> >> >
>> >>
>> >> But if currently allocations cannot fail, then why do we need patch
>> >> 2/2 (queue overflow event). It is because small allocations can fail when
>> >> accounted to non-root memcg?
>> >
>> > Yes. So how about changelog like:
>> >
>> > Fanotify queues of unlimited length do not expect events can be lost.
>> > Since these queues are used for system auditing and other security
>> > related tasks, loosing events can even have security implications.
>> > Currently, since the allocation is small (32-bytes), it cannot fail
>> > however when we start accounting events in memcgs, allocation can start
>> > failing. So avoid loosing events due to failure to allocate memory by
>> > making event allocation use __GFP_NOFAIL.
>> >
>>
>> Very good.
>
> Can I add your reviewed-by to the patch then?
>

Yes, of course.

Thanks,
Amir.

Patch
diff mbox

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index 6702a6a0bbb5..928f2a5eedb7 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -139,23 +139,32 @@  static bool fanotify_should_send_event(struct fsnotify_mark *inode_mark,
 	return false;
 }
 
-struct fanotify_event_info *fanotify_alloc_event(struct inode *inode, u32 mask,
+struct fanotify_event_info *fanotify_alloc_event(struct fsnotify_group *group,
+						 struct inode *inode, u32 mask,
 						 const struct path *path)
 {
 	struct fanotify_event_info *event;
+	gfp_t gfp = GFP_KERNEL;
+
+	/*
+	 * For queues with unlimited length lost events are not expected and
+	 * can possibly have security implications. Avoid losing events when
+	 * memory is short.
+	 */
+	if (group->max_events == UINT_MAX)
+		gfp |= __GFP_NOFAIL;
 
 	if (fanotify_is_perm_event(mask)) {
 		struct fanotify_perm_event_info *pevent;
 
-		pevent = kmem_cache_alloc(fanotify_perm_event_cachep,
-					  GFP_KERNEL);
+		pevent = kmem_cache_alloc(fanotify_perm_event_cachep, gfp);
 		if (!pevent)
 			return NULL;
 		event = &pevent->fae;
 		pevent->response = 0;
 		goto init;
 	}
-	event = kmem_cache_alloc(fanotify_event_cachep, GFP_KERNEL);
+	event = kmem_cache_alloc(fanotify_event_cachep, gfp);
 	if (!event)
 		return NULL;
 init: __maybe_unused
@@ -210,7 +219,7 @@  static int fanotify_handle_event(struct fsnotify_group *group,
 			return 0;
 	}
 
-	event = fanotify_alloc_event(inode, mask, data);
+	event = fanotify_alloc_event(group, inode, mask, data);
 	ret = -ENOMEM;
 	if (unlikely(!event))
 		goto finish;
diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h
index 256d9d1ddea9..8609ba06f474 100644
--- a/fs/notify/fanotify/fanotify.h
+++ b/fs/notify/fanotify/fanotify.h
@@ -52,5 +52,6 @@  static inline struct fanotify_event_info *FANOTIFY_E(struct fsnotify_event *fse)
 	return container_of(fse, struct fanotify_event_info, fse);
 }
 
-struct fanotify_event_info *fanotify_alloc_event(struct inode *inode, u32 mask,
+struct fanotify_event_info *fanotify_alloc_event(struct fsnotify_group *group,
+						 struct inode *inode, u32 mask,
 						 const struct path *path);
diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index c07eb3d655ea..72e367822efb 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -757,7 +757,7 @@  SYSCALL_DEFINE2(fanotify_init, unsigned int, flags, unsigned int, event_f_flags)
 	group->fanotify_data.user = user;
 	atomic_inc(&user->fanotify_listeners);
 
-	oevent = fanotify_alloc_event(NULL, FS_Q_OVERFLOW, NULL);
+	oevent = fanotify_alloc_event(group, NULL, FS_Q_OVERFLOW, NULL);
 	if (unlikely(!oevent)) {
 		fd = -ENOMEM;
 		goto out_destroy_group;