diff mbox series

[v2,7/9] fanotify: record either old name new name or both for FAN_RENAME

Message ID 20211119071738.1348957-8-amir73il@gmail.com (mailing list archive)
State New, archived
Headers show
Series Extend fanotify dirent events | expand

Commit Message

Amir Goldstein Nov. 19, 2021, 7:17 a.m. UTC
We do not want to report the dirfid+name of a directory whose
inode/sb are not watched, because watcher may not have permissions
to see the directory content.

The FAN_MOVED_FROM/TO flags are used internally to indicate to
fanotify_alloc_event() if we need to record only the old parent+name,
only the new parent+name or both.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/notify/fanotify/fanotify.c | 57 ++++++++++++++++++++++++++++++-----
 1 file changed, 50 insertions(+), 7 deletions(-)

Comments

Jan Kara Nov. 26, 2021, 3:14 p.m. UTC | #1
On Fri 19-11-21 09:17:36, Amir Goldstein wrote:
> We do not want to report the dirfid+name of a directory whose
> inode/sb are not watched, because watcher may not have permissions
> to see the directory content.
> 
> The FAN_MOVED_FROM/TO flags are used internally to indicate to
> fanotify_alloc_event() if we need to record only the old parent+name,
> only the new parent+name or both.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/notify/fanotify/fanotify.c | 57 ++++++++++++++++++++++++++++++-----
>  1 file changed, 50 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> index 018b32a57702..c0a3fb1dd066 100644
> --- a/fs/notify/fanotify/fanotify.c
> +++ b/fs/notify/fanotify/fanotify.c
> @@ -290,6 +290,7 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group,
>  	__u32 marks_mask = 0, marks_ignored_mask = 0;
>  	__u32 test_mask, user_mask = FANOTIFY_OUTGOING_EVENTS |
>  				     FANOTIFY_EVENT_FLAGS;
> +	__u32 moved_mask = 0;
>  	const struct path *path = fsnotify_data_path(data, data_type);
>  	unsigned int fid_mode = FAN_GROUP_FLAG(group, FANOTIFY_FID_BITS);
>  	struct fsnotify_mark *mark;
> @@ -327,17 +328,44 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group,
>  			continue;
>  
>  		/*
> -		 * If the event is on a child and this mark is on a parent not
> -		 * watching children, don't send it!
> +		 * In the special case of FAN_RENAME event, inode mark is the
> +		 * mark on the old dir and parent mark is the mark on the new
> +		 * dir.  We do not want to report the dirfid+name of a directory
> +		 * whose inode/sb are not watched.  The FAN_MOVE flags
> +		 * are used internally to indicate if we need to report only
> +		 * the old parent+name, only the new parent+name or both.
>  		 */
> -		if (type == FSNOTIFY_OBJ_TYPE_PARENT &&
> -		    !(mark->mask & FS_EVENT_ON_CHILD))
> +		if (event_mask & FAN_RENAME) {
> +			/* Old dir sb are watched - report old info */
> +			if (type != FSNOTIFY_OBJ_TYPE_PARENT &&
> +			    (mark->mask & FAN_RENAME))
> +				moved_mask |= FAN_MOVED_FROM;
> +			/* New dir sb are watched - report new info */
> +			if (type != FSNOTIFY_OBJ_TYPE_INODE &&
> +			    (mark->mask & FAN_RENAME))
> +				moved_mask |= FAN_MOVED_TO;
> +		} else if (type == FSNOTIFY_OBJ_TYPE_PARENT &&
> +			   !(mark->mask & FS_EVENT_ON_CHILD)) {
> +			/*
> +			 * If the event is on a child and this mark is on
> +			 * a parent not watching children, don't send it!
> +			 */
>  			continue;
> +		}

It feels a bit hacky to mix the "what info to report" into the mask
especially as otherwise perfectly valid flags. Can we perhaps have a
separate function to find this out (like fanotify_rename_info_report_mask()
or something like that) and use it in fanotify_alloc_event() or directly in
fanotify_handle_event() and pass the result to fanotify_alloc_event()?
That would seem a bit cleaner to me.

								Honza
