open_by_handle_at: mount_fd opened with O_PATH

open_by_handle_at: mount_fd opened with O_PATH

[quentin.bouget]

[-- Attachment #1: Type: text/plain, Size: 436 bytes --]

Hello,

I recently noticed that the syscall open_by_handle_at() automatically 
fails if
its first argument is a file descriptor opened with O_PATH. I looked at 
the code
and saw no reason this could not be allowed. Attached to this mail are a
a reproducer and the patch I came up with.

I am not quite familiar with the kernel's way of processing patches. Any 
pointer
or advice on this matter is very welcome.

Cheers,
Quentin Bouget


[-- Attachment #2: reproducer.c --]
[-- Type: text/x-csrc, Size: 995 bytes --]

#define _GNU_SOURCE
#include <errno.h>
#include <error.h>
#include <fcntl.h>
#include <stdlib.h>
#include <unistd.h>

int
main()
{
    struct file_handle *fhandle;
    const char *pathname = "/";
    int mount_fd;
    int mountid;
    int fd;

    fhandle = malloc(sizeof(*fhandle) + 128);
    if (fhandle == NULL)
        error(EXIT_FAILURE, errno, "malloc");
    fhandle->handle_bytes = 128;

    fd = open(pathname, O_RDONLY | O_PATH | O_NOFOLLOW);
    if (fd < 0)
        error(EXIT_FAILURE, errno, "open");

    if (name_to_handle_at(fd, "", fhandle, &mountid, AT_EMPTY_PATH))
        error(EXIT_FAILURE, errno, "name_to_handle_at");

    mount_fd = fd;
    fd = open_by_handle_at(mount_fd, fhandle, O_RDONLY | O_PATH | O_NOFOLLOW);
    if (fd < 0)
        error(EXIT_FAILURE, errno, "open_by_handle_at");

    if (close(fd))
        error(EXIT_FAILURE, errno, "close");

    if (close(mount_fd))
        error(EXIT_FAILURE, errno, "close");

    free(fhandle);

    return EXIT_SUCCESS;
}

[-- Attachment #3: 0001-vfs-let-open_by_handle_at-use-mount_fd-opened-with-O.patch --]
[-- Type: text/x-patch, Size: 1760 bytes --]

From e3717e276444c5711335d398c29beedaf61bac82 Mon Sep 17 00:00:00 2001
From: Quentin Bouget <quentin.bouget@cea.fr>
Date: Thu, 24 Oct 2019 16:54:54 +0200
Subject: [PATCH] vfs: let open_by_handle_at() use mount_fd opened with O_PATH

The first argument of open_by_handle_at() is `mount_fd':

> a file descriptor for any object (file, directory, etc.) in the
> mounted filesystem with respect to which `handle' should be
> interpreted.

This patch allows for this file descriptor to be opened with O_PATH.

Signed-off-by: Quentin Bouget <quentin.bouget@cea.fr>
---
 fs/fhandle.c | 35 +++++++++++++++++++++++------------
 1 file changed, 23 insertions(+), 12 deletions(-)

diff --git a/fs/fhandle.c b/fs/fhandle.c
index 01263ffbc..8b67f1b9e 100644
--- a/fs/fhandle.c
+++ b/fs/fhandle.c
@@ -112,22 +112,33 @@ SYSCALL_DEFINE5(name_to_handle_at, int, dfd, const char __user *, name,
 	return err;
 }
 
+static struct vfsmount *get_vfsmount_from_cwd(void)
+{
+	struct fs_struct *fs = current->fs;
+	struct vfsmount *mnt;
+
+	spin_lock(&fs->lock);
+	mnt = mntget(fs->pwd.mnt);
+	spin_unlock(&fs->lock);
+
+	return mnt;
+}
+
 static struct vfsmount *get_vfsmount_from_fd(int fd)
 {
 	struct vfsmount *mnt;
+	struct path path;
+	int err;
 
-	if (fd == AT_FDCWD) {
-		struct fs_struct *fs = current->fs;
-		spin_lock(&fs->lock);
-		mnt = mntget(fs->pwd.mnt);
-		spin_unlock(&fs->lock);
-	} else {
-		struct fd f = fdget(fd);
-		if (!f.file)
-			return ERR_PTR(-EBADF);
-		mnt = mntget(f.file->f_path.mnt);
-		fdput(f);
-	}
+	if (fd == AT_FDCWD)
+		return get_vfsmount_from_cwd();
+
+	err = filename_lookup(fd, getname_kernel(""), LOOKUP_EMPTY, &path, NULL);
+	if (err)
+		return ERR_PTR(err);
+
+	mnt = mntget(path.mnt);
+	path_put(&path);
 	return mnt;
 }
 
-- 
2.18.1

Re: open_by_handle_at: mount_fd opened with O_PATH

[Amir Goldstein]

On Mon, Dec 16, 2019 at 11:39 AM <quentin.bouget@cea.fr> wrote:
>
> Hello,
>
> I recently noticed that the syscall open_by_handle_at() automatically
> fails if
> its first argument is a file descriptor opened with O_PATH. I looked at
> the code
> and saw no reason this could not be allowed. Attached to this mail are a
> a reproducer and the patch I came up with.
>
> I am not quite familiar with the kernel's way of processing patches. Any
> pointer
> or advice on this matter is very welcome.
>

See similar patch by Miklos to do the same for f*xattr() syscalls that
looks simpler:
https://lore.kernel.org/linux-fsdevel/20191128155940.17530-8-mszeredi@redhat.com/

Al, any objections to making this change?

Thanks,
Amir.

Re: open_by_handle_at: mount_fd opened with O_PATH

[BOUGET Quentin]

On 16/12/2019 13:28, Amir Goldstein wrote:
> On Mon, Dec 16, 2019 at 11:39 AM <quentin.bouget@cea.fr> wrote:
>> Hello,
>>
>> I recently noticed that the syscall open_by_handle_at() automatically
>> fails if
>> its first argument is a file descriptor opened with O_PATH. I looked at
>> the code
>> and saw no reason this could not be allowed. Attached to this mail are a
>> a reproducer and the patch I came up with.
>>
>> I am not quite familiar with the kernel's way of processing patches. Any
>> pointer
>> or advice on this matter is very welcome.
>>
> See similar patch by Miklos to do the same for f*xattr() syscalls that
> looks simpler:
> https://lore.kernel.org/linux-fsdevel/20191128155940.17530-8-mszeredi@redhat.com/
>
> Al, any objections to making this change?
>
> Thanks,
> Amir.

Much simpler, indeed (tested it, seems to work).

Thanks,
Quentin

Re: open_by_handle_at: mount_fd opened with O_PATH

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 4458 bytes --]

On Mon, Dec 16 2019, quentin.bouget@cea.fr wrote:

> Hello,
>
> I recently noticed that the syscall open_by_handle_at() automatically 
> fails if
> its first argument is a file descriptor opened with O_PATH. I looked at 
> the code
> and saw no reason this could not be allowed. Attached to this mail are a
> a reproducer and the patch I came up with.
>
> I am not quite familiar with the kernel's way of processing patches. Any 
> pointer
> or advice on this matter is very welcome.

It is probably worth reading through Documentation/process/,
particularly
   5.Posting.rst
We generally like the email to be the commit.  If you have comments to
make that really don't need to get included in the git commit, they can
go after the "---" after the Signed-off-by.

Also including a reproducer is great, but inline is generally preferred
to attachments as long as it isn't too big.

When making API changes (which this fix does), it is best to Cc
linux-api@vger.kernel.org.

You might also like to submit a separate patch to linux man-pages
to update the open(2) man page to include open_by_handle_at
in the list of "operations [that] can be performed on the resulting file descriptor"
in the section about O_PATH, but just cc:ing linux-api might be enough,
depending on how busy Michael is.

Thanks,
NeilBrown

>
> Cheers,
> Quentin Bouget
>
> #define _GNU_SOURCE
> #include <errno.h>
> #include <error.h>
> #include <fcntl.h>
> #include <stdlib.h>
> #include <unistd.h>
>
> int
> main()
> {
>     struct file_handle *fhandle;
>     const char *pathname = "/";
>     int mount_fd;
>     int mountid;
>     int fd;
>
>     fhandle = malloc(sizeof(*fhandle) + 128);
>     if (fhandle == NULL)
>         error(EXIT_FAILURE, errno, "malloc");
>     fhandle->handle_bytes = 128;
>
>     fd = open(pathname, O_RDONLY | O_PATH | O_NOFOLLOW);
>     if (fd < 0)
>         error(EXIT_FAILURE, errno, "open");
>
>     if (name_to_handle_at(fd, "", fhandle, &mountid, AT_EMPTY_PATH))
>         error(EXIT_FAILURE, errno, "name_to_handle_at");
>
>     mount_fd = fd;
>     fd = open_by_handle_at(mount_fd, fhandle, O_RDONLY | O_PATH | O_NOFOLLOW);
>     if (fd < 0)
>         error(EXIT_FAILURE, errno, "open_by_handle_at");
>
>     if (close(fd))
>         error(EXIT_FAILURE, errno, "close");
>
>     if (close(mount_fd))
>         error(EXIT_FAILURE, errno, "close");
>
>     free(fhandle);
>
>     return EXIT_SUCCESS;
> }
> From e3717e276444c5711335d398c29beedaf61bac82 Mon Sep 17 00:00:00 2001
> From: Quentin Bouget <quentin.bouget@cea.fr>
> Date: Thu, 24 Oct 2019 16:54:54 +0200
> Subject: [PATCH] vfs: let open_by_handle_at() use mount_fd opened with O_PATH
>
> The first argument of open_by_handle_at() is `mount_fd':
>
>> a file descriptor for any object (file, directory, etc.) in the
>> mounted filesystem with respect to which `handle' should be
>> interpreted.
>
> This patch allows for this file descriptor to be opened with O_PATH.
>
> Signed-off-by: Quentin Bouget <quentin.bouget@cea.fr>
> ---
>  fs/fhandle.c | 35 +++++++++++++++++++++++------------
>  1 file changed, 23 insertions(+), 12 deletions(-)
>
> diff --git a/fs/fhandle.c b/fs/fhandle.c
> index 01263ffbc..8b67f1b9e 100644
> --- a/fs/fhandle.c
> +++ b/fs/fhandle.c
> @@ -112,22 +112,33 @@ SYSCALL_DEFINE5(name_to_handle_at, int, dfd, const char __user *, name,
>  	return err;
>  }
>  
> +static struct vfsmount *get_vfsmount_from_cwd(void)
> +{
> +	struct fs_struct *fs = current->fs;
> +	struct vfsmount *mnt;
> +
> +	spin_lock(&fs->lock);
> +	mnt = mntget(fs->pwd.mnt);
> +	spin_unlock(&fs->lock);
> +
> +	return mnt;
> +}
> +
>  static struct vfsmount *get_vfsmount_from_fd(int fd)
>  {
>  	struct vfsmount *mnt;
> +	struct path path;
> +	int err;
>  
> -	if (fd == AT_FDCWD) {
> -		struct fs_struct *fs = current->fs;
> -		spin_lock(&fs->lock);
> -		mnt = mntget(fs->pwd.mnt);
> -		spin_unlock(&fs->lock);
> -	} else {
> -		struct fd f = fdget(fd);
> -		if (!f.file)
> -			return ERR_PTR(-EBADF);
> -		mnt = mntget(f.file->f_path.mnt);
> -		fdput(f);
> -	}
> +	if (fd == AT_FDCWD)
> +		return get_vfsmount_from_cwd();
> +
> +	err = filename_lookup(fd, getname_kernel(""), LOOKUP_EMPTY, &path, NULL);
> +	if (err)
> +		return ERR_PTR(err);
> +
> +	mnt = mntget(path.mnt);
> +	path_put(&path);
>  	return mnt;
>  }
>  
> -- 
> 2.18.1

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]