fanotify: self describing event metadata

PATCH In review Amir Goldstein 3 participants 4 messages Oct 8, 2018 – Oct 9, 2018

Subsystems (3)

VFS Fsnotify: filesystem notification infrastructure Fanotify

lore

Share & tools

lore

Amir Goldstein

author reviewer

2018-10-08 17:23 UTC #0 lore raw

PATCH 1/1 fanotify: self describing event metadata

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 <[email protected]>

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

---

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

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

index 2c57186caa2e..5769cf3ff035 100644
@@ -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));

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

index e03be5071362..79c4ba349780 100644
@@ -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 {

include/linux/fanotify.h +1 -1

index 11c85bd3d82e..a5a60691e48b 100644
@@ -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)

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

index 00b2304ed124..3e2ca5c97892 100644
@@ -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;

Signature · 1 line

2.17.1

Jan Kara

maintainer

2018-10-08 19:06 UTC #1 lore raw

Hi Amir,

Quoted 19 lines · 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 <[email protected]>
Signed-off-by Amir Goldstein <[email protected]>

Quoted 7 lines

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

Quoted 4 lines

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.

Quoted 5 lines

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

Signature · 2 lines

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

Amir Goldstein

author reviewer

2018-10-08 22:53 UTC #2 lore raw

Quoted 31 lines · Jan Kara

Hi Amir,
Quoted 19 lines · 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 <[email protected]>
Signed-off-by Amir Goldstein <[email protected]>
Quoted 7 lines
---
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().

Quoted 7 lines

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.

Quoted 7 lines

Quoted 4 lines
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!

Quoted 9 lines

Quoted 5 lines
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.

Marko Rauhamaa

2018-10-09 15:06 UTC #3 lore raw

In reply to Jan Kara

Jan Kara <[email protected]>:

Quoted 6 lines

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