Re: [PATCH 2/2] fanotify: Add pidfd support to the fanotify API
flat view
From: Jan Kara <jack@suse.cz>
Date: 2021-04-19 10:22:43
Also in:
linux-fsdevel
On Fri 16-04-21 09:22:25, Matthew Bobrowski wrote:
Introduce a new flag FAN_REPORT_PIDFD for fanotify_init(2) which allows userspace applications to control whether a pidfd is to be returned instead of a pid for `struct fanotify_event_metadata.pid`. FAN_REPORT_PIDFD is mutually exclusive with FAN_REPORT_TID as the pidfd API is currently restricted to only support pidfd generation for thread-group leaders. Attempting to set them both when calling fanotify_init(2) will result in -EINVAL being returned to the caller. As the pidfd API evolves and support is added for tids, this is something that could be relaxed in the future. If pidfd creation fails, the pid in struct fanotify_event_metadata is set to FAN_NOPIDFD(-1). Falling back and providing a pid instead of a pidfd on pidfd creation failures was considered, although this could possibly lead to confusion and unpredictability within userspace applications as distinguishing between whether an actual pidfd or pid was returned could be difficult, so it's best to be explicit. Signed-off-by: Matthew Bobrowski <redacted>
Overall this looks OK to me. Just one style nit & one question below in addition to what Amir wrote.
quoted hunk ↗ jump to hunk
--- fs/notify/fanotify/fanotify_user.c | 33 +++++++++++++++++++++++++++--- include/linux/fanotify.h | 2 +- include/uapi/linux/fanotify.h | 2 ++ 3 files changed, 33 insertions(+), 4 deletions(-)diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c index 9e0c1afac8bd..fd8ae88796a8 100644 --- a/fs/notify/fanotify/fanotify_user.c +++ b/fs/notify/fanotify/fanotify_user.c@@ -329,7 +329,7 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group, struct fanotify_info *info = fanotify_event_info(event); unsigned int fid_mode = FAN_GROUP_FLAG(group, FANOTIFY_FID_BITS); struct file *f = NULL; - int ret, fd = FAN_NOFD; + int ret, pidfd, fd = FAN_NOFD; int info_type = 0; pr_debug("%s: group=%p event=%p\n", __func__, group, event);@@ -340,7 +340,25 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group, metadata.vers = FANOTIFY_METADATA_VERSION; metadata.reserved = 0; metadata.mask = event->mask & FANOTIFY_OUTGOING_EVENTS; - metadata.pid = pid_vnr(event->pid); + + if (FAN_GROUP_FLAG(group, FAN_REPORT_PIDFD) && + pid_has_task(event->pid, PIDTYPE_TGID)) {
Please align the rest of the condition to the opening brace. I.e., like:
if (FAN_GROUP_FLAG(group, FAN_REPORT_PIDFD) &&
pid_has_task(event->pid, PIDTYPE_TGID)) {
BTW, why is the pid_has_task() check here? And why is it OK to fall back to
returning pid if pid_has_task() is false?
Honza
--
Jan Kara [off-list ref]
SUSE Labs, CR