New fanotify event info API

PATCH Superseded Amir Goldstein 2 participants 7 messages Sep 22, 2018 – Oct 2, 2018

Subsystems (3)

VFS Fsnotify: filesystem notification infrastructure Fanotify

lore

Share & tools

lore

Amir Goldstein

author reviewer

2018-09-22 00:10 UTC #0 lore raw

v2 cover New fanotify event info API

Jan,

Reposting my slightly modified cleanup patch along with the patch from nixiaoming that uses it to add a new fanotify_init() flag.

After a few review cycles with nixiaoming, per his request, I took his FAN_EVENT_INFO_TID patch to my tree, fixes a couple of issues including commit message wording and tested it.

For me, the new API seems very intuitive, not sure why thread id was not reported to begin with, but if you like more concrete use cases, you will need to ask them from nixiaoming.

Thanks,
Amir.

Amir Goldstein (1):
fanotify: store fanotify_init() flags in group's fanotify_data

nixiaoming (1):
fanotify: support reporting thread id instead of process id

 fs/notify/fanotify/fanotify.c      |  9 ++++++---
 fs/notify/fanotify/fanotify.h      |  2 +-
 fs/notify/fanotify/fanotify_user.c | 10 +++++-----
 fs/notify/fdinfo.c                 | 24 +-----------------------
 include/linux/fanotify.h           |  4 ++++
 include/linux/fsnotify_backend.h   |  4 ++--
 include/uapi/linux/fanotify.h      |  9 +++++++--
 7 files changed, 26 insertions(+), 36 deletions(-)

Signature · 1 line

2.17.1

Amir Goldstein

author reviewer

2018-09-22 00:10 UTC #1 lore raw

PATCH v2 1/2 fanotify: store fanotify_init() flags in group's fanotify_data

This averts the need to re-generate flags in fanotify_show_fdinfo() and sets the scene for addition of more upcoming flags without growing new members to the fanotify_data struct.

Signed-off-by Amir Goldstein <[email protected]>

---
 fs/notify/fanotify/fanotify_user.c |  8 ++++----
 fs/notify/fdinfo.c                 | 24 +-----------------------
 include/linux/fanotify.h           |  4 ++++
 include/linux/fsnotify_backend.h   |  4 ++--
 4 files changed, 11 insertions(+), 29 deletions(-)

fs/notify/fanotify/fanotify_user.c +4 -4

index 1347c588f778..15719d4aa4b5 100644
@@ -191,7 +191,7 @@ static int process_access_response(struct fsnotify_group *group,
 	if (fd < 0)
 		return -EINVAL;
 
-	if ((response & FAN_AUDIT) && !group->fanotify_data.audit)
+	if ((response & FAN_AUDIT) && !FAN_GROUP_FLAG(group, FAN_ENABLE_AUDIT))
 		return -EINVAL;
 
 	event = dequeue_event(group, fd);

@@ -701,8 +701,8 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int, flags, unsigned int, event_f_flags)
 	struct user_struct *user;
 	struct fanotify_event_info *oevent;
 
-	pr_debug("%s: flags=%d event_f_flags=%d\n",
-		__func__, flags, event_f_flags);
+	pr_debug("%s: flags=%x event_f_flags=%x\n",
+		 __func__, flags, event_f_flags);
 
 	if (!capable(CAP_SYS_ADMIN))
 		return -EPERM;

@@ -746,6 +746,7 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int, flags, unsigned int, event_f_flags)
 	}
 
 	group->fanotify_data.user = user;
+	group->fanotify_data.flags = flags;
 	atomic_inc(&user->fanotify_listeners);
 	group->memcg = get_mem_cgroup_from_mm(current->mm);

@@ -798,7 +799,6 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int, flags, unsigned int, event_f_flags)
 		fd = -EPERM;
 		if (!capable(CAP_AUDIT_WRITE))
 			goto out_destroy_group;
-		group->fanotify_data.audit = true;
 	}
 
 	fd = anon_inode_getfd("[fanotify]", &fanotify_fops, group, f_flags);

fs/notify/fdinfo.c +1 ~~-23~~

