linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] fanotify: fix logic of events on child
@ 2018-04-04 20:42 Amir Goldstein
  2018-04-05 16:32 ` Jan Kara
  0 siblings, 1 reply; 2+ messages in thread
From: Amir Goldstein @ 2018-04-04 20:42 UTC (permalink / raw)
  To: Jan Kara; +Cc: linux-fsdevel, stable

When event on child inodes are sent to the parent inode mark and
parent inode mark was not marked with FAN_EVENT_ON_CHILD, the event
will not be delivered to the listener process. However, if the same
process also has a mount mark, the event to the parent inode will be
delivered regadless of the mount mark mask.

This behavior is incorrect in the case where the mount mark mask does
not contain the specific event type. For example, the process adds
a mark on a directory with mask FAN_MODIFY (without FAN_EVENT_ON_CHILD)
and a mount mark with mask FAN_CLOSE_NOWRITE (without FAN_ONDIR).

A modify event on a file inside that directory (and inside that mount)
should not create a FAN_MODIFY event, because neither of the marks
requested to get that event on the file.

Fixes: 1968f5eed54c ("fanotify: use both marks when possible")
Cc: stable <stable@vger.kernel.org>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/notify/fanotify/fanotify.c | 34 +++++++++++++++-------------------
 1 file changed, 15 insertions(+), 19 deletions(-)

Jan,

While working on the super block mark patches [1], I stumbbled on
this bug. I figured I might as well send the fix out now.

I have written an LTP test [2] to reproduce it.

Thanks,
Amir.

[1] https://github.com/amir73il/linux/commits/fanotify_sb
[2] https://github.com/amir73il/ltp/commits/fanotify_sb

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index 6702a6a0bbb5..e0e6a9d627df 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -92,7 +92,7 @@ static bool fanotify_should_send_event(struct fsnotify_mark *inode_mark,
 				       u32 event_mask,
 				       const void *data, int data_type)
 {
-	__u32 marks_mask, marks_ignored_mask;
+	__u32 marks_mask = 0, marks_ignored_mask = 0;
 	const struct path *path = data;
 
 	pr_debug("%s: inode_mark=%p vfsmnt_mark=%p mask=%x data=%p"
@@ -108,24 +108,20 @@ static bool fanotify_should_send_event(struct fsnotify_mark *inode_mark,
 	    !d_can_lookup(path->dentry))
 		return false;
 
-	if (inode_mark && vfsmnt_mark) {
-		marks_mask = (vfsmnt_mark->mask | inode_mark->mask);
-		marks_ignored_mask = (vfsmnt_mark->ignored_mask | inode_mark->ignored_mask);
-	} else if (inode_mark) {
-		/*
-		 * if the event is for a child and this inode doesn't care about
-		 * events on the child, don't send it!
-		 */
-		if ((event_mask & FS_EVENT_ON_CHILD) &&
-		    !(inode_mark->mask & FS_EVENT_ON_CHILD))
-			return false;
-		marks_mask = inode_mark->mask;
-		marks_ignored_mask = inode_mark->ignored_mask;
-	} else if (vfsmnt_mark) {
-		marks_mask = vfsmnt_mark->mask;
-		marks_ignored_mask = vfsmnt_mark->ignored_mask;
-	} else {
-		BUG();
+	/*
+	 * if the event is for a child and this inode doesn't care about
+	 * events on the child, don't send it!
+	 */
+	if (inode_mark &&
+	    (!(event_mask & FS_EVENT_ON_CHILD) ||
+	     (inode_mark->mask & FS_EVENT_ON_CHILD))) {
+		marks_mask |= inode_mark->mask;
+		marks_ignored_mask |= inode_mark->ignored_mask;
+	}
+
+	if (vfsmnt_mark) {
+		marks_mask |= vfsmnt_mark->mask;
+		marks_ignored_mask |= vfsmnt_mark->ignored_mask;
 	}
 
 	if (d_is_dir(path->dentry) &&
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 2+ messages in thread
* Re: [PATCH] fanotify: fix logic of events on child
  2018-04-04 20:42 [PATCH] fanotify: fix logic of events on child Amir Goldstein
@ 2018-04-05 16:32 ` Jan Kara
  0 siblings, 0 replies; 2+ messages in thread
From: Jan Kara @ 2018-04-05 16:32 UTC (permalink / raw)
  To: Amir Goldstein; +Cc: Jan Kara, linux-fsdevel, stable