Amir Goldstein Nov. 29, 2021, 7:15 p.m. UTC | #2
On Fri, Nov 26, 2021 at 5:14 PM Jan Kara <jack@suse.cz> wrote:
>
> On Fri 19-11-21 09:17:36, Amir Goldstein wrote:
> > We do not want to report the dirfid+name of a directory whose
> > inode/sb are not watched, because watcher may not have permissions
> > to see the directory content.
> >
> > The FAN_MOVED_FROM/TO flags are used internally to indicate to
> > fanotify_alloc_event() if we need to record only the old parent+name,
> > only the new parent+name or both.
> >
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > ---
> >  fs/notify/fanotify/fanotify.c | 57 ++++++++++++++++++++++++++++++-----
> >  1 file changed, 50 insertions(+), 7 deletions(-)
> >
> > diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> > index 018b32a57702..c0a3fb1dd066 100644
> > --- a/fs/notify/fanotify/fanotify.c
> > +++ b/fs/notify/fanotify/fanotify.c
> > @@ -290,6 +290,7 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group,
> >       __u32 marks_mask = 0, marks_ignored_mask = 0;
> >       __u32 test_mask, user_mask = FANOTIFY_OUTGOING_EVENTS |
> >                                    FANOTIFY_EVENT_FLAGS;
> > +     __u32 moved_mask = 0;
> >       const struct path *path = fsnotify_data_path(data, data_type);
> >       unsigned int fid_mode = FAN_GROUP_FLAG(group, FANOTIFY_FID_BITS);
> >       struct fsnotify_mark *mark;
> > @@ -327,17 +328,44 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group,
> >                       continue;
> >
> >               /*
> > -              * If the event is on a child and this mark is on a parent not
> > -              * watching children, don't send it!
> > +              * In the special case of FAN_RENAME event, inode mark is the
> > +              * mark on the old dir and parent mark is the mark on the new
> > +              * dir.  We do not want to report the dirfid+name of a directory
> > +              * whose inode/sb are not watched.  The FAN_MOVE flags
> > +              * are used internally to indicate if we need to report only
> > +              * the old parent+name, only the new parent+name or both.
> >                */
> > -             if (type == FSNOTIFY_OBJ_TYPE_PARENT &&
> > -                 !(mark->mask & FS_EVENT_ON_CHILD))
> > +             if (event_mask & FAN_RENAME) {
> > +                     /* Old dir sb are watched - report old info */
> > +                     if (type != FSNOTIFY_OBJ_TYPE_PARENT &&
> > +                         (mark->mask & FAN_RENAME))
> > +                             moved_mask |= FAN_MOVED_FROM;
> > +                     /* New dir sb are watched - report new info */
> > +                     if (type != FSNOTIFY_OBJ_TYPE_INODE &&
> > +                         (mark->mask & FAN_RENAME))
> > +                             moved_mask |= FAN_MOVED_TO;
> > +             } else if (type == FSNOTIFY_OBJ_TYPE_PARENT &&
> > +                        !(mark->mask & FS_EVENT_ON_CHILD)) {
> > +                     /*
> > +                      * If the event is on a child and this mark is on
> > +                      * a parent not watching children, don't send it!
> > +                      */
> >                       continue;
> > +             }
>
> It feels a bit hacky to mix the "what info to report" into the mask
> especially as otherwise perfectly valid flags. Can we perhaps have a
> separate function to find this out (like fanotify_rename_info_report_mask()
> or something like that) and use it in fanotify_alloc_event() or directly in
> fanotify_handle_event() and pass the result to fanotify_alloc_event()?
> That would seem a bit cleaner to me.

I used fsnotify_iter_info *match_info arg to fanotify_group_event_mask()
to indicate the marks of this group that matched the event and passed
it into fanotify_alloc_event().

Thanks,
Amir.
diff mbox series

Patch

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index 018b32a57702..c0a3fb1dd066 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -290,6 +290,7 @@  static u32 fanotify_group_event_mask(struct fsnotify_group *group,
 	__u32 marks_mask = 0, marks_ignored_mask = 0;
 	__u32 test_mask, user_mask = FANOTIFY_OUTGOING_EVENTS |
 				     FANOTIFY_EVENT_FLAGS;
+	__u32 moved_mask = 0;
 	const struct path *path = fsnotify_data_path(data, data_type);
 	unsigned int fid_mode = FAN_GROUP_FLAG(group, FANOTIFY_FID_BITS);
 	struct fsnotify_mark *mark;
@@ -327,17 +328,44 @@  static u32 fanotify_group_event_mask(struct fsnotify_group *group,
 			continue;
 
 		/*
-		 * If the event is on a child and this mark is on a parent not
-		 * watching children, don't send it!
+		 * In the special case of FAN_RENAME event, inode mark is the
+		 * mark on the old dir and parent mark is the mark on the new
+		 * dir.  We do not want to report the dirfid+name of a directory
+		 * whose inode/sb are not watched.  The FAN_MOVE flags
+		 * are used internally to indicate if we need to report only
+		 * the old parent+name, only the new parent+name or both.
 		 */
-		if (type == FSNOTIFY_OBJ_TYPE_PARENT &&
-		    !(mark->mask & FS_EVENT_ON_CHILD))
+		if (event_mask & FAN_RENAME) {
+			/* Old dir sb are watched - report old info */
+			if (type != FSNOTIFY_OBJ_TYPE_PARENT &&
+			    (mark->mask & FAN_RENAME))
+				moved_mask |= FAN_MOVED_FROM;
+			/* New dir sb are watched - report new info */
+			if (type != FSNOTIFY_OBJ_TYPE_INODE &&
+			    (mark->mask & FAN_RENAME))
+				moved_mask |= FAN_MOVED_TO;
+		} else if (type == FSNOTIFY_OBJ_TYPE_PARENT &&
+			   !(mark->mask & FS_EVENT_ON_CHILD)) {
+			/*
+			 * If the event is on a child and this mark is on
+			 * a parent not watching children, don't send it!
+			 */
 			continue;
+		}
 
 		marks_mask |= mark->mask;
 	}
 
 	test_mask = event_mask & marks_mask & ~marks_ignored_mask;
+	/*
+	 * Add the internal FAN_MOVE flags to FAN_RENAME event.
+	 * They will not be reported to user along with FAN_RENAME.
+	 */
+	if (test_mask & FAN_RENAME) {
+		if (WARN_ON_ONCE(test_mask & FAN_MOVE))
+			return 0;
+		test_mask |= moved_mask;
+	}
 
 	/*
 	 * For dirent modification events (create/delete/move) that do not carry
@@ -753,13 +781,28 @@  static struct fanotify_event *fanotify_alloc_event(struct fsnotify_group *group,
 		}
 
 		/*
-		 * In the special case of FAN_RENAME event, we record both
-		 * old and new parent+name.
+		 * In the special case of FAN_RENAME event, the FAN_MOVE flags
+		 * are only used internally to indicate if we need to report
+		 * only the old parent+name, only the new parent+name or both.
 		 * 'dirid' and 'file_name' are the old parent+name and
 		 * 'moved' has the new parent+name.
 		 */
 		if (mask & FAN_RENAME) {
-			moved = fsnotify_data_dentry(data, data_type);
+			/* Either old and/or new info must be reported */
+			if (WARN_ON_ONCE(!(mask & FAN_MOVE)))
+				return NULL;
+
+			if (!(mask & FAN_MOVED_FROM)) {
+				/* Do not report old parent+name */
+				dirid = NULL;
+				file_name = NULL;
+			}
+			if (mask & FAN_MOVED_TO) {
+				/* Report new parent+name */
+				moved = fsnotify_data_dentry(data, data_type);
+			}
+			/* Clear internal flags */
+			mask &= ~FAN_MOVE;
 			name_event = true;
 		}
 	}