index 25385e336ac7..348a184bcdda 100644
@@ -142,31 +142,9 @@ static void fanotify_fdinfo(struct seq_file *m, struct fsnotify_mark *mark)
 void fanotify_show_fdinfo(struct seq_file *m, struct file *f)
 {
 	struct fsnotify_group *group = f->private_data;
-	unsigned int flags = 0;
-
-	switch (group->priority) {
-	case FS_PRIO_0:
-		flags |= FAN_CLASS_NOTIF;
-		break;
-	case FS_PRIO_1:
-		flags |= FAN_CLASS_CONTENT;
-		break;
-	case FS_PRIO_2:
-		flags |= FAN_CLASS_PRE_CONTENT;
-		break;
-	}
-
-	if (group->max_events == UINT_MAX)
-		flags |= FAN_UNLIMITED_QUEUE;
-
-	if (group->fanotify_data.max_marks == UINT_MAX)
-		flags |= FAN_UNLIMITED_MARKS;
-
-	if (group->fanotify_data.audit)
-		flags |= FAN_ENABLE_AUDIT;
 
 	seq_printf(m, "fanotify flags:%x event-flags:%x\n",
-		   flags, group->fanotify_data.f_flags);
+		   group->fanotify_data.flags, group->fanotify_data.f_flags);
 
 	show_fdinfo(m, f, fanotify_fdinfo);
 }

include/linux/fanotify.h +4 -0

index 096c96f4f16a..9c5ea3bdfaa0 100644
@@ -6,4 +6,8 @@
 
 /* not valid from userspace, only kernel internal */
 #define FAN_MARK_ONDIR		0x00000100
+
+#define FAN_GROUP_FLAG(group, flag) \
+	((group)->fanotify_data.flags & (flag))
+
 #endif /* _LINUX_FANOTIFY_H */

include/linux/fsnotify_backend.h +2 -2

index 81b88fc9df31..8e91341cbd8a 100644
@@ -189,10 +189,10 @@ struct fsnotify_group {
 			/* allows a group to block waiting for a userspace response */
 			struct list_head access_list;
 			wait_queue_head_t access_waitq;
-			int f_flags;
+			int flags;           /* flags from fanotify_init() */
+			int f_flags; /* event_f_flags from fanotify_init() */
 			unsigned int max_marks;
 			struct user_struct *user;
-			bool audit;
 		} fanotify_data;
 #endif /* CONFIG_FANOTIFY */
 	};

Signature · 1 line

2.17.1

Amir Goldstein

author reviewer

2018-09-22 00:10 UTC #2 lore raw

PATCH v2 2/2 fanotify: support reporting thread id instead of process id

In reply to Amir Goldstein

From: nixiaoming <[email protected]>

In order to identify which thread triggered the event in a multi-threaded program, add the FAN_EVENT_INFO_TID flag in fanotify_init to opt-in for reporting the event creator's thread id information.

Signed-off-by nixiaoming <[email protected]>

Signed-off-by Amir Goldstein <[email protected]>

---
 fs/notify/fanotify/fanotify.c      | 9 ++++++---
 fs/notify/fanotify/fanotify.h      | 2 +-
 fs/notify/fanotify/fanotify_user.c | 2 +-
 include/uapi/linux/fanotify.h      | 9 +++++++--
 4 files changed, 15 insertions(+), 7 deletions(-)

fs/notify/fanotify/fanotify.c +6 -3

