[PATCH v2 0/2] New fanotify event info API

[PATCH v2 0/2] New fanotify event info API

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

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

-- 
2.17.1

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

[Amir Goldstein]

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 <amir73il@gmail.com>
---
 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(-)

diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index 1347c588f778..15719d4aa4b5 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -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);
diff --git a/fs/notify/fdinfo.c b/fs/notify/fdinfo.c
index 25385e336ac7..348a184bcdda 100644
--- a/fs/notify/fdinfo.c
+++ b/fs/notify/fdinfo.c
@@ -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);
 }
diff --git a/include/linux/fanotify.h b/include/linux/fanotify.h
index 096c96f4f16a..9c5ea3bdfaa0 100644
--- a/include/linux/fanotify.h
+++ b/include/linux/fanotify.h
@@ -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 */
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index 81b88fc9df31..8e91341cbd8a 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -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 */
 	};
-- 
2.17.1

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

[Amir Goldstein]

From: nixiaoming <nixiaoming@huawei.com>

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 <nixiaoming@huawei.com>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 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(-)

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index 94b52157bf8d..e380027e545c 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -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));
diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h
index 8609ba06f474..d8d85104e7ff 100644
--- a/fs/notify/fanotify/fanotify.h
+++ b/fs/notify/fanotify/fanotify.h
@@ -19,7 +19,7 @@ struct fanotify_event_info {
 	 * during this object's lifetime
 	 */
 	struct path path;
-	struct pid *tgid;
+	struct pid *pid;
 };
 
 /*
diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index 15719d4aa4b5..039d31ea4380 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -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 {
diff --git a/include/uapi/linux/fanotify.h b/include/uapi/linux/fanotify.h
index ad81234d1919..6d98d14a6314 100644
--- a/include/uapi/linux/fanotify.h
+++ b/include/uapi/linux/fanotify.h
@@ -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
-- 
2.17.1

Re: [PATCH v2 0/2] New fanotify event info API

[Jan Kara]

On Fri 21-09-18 21:20:29, Amir Goldstein wrote:
> 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

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

								Honza

> 
>  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(-)
> 
> -- 
> 2.17.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v2 0/2] New fanotify event info API

[Amir Goldstein]

On Thu, Sep 27, 2018 at 4:39 PM Jan Kara <jack@suse.cz> wrote:
>
> On Fri 21-09-18 21:20:29, Amir Goldstein wrote:
> > 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
>
> 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.

Re: [PATCH v2 0/2] New fanotify event info API

[Jan Kara]

On Thu 27-09-18 22:32:23, Amir Goldstein wrote:
> On Thu, Sep 27, 2018 at 4:39 PM Jan Kara <jack@suse.cz> wrote:
> >
> > On Fri 21-09-18 21:20:29, Amir Goldstein wrote:
> > > 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
> >
> > 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.

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.

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

> 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 <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v2 0/2] New fanotify event info API

[Amir Goldstein]

On Tue, Oct 2, 2018 at 12:30 PM Jan Kara <jack@suse.cz> wrote:
>
> On Thu 27-09-18 22:32:23, Amir Goldstein wrote:
[...]
> > - 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.

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

> > 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 <jack@suse.com>
> SUSE Labs, CR