On Wed 04-04-18 23:42:18, Amir Goldstein wrote:
> When event on child inodes are sent to the parent inode mark and
> parent inode mark was not marked with FAN_EVENT_ON_CHILD, the event
> will not be delivered to the listener process. However, if the same
> process also has a mount mark, the event to the parent inode will be
> delivered regadless of the mount mark mask.
> 
> This behavior is incorrect in the case where the mount mark mask does
> not contain the specific event type. For example, the process adds
> a mark on a directory with mask FAN_MODIFY (without FAN_EVENT_ON_CHILD)
> and a mount mark with mask FAN_CLOSE_NOWRITE (without FAN_ONDIR).
> 
> A modify event on a file inside that directory (and inside that mount)
> should not create a FAN_MODIFY event, because neither of the marks
> requested to get that event on the file.
> 
> Fixes: 1968f5eed54c ("fanotify: use both marks when possible")
> Cc: stable <stable@vger.kernel.org>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/notify/fanotify/fanotify.c | 34 +++++++++++++++-------------------
>  1 file changed, 15 insertions(+), 19 deletions(-)
> 
> Jan,
> 
> While working on the super block mark patches [1], I stumbbled on
> this bug. I figured I might as well send the fix out now.
> 
> I have written an LTP test [2] to reproduce it.

Thanks for the patch. I've added it to my tree and will push it to Linus
(probably for rc2).

								Honza

> 
> Thanks,
> Amir.
> 
> [1] https://github.com/amir73il/linux/commits/fanotify_sb
> [2] https://github.com/amir73il/ltp/commits/fanotify_sb
> 
> diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> index 6702a6a0bbb5..e0e6a9d627df 100644
> --- a/fs/notify/fanotify/fanotify.c
> +++ b/fs/notify/fanotify/fanotify.c
> @@ -92,7 +92,7 @@ static bool fanotify_should_send_event(struct fsnotify_mark *inode_mark,
>  				       u32 event_mask,
>  				       const void *data, int data_type)
>  {
> -	__u32 marks_mask, marks_ignored_mask;
> +	__u32 marks_mask = 0, marks_ignored_mask = 0;
>  	const struct path *path = data;
>  
>  	pr_debug("%s: inode_mark=%p vfsmnt_mark=%p mask=%x data=%p"
> @@ -108,24 +108,20 @@ static bool fanotify_should_send_event(struct fsnotify_mark *inode_mark,
>  	    !d_can_lookup(path->dentry))
>  		return false;
>  
> -	if (inode_mark && vfsmnt_mark) {
> -		marks_mask = (vfsmnt_mark->mask | inode_mark->mask);
> -		marks_ignored_mask = (vfsmnt_mark->ignored_mask | inode_mark->ignored_mask);
> -	} else if (inode_mark) {
> -		/*
> -		 * if the event is for a child and this inode doesn't care about
> -		 * events on the child, don't send it!
> -		 */
> -		if ((event_mask & FS_EVENT_ON_CHILD) &&
> -		    !(inode_mark->mask & FS_EVENT_ON_CHILD))
> -			return false;
> -		marks_mask = inode_mark->mask;
> -		marks_ignored_mask = inode_mark->ignored_mask;
> -	} else if (vfsmnt_mark) {
> -		marks_mask = vfsmnt_mark->mask;
> -		marks_ignored_mask = vfsmnt_mark->ignored_mask;
> -	} else {
> -		BUG();
> +	/*
> +	 * if the event is for a child and this inode doesn't care about
> +	 * events on the child, don't send it!
> +	 */
> +	if (inode_mark &&
> +	    (!(event_mask & FS_EVENT_ON_CHILD) ||
> +	     (inode_mark->mask & FS_EVENT_ON_CHILD))) {
> +		marks_mask |= inode_mark->mask;
> +		marks_ignored_mask |= inode_mark->ignored_mask;
> +	}
> +
> +	if (vfsmnt_mark) {
> +		marks_mask |= vfsmnt_mark->mask;
> +		marks_ignored_mask |= vfsmnt_mark->ignored_mask;
>  	}
>  
>  	if (d_is_dir(path->dentry) &&
> -- 
> 2.7.4
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

^ permalink raw reply	[flat|nested] 2+ messages in thread
end of thread, other threads:[~2018-04-05 16:32 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-04 20:42 [PATCH] fanotify: fix logic of events on child Amir Goldstein
2018-04-05 16:32 ` 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).