| Message ID | 20210331080519.172-2-xieyongji@bytedance.com |
|---|---|
| State | New |
| Headers | show |
| Series | Introduce VDUSE - vDPA Device in Userspace | expand |
On Wed, Mar 31, 2021 at 04:05:10PM +0800, Xie Yongji wrote: > Export receive_fd() so that some modules can use > it to pass file descriptor between processes without > missing any security stuffs. > > Signed-off-by: Xie Yongji <xieyongji@bytedance.com> > --- Yeah, as I said in the other mail I'd be comfortable with exposing just this variant of the helper. Maybe this should be a separate patch bundled together with Christoph's patch to split parts of receive_fd() into a separate helper. This would also allow us to simplify a few other codepaths in drivers as well btw. I just took a hasty stab at two of them: diff --git a/drivers/android/binder.c b/drivers/android/binder.c index c119736ca56a..3c716bf6d84b 100644 --- a/drivers/android/binder.c +++ b/drivers/android/binder.c @@ -3728,8 +3728,9 @@ static int binder_apply_fd_fixups(struct binder_proc *proc, int ret = 0; list_for_each_entry(fixup, &t->fd_fixups, fixup_entry) { - int fd = get_unused_fd_flags(O_CLOEXEC); + int fd = receive_fd(fixup->file, O_CLOEXEC); + fd = receive_fd(fixup->file, O_CLOEXEC); if (fd < 0) { binder_debug(BINDER_DEBUG_TRANSACTION, "failed fd fixup txn %d fd %d\n", @@ -3741,7 +3742,7 @@ static int binder_apply_fd_fixups(struct binder_proc *proc, "fd fixup txn %d fd %d\n", t->debug_id, fd); trace_binder_transaction_fd_recv(t, fd, fixup->offset); - fd_install(fd, fixup->file); + fput(fixup->file); fixup->file = NULL; if (binder_alloc_copy_to_buffer(&proc->alloc, t->buffer, fixup->offset, &fd, diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c index 5e2374580e27..c3a6b6abb7f4 100644 --- a/drivers/tty/pty.c +++ b/drivers/tty/pty.c @@ -629,12 +629,6 @@ int ptm_open_peer(struct file *master, struct tty_struct *tty, int flags) if (tty->driver != ptm_driver) return -EIO; - fd = get_unused_fd_flags(flags); - if (fd < 0) { - retval = fd; - goto err; - } - /* Compute the slave's path */ path.mnt = devpts_mntget(master, tty->driver_data); if (IS_ERR(path.mnt)) { @@ -650,7 +644,8 @@ int ptm_open_peer(struct file *master, struct tty_struct *tty, int flags) goto err_put; } - fd_install(fd, filp); + fd = receive_fd(filp, flags); + fput(filp); return fd; err_put: > fs/file.c | 6 ++++++ > include/linux/file.h | 7 +++---- > 2 files changed, 9 insertions(+), 4 deletions(-) > > diff --git a/fs/file.c b/fs/file.c > index dab120b71e44..d7d957217576 100644 > --- a/fs/file.c > +++ b/fs/file.c > @@ -1108,6 +1108,12 @@ int __receive_fd(int fd, struct file *file, int __user *ufd, unsigned int o_flag > return new_fd; > } > > +int receive_fd(struct file *file, unsigned int o_flags) > +{ > + return __receive_fd(-1, file, NULL, o_flags); > +} > +EXPORT_SYMBOL(receive_fd); > + > static int ksys_dup3(unsigned int oldfd, unsigned int newfd, int flags) > { > int err = -EBADF; > diff --git a/include/linux/file.h b/include/linux/file.h > index 225982792fa2..4667f9567d3e 100644 > --- a/include/linux/file.h > +++ b/include/linux/file.h > @@ -94,6 +94,9 @@ extern void fd_install(unsigned int fd, struct file *file); > > extern int __receive_fd(int fd, struct file *file, int __user *ufd, > unsigned int o_flags); > + > +extern int receive_fd(struct file *file, unsigned int o_flags); > + > static inline int receive_fd_user(struct file *file, int __user *ufd, > unsigned int o_flags) > { > @@ -101,10 +104,6 @@ static inline int receive_fd_user(struct file *file, int __user *ufd, > return -EFAULT; > return __receive_fd(-1, file, ufd, o_flags); > } > -static inline int receive_fd(struct file *file, unsigned int o_flags) > -{ > - return __receive_fd(-1, file, NULL, o_flags); > -} > static inline int receive_fd_replace(int fd, struct file *file, unsigned int o_flags) > { > return __receive_fd(fd, file, NULL, o_flags); > -- > 2.11.0 >
On Wed, Mar 31, 2021 at 11:15:45AM +0200, Christian Brauner wrote: > On Wed, Mar 31, 2021 at 04:05:10PM +0800, Xie Yongji wrote: > > Export receive_fd() so that some modules can use > > it to pass file descriptor between processes without > > missing any security stuffs. > > > > Signed-off-by: Xie Yongji <xieyongji@bytedance.com> > > --- > > Yeah, as I said in the other mail I'd be comfortable with exposing just > this variant of the helper. > Maybe this should be a separate patch bundled together with Christoph's > patch to split parts of receive_fd() into a separate helper. > This would also allow us to simplify a few other codepaths in drivers as > well btw. I just took a hasty stab at two of them: > > diff --git a/drivers/android/binder.c b/drivers/android/binder.c > index c119736ca56a..3c716bf6d84b 100644 > --- a/drivers/android/binder.c > +++ b/drivers/android/binder.c > @@ -3728,8 +3728,9 @@ static int binder_apply_fd_fixups(struct binder_proc *proc, > int ret = 0; > > list_for_each_entry(fixup, &t->fd_fixups, fixup_entry) { > - int fd = get_unused_fd_flags(O_CLOEXEC); > + int fd = receive_fd(fixup->file, O_CLOEXEC); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Assignment duplicated on the next line. > > + fd = receive_fd(fixup->file, O_CLOEXEC); > if (fd < 0) { > binder_debug(BINDER_DEBUG_TRANSACTION, > "failed fd fixup txn %d fd %d\n", regards, dan carpenter
On Wed, Mar 31, 2021 at 5:15 PM Christian Brauner <christian.brauner@ubuntu.com> wrote: > > On Wed, Mar 31, 2021 at 04:05:10PM +0800, Xie Yongji wrote: > > Export receive_fd() so that some modules can use > > it to pass file descriptor between processes without > > missing any security stuffs. > > > > Signed-off-by: Xie Yongji <xieyongji@bytedance.com> > > --- > > Yeah, as I said in the other mail I'd be comfortable with exposing just > this variant of the helper. Thanks, I got it now. > Maybe this should be a separate patch bundled together with Christoph's > patch to split parts of receive_fd() into a separate helper. Do we need to add the seccomp notifier into the separate helper? In our case, the file passed to the separate helper is from another process. Thanks, Yongji
On Wed, Mar 31, 2021 at 09:59:07PM +0800, Yongji Xie wrote: > On Wed, Mar 31, 2021 at 8:23 PM Christian Brauner > <christian.brauner@ubuntu.com> wrote: > > > > On Wed, Mar 31, 2021 at 07:32:33PM +0800, Yongji Xie wrote: > > > On Wed, Mar 31, 2021 at 5:15 PM Christian Brauner > > > <christian.brauner@ubuntu.com> wrote: > > > > > > > > On Wed, Mar 31, 2021 at 04:05:10PM +0800, Xie Yongji wrote: > > > > > Export receive_fd() so that some modules can use > > > > > it to pass file descriptor between processes without > > > > > missing any security stuffs. > > > > > > > > > > Signed-off-by: Xie Yongji <xieyongji@bytedance.com> > > > > > --- > > > > > > > > Yeah, as I said in the other mail I'd be comfortable with exposing just > > > > this variant of the helper. > > > > > > Thanks, I got it now. > > > > > > > Maybe this should be a separate patch bundled together with Christoph's > > > > patch to split parts of receive_fd() into a separate helper. > > > > > > Do we need to add the seccomp notifier into the separate helper? In > > > our case, the file passed to the separate helper is from another > > > process. > > > > Not sure what you mean. Christoph has proposed > > https://lore.kernel.org/linux-fsdevel/20210325082209.1067987-2-hch@lst.de > > I was just saying that if we think this patch is useful we might bundle > > it together with the > > EXPORT_SYMBOL(receive_fd) > > part here, convert all drivers that currently open-code get_unused_fd() > > + fd_install() to use receive_fd(), and make this a separate patchset. > > > > Yes, I see. We can split the parts (get_unused_fd() + fd_install()) of > receive_fd() into a separate helper and convert all drivers to use > that. What I mean is that I also would like to use > security_file_receive() in my modules. So I'm not sure if it's ok to > add security_file_receive() into the separate helper. Or do I need to > export security_file_receive() separately? I think I confused you which is my bad. What you do here is - in my opinion - correct. I'm just saying that exporting receive_fd() allows further cleanups and your export here could go on top of Christoph's change in a separate series. Christian
On Wed, Mar 31, 2021 at 10:08 PM Christian Brauner <christian.brauner@ubuntu.com> wrote: > > On Wed, Mar 31, 2021 at 09:59:07PM +0800, Yongji Xie wrote: > > On Wed, Mar 31, 2021 at 8:23 PM Christian Brauner > > <christian.brauner@ubuntu.com> wrote: > > > > > > On Wed, Mar 31, 2021 at 07:32:33PM +0800, Yongji Xie wrote: > > > > On Wed, Mar 31, 2021 at 5:15 PM Christian Brauner > > > > <christian.brauner@ubuntu.com> wrote: > > > > > > > > > > On Wed, Mar 31, 2021 at 04:05:10PM +0800, Xie Yongji wrote: > > > > > > Export receive_fd() so that some modules can use > > > > > > it to pass file descriptor between processes without > > > > > > missing any security stuffs. > > > > > > > > > > > > Signed-off-by: Xie Yongji <xieyongji@bytedance.com> > > > > > > --- > > > > > > > > > > Yeah, as I said in the other mail I'd be comfortable with exposing just > > > > > this variant of the helper. > > > > > > > > Thanks, I got it now. > > > > > > > > > Maybe this should be a separate patch bundled together with Christoph's > > > > > patch to split parts of receive_fd() into a separate helper. > > > > > > > > Do we need to add the seccomp notifier into the separate helper? In > > > > our case, the file passed to the separate helper is from another > > > > process. > > > > > > Not sure what you mean. Christoph has proposed > > > https://lore.kernel.org/linux-fsdevel/20210325082209.1067987-2-hch@lst.de > > > I was just saying that if we think this patch is useful we might bundle > > > it together with the > > > EXPORT_SYMBOL(receive_fd) > > > part here, convert all drivers that currently open-code get_unused_fd() > > > + fd_install() to use receive_fd(), and make this a separate patchset. > > > > > > > Yes, I see. We can split the parts (get_unused_fd() + fd_install()) of > > receive_fd() into a separate helper and convert all drivers to use > > that. What I mean is that I also would like to use > > security_file_receive() in my modules. So I'm not sure if it's ok to > > add security_file_receive() into the separate helper. Or do I need to > > export security_file_receive() separately? > > I think I confused you which is my bad. What you do here is - in my > opinion - correct. > I'm just saying that exporting receive_fd() allows further cleanups and > your export here could go on top of Christoph's change in a separate > series. > Oh, I get you now! I'm glad to do that. Thanks, Yongji
diff --git a/fs/file.c b/fs/file.c index dab120b71e44..d7d957217576 100644 --- a/fs/file.c +++ b/fs/file.c @@ -1108,6 +1108,12 @@ int __receive_fd(int fd, struct file *file, int __user *ufd, unsigned int o_flag return new_fd; } +int receive_fd(struct file *file, unsigned int o_flags) +{ + return __receive_fd(-1, file, NULL, o_flags); +} +EXPORT_SYMBOL(receive_fd); + static int ksys_dup3(unsigned int oldfd, unsigned int newfd, int flags) { int err = -EBADF; diff --git a/include/linux/file.h b/include/linux/file.h index 225982792fa2..4667f9567d3e 100644 --- a/include/linux/file.h +++ b/include/linux/file.h @@ -94,6 +94,9 @@ extern void fd_install(unsigned int fd, struct file *file); extern int __receive_fd(int fd, struct file *file, int __user *ufd, unsigned int o_flags); + +extern int receive_fd(struct file *file, unsigned int o_flags); + static inline int receive_fd_user(struct file *file, int __user *ufd, unsigned int o_flags) { @@ -101,10 +104,6 @@ static inline int receive_fd_user(struct file *file, int __user *ufd, return -EFAULT; return __receive_fd(-1, file, ufd, o_flags); } -static inline int receive_fd(struct file *file, unsigned int o_flags) -{ - return __receive_fd(-1, file, NULL, o_flags); -} static inline int receive_fd_replace(int fd, struct file *file, unsigned int o_flags) { return __receive_fd(fd, file, NULL, o_flags);
Export receive_fd() so that some modules can use it to pass file descriptor between processes without missing any security stuffs. Signed-off-by: Xie Yongji <xieyongji@bytedance.com> --- fs/file.c | 6 ++++++ include/linux/file.h | 7 +++---- 2 files changed, 9 insertions(+), 4 deletions(-)