index 94b52157bf8d..e380027e545c 100644
@@ -25,7 +25,7 @@ static bool should_merge(struct fsnotify_event *old_fsn,
 	old = FANOTIFY_E(old_fsn);
 	new = FANOTIFY_E(new_fsn);
 
-	if (old_fsn->inode == new_fsn->inode && old->tgid == new->tgid &&
+	if (old_fsn->inode == new_fsn->inode && old->pid == new->pid &&
 	    old->path.mnt == new->path.mnt &&
 	    old->path.dentry == new->path.dentry)
 		return true;

@@ -171,7 +171,10 @@ struct fanotify_event_info *fanotify_alloc_event(struct fsnotify_group *group,
 		goto out;
 init: __maybe_unused
 	fsnotify_init_event(&event->fse, inode, mask);
-	event->tgid = get_pid(task_tgid(current));
+	if (FAN_GROUP_FLAG(group, FAN_EVENT_INFO_TID))
+		event->pid = get_pid(task_pid(current));
+	else
+		event->pid = get_pid(task_tgid(current));
 	if (path) {
 		event->path = *path;
 		path_get(&event->path);

@@ -268,7 +271,7 @@ static void fanotify_free_event(struct fsnotify_event *fsn_event)
 
 	event = FANOTIFY_E(fsn_event);
 	path_put(&event->path);
-	put_pid(event->tgid);
+	put_pid(event->pid);
 	if (fanotify_is_perm_event(fsn_event->mask)) {
 		kmem_cache_free(fanotify_perm_event_cachep,
 				FANOTIFY_PE(fsn_event));

fs/notify/fanotify/fanotify.h +1 -1

index 8609ba06f474..d8d85104e7ff 100644
@@ -19,7 +19,7 @@ struct fanotify_event_info {
 	 * during this object's lifetime
 	 */
 	struct path path;
-	struct pid *tgid;
+	struct pid *pid;
 };
 
 /*

fs/notify/fanotify/fanotify_user.c +1 -1

index 15719d4aa4b5..039d31ea4380 100644
@@ -132,7 +132,7 @@ static int fill_event_metadata(struct fsnotify_group *group,
 	metadata->vers = FANOTIFY_METADATA_VERSION;
 	metadata->reserved = 0;
 	metadata->mask = fsn_event->mask & FAN_ALL_OUTGOING_EVENTS;
-	metadata->pid = pid_vnr(event->tgid);
+	metadata->pid = pid_vnr(event->pid);
 	if (unlikely(fsn_event->mask & FAN_Q_OVERFLOW))
 		metadata->fd = FAN_NOFD;
 	else {

include/uapi/linux/fanotify.h +7 -2

index ad81234d1919..6d98d14a6314 100644
@@ -38,9 +38,14 @@
 #define FAN_UNLIMITED_MARKS	0x00000020
 #define FAN_ENABLE_AUDIT	0x00000040
 
+/* Flags to determine fanotify event format */
+#define FAN_EVENT_INFO_TID	0x00000100	/* event->pid is thread id */
+
+#define FAN_EVENT_INFO_FLAGS	(FAN_EVENT_INFO_TID)
+
 #define FAN_ALL_INIT_FLAGS	(FAN_CLOEXEC | FAN_NONBLOCK | \
-				 FAN_ALL_CLASS_BITS | FAN_UNLIMITED_QUEUE |\
-				 FAN_UNLIMITED_MARKS)
+				 FAN_ALL_CLASS_BITS | FAN_EVENT_INFO_FLAGS | \
+				 FAN_UNLIMITED_QUEUE | FAN_UNLIMITED_MARKS)
 
 /* flags used for fanotify_modify_mark() */
 #define FAN_MARK_ADD		0x00000001

Signature · 1 line

2.17.1

Jan Kara

maintainer

2018-09-27 19:57 UTC #3 lore raw

In reply to Amir Goldstein

Quoted 13 lines · Amir Goldstein

Jan,
Reposting my slightly modified cleanup patch along with the patch from nixiaoming that uses it to add a new fanotify_init() flag.
After a few review cycles with nixiaoming, per his request, I took his FAN_EVENT_INFO_TID patch to my tree, fixes a couple of issues including commit message wording and tested it.
For me, the new API seems very intuitive, not sure why thread id was not reported to begin with, but if you like more concrete use cases, you will need to ask them from nixiaoming.

Quoted 8 lines

Thanks,
Amir.
Amir Goldstein (1):
fanotify: store fanotify_init() flags in group's fanotify_data
nixiaoming (1):
fanotify: support reporting thread id instead of process id

Thanks. Both patches look good to me and I've queued them up to my tree.

Honza

Quoted 13 lines

 fs/notify/fanotify/fanotify.c      |  9 ++++++---
 fs/notify/fanotify/fanotify.h      |  2 +-
 fs/notify/fanotify/fanotify_user.c | 10 +++++-----
 fs/notify/fdinfo.c                 | 24 +-----------------------
 include/linux/fanotify.h           |  4 ++++
 include/linux/fsnotify_backend.h   |  4 ++--
 include/uapi/linux/fanotify.h      |  9 +++++++--
 7 files changed, 26 insertions(+), 36 deletions(-)

Signature · 1 line

2.17.1

Signature · 2 lines

Jan Kara <[email protected]>
SUSE Labs, CR

Amir Goldstein

author reviewer

2018-09-28 01:52 UTC #4 lore raw

Quoted 18 lines · Jan Kara

Quoted 16 lines · Amir Goldstein
Jan,
Reposting my slightly modified cleanup patch along with the patch from nixiaoming that uses it to add a new fanotify_init() flag.
After a few review cycles with nixiaoming, per his request, I took his FAN_EVENT_INFO_TID patch to my tree, fixes a couple of issues including commit message wording and tested it.
For me, the new API seems very intuitive, not sure why thread id was not reported to begin with, but if you like more concrete use cases, you will need to ask them from nixiaoming.
Thanks,
Amir.

Quoted 8 lines

Quoted 5 lines
Amir Goldstein (1):
fanotify: store fanotify_init() flags in group's fanotify_data
nixiaoming (1):
fanotify: support reporting thread id instead of process id
Thanks. Both patches look good to me and I've queued them up to my tree.

Thanks!
However... I have found some issue that may require v3:

- I do not like the idea of changing uapi bit group constants (FAN_ALL_INIT_FLAGS) and adding new bit group constants (FAN_EVENT_INFO_FLAGS) which I don't think should be in uapi.
- uapi flag FAN_MARK_FILESYSTEM collides with kernel internal flag FAN_MARK_ONDIR

Sigh!
I will send a suggestion patch of how I think those issues should be sorted out as a basis for making the API change for_next safer.

Thanks,
Amir.

Jan Kara

maintainer

2018-10-02 16:13 UTC #5 lore raw

Quoted 28 lines · Amir Goldstein

Quoted 18 lines · Jan Kara
Quoted 16 lines · Amir Goldstein
Jan,
Reposting my slightly modified cleanup patch along with the patch from nixiaoming that uses it to add a new fanotify_init() flag.
After a few review cycles with nixiaoming, per his request, I took his FAN_EVENT_INFO_TID patch to my tree, fixes a couple of issues including commit message wording and tested it.
For me, the new API seems very intuitive, not sure why thread id was not reported to begin with, but if you like more concrete use cases, you will need to ask them from nixiaoming.
Thanks,
Amir.
Quoted 8 lines
Quoted 5 lines
Amir Goldstein (1):
fanotify: store fanotify_init() flags in group's fanotify_data
nixiaoming (1):
fanotify: support reporting thread id instead of process id
Thanks. Both patches look good to me and I've queued them up to my tree.

Quoted 6 lines

Thanks!
However... I have found some issue that may require v3:
- I do not like the idea of changing uapi bit group constants (FAN_ALL_INIT_FLAGS) and adding new bit group constants (FAN_EVENT_INFO_FLAGS) which I don't think should be in uapi.

I agree on the "should not be in uapi" part. For FAN_ALL_INIT_FLAGS changing the constant is IMO no real issue as I just cannot imagine how anybody could use that. For FAN_EVENT_INFO_FLAGS the concern is real, I agree.

Quoted 2 lines

- uapi flag FAN_MARK_FILESYSTEM collides with kernel internal flag FAN_MARK_ONDIR

Good point, that needs to be fixed. Probably we miss some bit in the BUILD_BUG_ON tests... Looking at FAN_MARK_ONDIR I just don't see why that is needed at all but let's leave that for a future cleanup.

Quoted 3 lines

Sigh!
I will send a suggestion patch of how I think those issues should be sorted out as a basis for making the API change for_next safer.

OK, I'll have a look at your patch and decide how to go about it.

Honza

Signature · 2 lines

Jan Kara <[email protected]>
SUSE Labs, CR

Amir Goldstein

author reviewer

2018-10-02 18:39 UTC #6 lore raw

Quoted 2 lines · Jan Kara

On Thu 27-09-18 22:32:23, Amir Goldstein wrote:

[...]

Quoted 8 lines

Quoted 3 lines
- I do not like the idea of changing uapi bit group constants (FAN_ALL_INIT_FLAGS) and adding new bit group constants (FAN_EVENT_INFO_FLAGS) which I don't think should be in uapi.
I agree on the "should not be in uapi" part. For FAN_ALL_INIT_FLAGS changing the constant is IMO no real issue as I just cannot imagine how anybody could use that. For FAN_EVENT_INFO_FLAGS the concern is real, I agree.

I agree that changing FAN_ALL_INIT_FLAGS is a non issue, but since I made a change called "fanotify: deprecate uapi FAN_ALL_* constants" I saw no reason to exclude it. Same goes for FAN_ALL_MARK_FLAGS and FAN_ALL_CLASS_BITS...
Also, since FAN_EVENT_INFO_FLAGS is defined outside uapi header and FAN_ALL_INIT_FLAGS needs to include FAN_EVENT_INFO_FLAGS I needed to use a constant FAN_ALL_USER_FLAGS that is also defined outside uapi header.

Quoted 8 lines

Quoted 3 lines
- uapi flag FAN_MARK_FILESYSTEM collides with kernel internal flag FAN_MARK_ONDIR
Good point, that needs to be fixed. Probably we miss some bit in the BUILD_BUG_ON tests... Looking at FAN_MARK_ONDIR I just don't see why that is needed at all but let's leave that for a future cleanup.

I have added that missing BUILD_BUG_ON in the posted RFC. Now with the user/kernel flags split, it looks conveniently like this:
BUILD_BUG_ON(FAN_USER_MARK_FLAGS & FAN_KERN_MARK_FLAGS)

Thanks,
Amir.

Quoted 10 lines

Quoted 3 lines
Sigh!
I will send a suggestion patch of how I think those issues should be sorted out as a basis for making the API change for_next safer.
OK, I'll have a look at your patch and decide how to go about it.
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR