[PATCH] fanotify: self describing event metadata

[PATCH] fanotify: self describing event metadata

[Amir Goldstein]

Use the 'reserved' u8 field in event metadata to describe event
optional information.

When event->pid is thread id, set the flag FAN_EVENT_INFO_TID in
event->flags.

Rename the init flag that is used to request reporting thread id
in event to FAN_REPORT_TID.

This change is semantic, because in the only case that user requests
FAN_REPORT_TID and fanotify is not able to meet that request,
event->pid will be zero anyway.

However, for future event info requests, it is better to be explicit
about whether fanotify was able to meet the request, similar to how
statx() API behaves.

Cc: <linux-api@vger.kernel.org>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---

Jan,

While working on reporting file handles, I realized that the API would
be more solid if the event info flags are bi-directional similar to
statx().

Even if you disapprove of the way this patch modifies the event foramt,
or if you would like to take more time to consider that, I would still
like to rename the fanotify_init flag to FAN_REPORT_TID, becasue I think
the name is better describing. See example man page with new name [1].

I realize that the reserved/flags union is a bit ugly, but I think it
will be less painful than introducing event metadata format v4 at this
time.

What do you thing?

Amir.

[1] https://github.com/amir73il/man-pages/commits/fanotify_tid

 fs/notify/fanotify/fanotify.c      |  2 +-
 fs/notify/fanotify/fanotify_user.c |  4 +++-
 include/linux/fanotify.h           |  2 +-
 include/uapi/linux/fanotify.h      | 10 ++++++++--
 4 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index 2c57186caa2e..5769cf3ff035 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -171,7 +171,7 @@ struct fanotify_event_info *fanotify_alloc_event(struct fsnotify_group *group,
 		goto out;
 init: __maybe_unused
 	fsnotify_init_event(&event->fse, inode, mask);
-	if (FAN_GROUP_FLAG(group, FAN_EVENT_INFO_TID))
+	if (FAN_GROUP_FLAG(group, FAN_REPORT_TID))
 		event->pid = get_pid(task_pid(current));
 	else
 		event->pid = get_pid(task_tgid(current));
diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index e03be5071362..79c4ba349780 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -130,9 +130,11 @@ static int fill_event_metadata(struct fsnotify_group *group,
 	metadata->event_len = FAN_EVENT_METADATA_LEN;
 	metadata->metadata_len = FAN_EVENT_METADATA_LEN;
 	metadata->vers = FANOTIFY_METADATA_VERSION;
-	metadata->reserved = 0;
+	metadata->flags = 0;
 	metadata->mask = fsn_event->mask & FANOTIFY_OUTGOING_EVENTS;
 	metadata->pid = pid_vnr(event->pid);
+	if (metadata->pid && FAN_GROUP_FLAG(group, FAN_REPORT_TID))
+		metadata->flags |= FAN_EVENT_INFO_TID;
 	if (unlikely(fsn_event->mask & FAN_Q_OVERFLOW))
 		metadata->fd = FAN_NOFD;
 	else {
diff --git a/include/linux/fanotify.h b/include/linux/fanotify.h
index 11c85bd3d82e..a5a60691e48b 100644
--- a/include/linux/fanotify.h
+++ b/include/linux/fanotify.h
@@ -19,7 +19,7 @@
 				 FAN_CLASS_PRE_CONTENT)
 
 #define FANOTIFY_INIT_FLAGS	(FANOTIFY_CLASS_BITS | \
-				 FAN_EVENT_INFO_TID | \
+				 FAN_REPORT_TID | \
 				 FAN_CLOEXEC | FAN_NONBLOCK | \
 				 FAN_UNLIMITED_QUEUE | FAN_UNLIMITED_MARKS)
 
diff --git a/include/uapi/linux/fanotify.h b/include/uapi/linux/fanotify.h
index 00b2304ed124..3e2ca5c97892 100644
--- a/include/uapi/linux/fanotify.h
+++ b/include/uapi/linux/fanotify.h
@@ -41,7 +41,7 @@
 #define FAN_ENABLE_AUDIT	0x00000040
 
 /* Flags to determine fanotify event format */
-#define FAN_EVENT_INFO_TID	0x00000100	/* event->pid is thread id */
+#define FAN_REPORT_TID		0x00000100	/* Report thread id in event */
 
 /* Deprecated - do not use this in programs and do not add new flags here! */
 #define FAN_ALL_INIT_FLAGS	(FAN_CLOEXEC | FAN_NONBLOCK | \
@@ -94,10 +94,16 @@
 
 #define FANOTIFY_METADATA_VERSION	3
 
+/* Flags reported on event->flags */
+#define FAN_EVENT_INFO_TID	0x01	/* event->pid is thread id */
+
 struct fanotify_event_metadata {
 	__u32 event_len;
 	__u8 vers;
-	__u8 reserved;
+	union {
+		__u8 reserved;
+		__u8 flags;
+	};
 	__u16 metadata_len;
 	__aligned_u64 mask;
 	__s32 fd;
-- 
2.17.1

Re: [PATCH] fanotify: self describing event metadata

[Jan Kara]

Hi Amir,

On Mon 08-10-18 13:12:29, Amir Goldstein wrote:
> Use the 'reserved' u8 field in event metadata to describe event
> optional information.
> 
> When event->pid is thread id, set the flag FAN_EVENT_INFO_TID in
> event->flags.
> 
> Rename the init flag that is used to request reporting thread id
> in event to FAN_REPORT_TID.
> 
> This change is semantic, because in the only case that user requests
> FAN_REPORT_TID and fanotify is not able to meet that request,
> event->pid will be zero anyway.
> 
> However, for future event info requests, it is better to be explicit
> about whether fanotify was able to meet the request, similar to how
> statx() API behaves.
> 
> Cc: <linux-api@vger.kernel.org>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
> 
> Jan,
> 
> While working on reporting file handles, I realized that the API would
> be more solid if the event info flags are bi-directional similar to
> statx().

That makes some sense but I'd really like to see how you apply this to
other things because e.g. with PID vs TGID I don't really see the need for
any flags. It might be interesting to have PID vs TGID flag there for
consistency once we really start to send them for other things but I don't
see a need to rush it now. Plus we are at rc7 already so we are out of
time for changes going to the coming merge window.

> Even if you disapprove of the way this patch modifies the event foramt,
> or if you would like to take more time to consider that, I would still
> like to rename the fanotify_init flag to FAN_REPORT_TID, becasue I think
> the name is better describing. See example man page with new name [1].

I agree the name is somewhat better. I did the renaming.

> I realize that the reserved/flags union is a bit ugly, but I think it
> will be less painful than introducing event metadata format v4 at this
> time.
> 
> What do you thing?

Why would be a new metadata format problem? You will quickly run out of
bits in that u8 anyways...

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] fanotify: self describing event metadata

[Amir Goldstein]

On Mon, Oct 8, 2018 at 2:54 PM Jan Kara <jack@suse.cz> wrote:
>
> Hi Amir,
>
> On Mon 08-10-18 13:12:29, Amir Goldstein wrote:
> > Use the 'reserved' u8 field in event metadata to describe event
> > optional information.
> >
> > When event->pid is thread id, set the flag FAN_EVENT_INFO_TID in
> > event->flags.
> >
> > Rename the init flag that is used to request reporting thread id
> > in event to FAN_REPORT_TID.
> >
> > This change is semantic, because in the only case that user requests
> > FAN_REPORT_TID and fanotify is not able to meet that request,
> > event->pid will be zero anyway.
> >
> > However, for future event info requests, it is better to be explicit
> > about whether fanotify was able to meet the request, similar to how
> > statx() API behaves.
> >
> > Cc: <linux-api@vger.kernel.org>
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > ---
> >
> > Jan,
> >
> > While working on reporting file handles, I realized that the API would
> > be more solid if the event info flags are bi-directional similar to
> > statx().
>
> That makes some sense but I'd really like to see how you apply this to
> other things because e.g. with PID vs TGID I don't really see the need for
> any flags. It might be interesting to have PID vs TGID flag there for
> consistency once we really start to send them for other things but I don't
> see a need to rush it now. Plus we are at rc7 already so we are out of
> time for changes going to the coming merge window.
>

I agree, I posted the event flags as a concept.
There is no need to rush it now and frankly, just the single use case of
FAN_REPORT_FID, I don't think the flags will be needed either.

> > Even if you disapprove of the way this patch modifies the event foramt,
> > or if you would like to take more time to consider that, I would still
> > like to rename the fanotify_init flag to FAN_REPORT_TID, becasue I think
> > the name is better describing. See example man page with new name [1].
>
> I agree the name is somewhat better. I did the renaming.
>

Great!

> > I realize that the reserved/flags union is a bit ugly, but I think it
> > will be less painful than introducing event metadata format v4 at this
> > time.
> >
> > What do you thing?
>
> Why would be a new metadata format problem? You will quickly run out of
> bits in that u8 anyways...
>

Supporting and documenting two format versions is a burden.
v3 was designed with several flexible design concept we still never used,
so we can still squeeze some extra usability from it without maintaining and
documenting a new format version. FAN_REPORT_FID is going to use
meta->event_len > FAN_EVENT_METADATA_LEN.
I thought that making use of the reserved bits for self descriptive info is
harmless, but anyway its not a requirement for my work (yet).

Thanks,
Amir.

Re: [PATCH] fanotify: self describing event metadata

[Marko Rauhamaa]

Jan Kara <jack@suse.cz>:

> That makes some sense but I'd really like to see how you apply this to
> other things because e.g. with PID vs TGID I don't really see the need
> for any flags. It might be interesting to have PID vs TGID flag there
> for consistency once we really start to send them for other things but
> I don't see a need to rush it now. Plus we are at rc7 already so we
> are out of time for changes going to the coming merge window.

A general side note: the PID is often useless as the process can die
before the fa-notification reaches the handler. I wish there were a
CLOSE_PERM event that held the closing process in existence (as a zombie
if nothing else) until the event has been processed.


Marko