diff mbox series

[3/3] fanotify: use pidfd_prepare()

Message ID 20230327-pidfd-file-api-v1-3-5c0e9a3158e4@kernel.org
State New
Headers
Series pidfd: add pidfd_prepare() |

Commit Message

Christian Brauner March 27, 2023, 6:22 p.m. UTC 
  We generally try to avoid installing a file descriptor into the caller's
file descriptor table just to close it again via close_fd() in case an
error occurs. Instead we reserve a file descriptor but don't install it
into the caller's file descriptor table yet. If we fail for other,
unrelated reasons we can just close the reserved file descriptor and if
we make it past all meaningful error paths we just install it. Fanotify
gets this right already for one fd type but not for pidfds.

Use the new pidfd_prepare() helper to reserve a pidfd and a pidfd file
and switch to the more common fd allocation and installation pattern.

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/notify/fanotify/fanotify_user.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

Comments

Jan Kara March 28, 2023, 7:54 a.m. UTC | #1 
On Mon 27-03-23 20:22:53, Christian Brauner wrote:
> We generally try to avoid installing a file descriptor into the caller's
> file descriptor table just to close it again via close_fd() in case an
> error occurs. Instead we reserve a file descriptor but don't install it
> into the caller's file descriptor table yet. If we fail for other,
> unrelated reasons we can just close the reserved file descriptor and if
> we make it past all meaningful error paths we just install it. Fanotify
> gets this right already for one fd type but not for pidfds.
> 
> Use the new pidfd_prepare() helper to reserve a pidfd and a pidfd file
> and switch to the more common fd allocation and installation pattern.
> 
> Signed-off-by: Christian Brauner <brauner@kernel.org>

Thanks for the improvement! It looks good to me. Feel free to add:

Acked-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/notify/fanotify/fanotify_user.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> index 8f430bfad487..22fb1cf7e1fc 100644
> --- a/fs/notify/fanotify/fanotify_user.c
> +++ b/fs/notify/fanotify/fanotify_user.c
> @@ -663,7 +663,7 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
>  	struct fanotify_info *info = fanotify_event_info(event);
>  	unsigned int info_mode = FAN_GROUP_FLAG(group, FANOTIFY_INFO_MODES);
>  	unsigned int pidfd_mode = info_mode & FAN_REPORT_PIDFD;
> -	struct file *f = NULL;
> +	struct file *f = NULL, *pidfd_file = NULL;
>  	int ret, pidfd = FAN_NOPIDFD, fd = FAN_NOFD;
>  
>  	pr_debug("%s: group=%p event=%p\n", __func__, group, event);
> @@ -718,7 +718,7 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
>  		    !pid_has_task(event->pid, PIDTYPE_TGID)) {
>  			pidfd = FAN_NOPIDFD;
>  		} else {
> -			pidfd = pidfd_create(event->pid, 0);
> +			pidfd = pidfd_prepare(event->pid, 0, &pidfd_file);
>  			if (pidfd < 0)
>  				pidfd = FAN_EPIDFD;
>  		}
> @@ -751,6 +751,9 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
>  	if (f)
>  		fd_install(fd, f);
>  
> +	if (pidfd_file)
> +		fd_install(pidfd, pidfd_file);
> +
>  	return metadata.event_len;
>  
>  out_close_fd:
> @@ -759,8 +762,10 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
>  		fput(f);
>  	}
>  
> -	if (pidfd >= 0)
> -		close_fd(pidfd);
> +	if (pidfd >= 0) {
> +		put_unused_fd(pidfd);
> +		fput(pidfd_file);
> +	}
>  
>  	return ret;
>  }
> 
> -- 
> 2.34.1
>
diff mbox series

Patch

diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index 8f430bfad487..22fb1cf7e1fc 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -663,7 +663,7 @@  static ssize_t copy_event_to_user(struct fsnotify_group *group,
 	struct fanotify_info *info = fanotify_event_info(event);
 	unsigned int info_mode = FAN_GROUP_FLAG(group, FANOTIFY_INFO_MODES);
 	unsigned int pidfd_mode = info_mode & FAN_REPORT_PIDFD;
-	struct file *f = NULL;
+	struct file *f = NULL, *pidfd_file = NULL;
 	int ret, pidfd = FAN_NOPIDFD, fd = FAN_NOFD;
 
 	pr_debug("%s: group=%p event=%p\n", __func__, group, event);
@@ -718,7 +718,7 @@  static ssize_t copy_event_to_user(struct fsnotify_group *group,
 		    !pid_has_task(event->pid, PIDTYPE_TGID)) {
 			pidfd = FAN_NOPIDFD;
 		} else {
-			pidfd = pidfd_create(event->pid, 0);
+			pidfd = pidfd_prepare(event->pid, 0, &pidfd_file);
 			if (pidfd < 0)
 				pidfd = FAN_EPIDFD;
 		}
@@ -751,6 +751,9 @@  static ssize_t copy_event_to_user(struct fsnotify_group *group,
 	if (f)
 		fd_install(fd, f);
 
+	if (pidfd_file)
+		fd_install(pidfd, pidfd_file);
+
 	return metadata.event_len;
 
 out_close_fd:
@@ -759,8 +762,10 @@  static ssize_t copy_event_to_user(struct fsnotify_group *group,
 		fput(f);
 	}
 
-	if (pidfd >= 0)
-		close_fd(pidfd);
+	if (pidfd >= 0) {
+		put_unused_fd(pidfd);
+		fput(pidfd_file);
+	}
 
 	return ret;
 }