Re: [PATCH 1/2] pid: add pidfd_open()
Hi,
Le mercredi 15 mai 2019 à 12:03 +0200, Christian Brauner a écrit :
>
> diff --git a/kernel/pid.c b/kernel/pid.c
> index 20881598bdfa..237d18d6ecb8 100644
> --- a/kernel/pid.c
> +++ b/kernel/pid.c
> @@ -451,6 +452,53 @@ struct pid *find_ge_pid(int nr, struct
> pid_namespace *ns)
> return idr_get_next(>idr, );
> }
>
> +/**
> + * pidfd_open() - Open new pid file descriptor.
> + *
> + * @pid: pid for which to retrieve a pidfd
> + * @flags: flags to pass
> + *
> + * This creates a new pid file descriptor with the O_CLOEXEC flag set for
> + * the process identified by @pid. Currently, the process identified by
> + * @pid must be a thread-group leader. This restriction currently exists
> + * for all aspects of pidfds including pidfd creation (CLONE_PIDFD cannot
> + * be used with CLONE_THREAD) and pidfd polling (only supports thread group
> + * leaders).
> + *
Would it be possible to create file descriptor with "restricted"
operation ?
- O_RDONLY: waiting for process completion allowed (for example)
- O_WRONLY: sending process signal allowed
For example, a process could send over a Unix socket a process a pidfd,
allowing this to only wait for completion, but not sending signal ?
I see the permission check is not done in pidfd_open(), so what prevent
a user from sending a signal to another user owned process ?
If it's in pidfd_send_signal(), then, passing the socket through
SCM_RIGHT won't be useful if the target process is not owned by the
same user, or root.
> + * Return: On success, a cloexec pidfd is returned.
> + * On error, a negative errno number will be returned.
> + */
> +SYSCALL_DEFINE2(pidfd_open, pid_t, pid, unsigned int, flags)
> +{
> + int fd, ret;
> + struct pid *p;
> + struct task_struct *tsk;
> +
> + if (flags)
> + return -EINVAL;
> +
> + if (pid <= 0)
> + return -EINVAL;
> +
> + p = find_get_pid(pid);
> + if (!p)
> + return -ESRCH;
> +
> + rcu_read_lock();
> + tsk = pid_task(p, PIDTYPE_PID);
> + if (!tsk)
> + ret = -ESRCH;
> + else if (unlikely(!thread_group_leader(tsk)))
> + ret = -EINVAL;
> + else
> + ret = 0;
> + rcu_read_unlock();
> +
> + fd = ret ?: pidfd_create(p);
> + put_pid(p);
> + return fd;
> +}
> +
> void __init pid_idr_init(void)
> {
> /* Verify no one has done anything silly: */
Regards.
--
Yann Droneaud
OPTEYA
Re: [PATCH 1/2] pid: add pidfd_open()
Hi, Le mercredi 15 mai 2019 à 16:16 +0200, Christian Brauner a écrit : > On Wed, May 15, 2019 at 04:00:20PM +0200, Yann Droneaud wrote: > > Le mercredi 15 mai 2019 à 12:03 +0200, Christian Brauner a écrit : > > > diff --git a/kernel/pid.c b/kernel/pid.c > > > index 20881598bdfa..237d18d6ecb8 100644 > > > --- a/kernel/pid.c > > > +++ b/kernel/pid.c > > > @@ -451,6 +452,53 @@ struct pid *find_ge_pid(int nr, struct > > > pid_namespace *ns) > > > return idr_get_next(>idr, ); > > > } > > > > > > +/** > > > + * pidfd_open() - Open new pid file descriptor. > > > + * > > > + * @pid: pid for which to retrieve a pidfd > > > + * @flags: flags to pass > > > + * > > > + * This creates a new pid file descriptor with the O_CLOEXEC flag set for > > > + * the process identified by @pid. Currently, the process identified by > > > + * @pid must be a thread-group leader. This restriction currently exists > > > + * for all aspects of pidfds including pidfd creation (CLONE_PIDFD cannot > > > + * be used with CLONE_THREAD) and pidfd polling (only supports thread > > > group > > > + * leaders). > > > + * > > > > Would it be possible to create file descriptor with "restricted" > > operation ? > > > > - O_RDONLY: waiting for process completion allowed (for example) > > - O_WRONLY: sending process signal allowed > > Yes, something like this is likely going to be possible in the future. > We had discussion around this. But mapping this to O_RDONLY and O_WRONLY > is not the right model. It makes more sense to have specialized flags > that restrict actions. Yes, dedicated flags are the way to go. I've used the old open() flags here as examples as an echo of the O_CLOEXEC flag used in the comment. > > For example, a process could send over a Unix socket a process a pidfd, > > allowing this to only wait for completion, but not sending signal ? > > > > I see the permission check is not done in pidfd_open(), so what prevent > > a user from sending a signal to another user owned process ? > > That's supposed to be possible. You can do the same right now already > with pids. Tools like LMK need this probably very much. > Permission checking for signals is done at send time right now. > And if you can't signal via a pid you can't signal via a pidfd as > they're both subject to the same permissions checks. > I would have expect it to behave like most other file descriptor, permission check done at opening time, which allow more privileged process to open the file descriptor, then pass it to a less privileged process, or change its own privileged with setuid() and such. Then the less privileged process can act on behalf of the privileged process through the file descriptor. > > If it's in pidfd_send_signal(), then, passing the socket through > > SCM_RIGHT won't be useful if the target process is not owned by the > > same user, or root. > > If the permission check is done at sending time, the scenario above case cannot be implemented. Sending a pidfd through SCM_RIGHT is only useful if the receiver process is equally or more privileged than the sender then. For isolation purpose, I would have expect to be able to give a right to send a signal to a highly privileged process a specific less privileged process though Unix socket. But I can't come up with a specific use case. So I dunno. Regards. -- Yann Droneaud OPTEYA
Re: [PATCHv9 2/5] ppc/cell: trivial: replace get_unused_fd() by get_unused_fd_flags(0)
Hi, Le mardi 14 octobre 2014 à 12:57 +1100, Michael Ellerman a écrit : On Mon, 2014-10-13 at 21:30 +0200, Yann Droneaud wrote: This patch replaces calls to get_unused_fd() with equivalent call to get_unused_fd_flags(0) to preserve current behavor for existing code. In a further patch, get_unused_fd() will be removed so that new code start using get_unused_fd_flags(), with the hope O_CLOEXEC could be used, either by default or choosen by userspace. Link: http://lkml.kernel.org/r/cover.1413223900.git.ydrone...@opteya.com Cc: Al Viro v...@zeniv.linux.org.uk Cc: Andrew Morton a...@linux-foundation.org Cc: triv...@kernel.org Signed-off-by: Yann Droneaud ydrone...@opteya.com This is fine by me, do you want an ack, or do you want us to take it via the powerpc tree? The patch was added in -mm by Andrew, so I guess the patch will be merged sooner or later. Anyway, you could investigate to check if O_CLOEXEC could be used instead of 0 in call to get_unused_fd_flags(). If the former: Acked-by: Michael Ellerman m...@ellerman.id.au Thanks a lot. Regards. -- Yann Droneaud OPTEYA ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCHv9 2/5] ppc/cell: trivial: replace get_unused_fd() by get_unused_fd_flags(0)
This patch replaces calls to get_unused_fd() with equivalent call to get_unused_fd_flags(0) to preserve current behavor for existing code. In a further patch, get_unused_fd() will be removed so that new code start using get_unused_fd_flags(), with the hope O_CLOEXEC could be used, either by default or choosen by userspace. Link: http://lkml.kernel.org/r/cover.1413223900.git.ydrone...@opteya.com Cc: Al Viro v...@zeniv.linux.org.uk Cc: Andrew Morton a...@linux-foundation.org Cc: triv...@kernel.org Signed-off-by: Yann Droneaud ydrone...@opteya.com --- arch/powerpc/platforms/cell/spufs/inode.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/arch/powerpc/platforms/cell/spufs/inode.c b/arch/powerpc/platforms/cell/spufs/inode.c index 87ba7cf99cd7..51effcec30d8 100644 --- a/arch/powerpc/platforms/cell/spufs/inode.c +++ b/arch/powerpc/platforms/cell/spufs/inode.c @@ -301,7 +301,7 @@ static int spufs_context_open(struct path *path) int ret; struct file *filp; - ret = get_unused_fd(); + ret = get_unused_fd_flags(0); if (ret 0) return ret; @@ -518,7 +518,7 @@ static int spufs_gang_open(struct path *path) int ret; struct file *filp; - ret = get_unused_fd(); + ret = get_unused_fd_flags(0); if (ret 0) return ret; -- 1.9.3 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCHv9 0/5] Getting rid of get_unused_fd()
Hi, Please find the 9th revision of my patchset to remove get_unused_fd() macro. In linux-next, tag next-20141013, they're currently: - 33 calls to fd_install() with one call part of anon_inode_getfd() - 27 calls to get_unused_fd_flags() with one call part of anon_inode_getfd() with another part of get_unused_fd() macro - 13 calls to anon_inode_getfd() - 8 calls to anon_inode_getfile() with one call part of anon_inode_getfd() - 5 calls to get_unused_fd() The following patchset replaces the 5 last calls to get_unused_fd() by calls to get_unused_fd_flags(0) and remove the macro so that it won't be used in newer code. For some detailed background information, please have a look at my previous patchset's cover letter[1]. Changes from patchset v8[1] - fanotify: enable close-on-exec on events' fd when requested in fanotify_init() DROPPED: applied upstream, commit 0b37e097a648. - reduce the amount of explanation in cover letter [1] http://lkml.kernel.org/r/cover.1411562410.git.ydrone...@opteya.com Yann Droneaud (5): ia64: trivial: replace get_unused_fd() by get_unused_fd_flags(0) ppc/cell: trivial: replace get_unused_fd() by get_unused_fd_flags(0) binfmt_misc: trivial: replace get_unused_fd() by get_unused_fd_flags(0) file: trivial: replace get_unused_fd() by get_unused_fd_flags(0) file: remove get_unused_fd() macro arch/ia64/kernel/perfmon.c| 2 +- arch/powerpc/platforms/cell/spufs/inode.c | 4 ++-- fs/binfmt_misc.c | 2 +- fs/file.c | 2 +- include/linux/file.h | 1 - 5 files changed, 5 insertions(+), 6 deletions(-) -- 1.9.3 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCHv8 3/6] ppc/cell: trivial: replace get_unused_fd() by get_unused_fd_flags(0)
This patch replaces calls to get_unused_fd() with equivalent call to get_unused_fd_flags(0) to preserve current behavor for existing code. In a further patch, get_unused_fd() will be removed so that new code start using get_unused_fd_flags(), with the hope O_CLOEXEC could be used, either by default or choosen by userspace. Link: http://lkml.kernel.org/r/cover.1411562410.git.ydrone...@opteya.com Cc: Al Viro v...@zeniv.linux.org.uk Cc: Andrew Morton a...@linux-foundation.org Cc: triv...@kernel.org Signed-off-by: Yann Droneaud ydrone...@opteya.com --- arch/powerpc/platforms/cell/spufs/inode.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/arch/powerpc/platforms/cell/spufs/inode.c b/arch/powerpc/platforms/cell/spufs/inode.c index 87ba7cf99cd7..51effcec30d8 100644 --- a/arch/powerpc/platforms/cell/spufs/inode.c +++ b/arch/powerpc/platforms/cell/spufs/inode.c @@ -301,7 +301,7 @@ static int spufs_context_open(struct path *path) int ret; struct file *filp; - ret = get_unused_fd(); + ret = get_unused_fd_flags(0); if (ret 0) return ret; @@ -518,7 +518,7 @@ static int spufs_gang_open(struct path *path) int ret; struct file *filp; - ret = get_unused_fd(); + ret = get_unused_fd_flags(0); if (ret 0) return ret; -- 1.9.3 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCHv8 0/6] Getting rid of get_unused_fd() / enable close-on-exec
.git.ydrone...@opteya.com [PATCHSETv5] http://lkml.kernel.org/r/cover.1388952061.git.ydrone...@opteya.com [PATCHSETv4] http://lkml.kernel.org/r/cover.1383121137.git.ydrone...@opteya.com [PATCHSETv3] http://lkml.kernel.org/r/cover.1378460926.git.ydrone...@opteya.com [PATCHSETv2] http://lkml.kernel.org/r/cover.1376327678.git.ydrone...@opteya.com [PATCHSETv1] http://lkml.kernel.org/r/cover.1372777600.git.ydrone...@opteya.com Yann Droneaud (6): fanotify: enable close-on-exec on events' fd when requested in fanotify_init() ia64: trivial: replace get_unused_fd() by get_unused_fd_flags(0) ppc/cell: trivial: replace get_unused_fd() by get_unused_fd_flags(0) binfmt_misc: trivial: replace get_unused_fd() by get_unused_fd_flags(0) file: trivial: replace get_unused_fd() by get_unused_fd_flags(0) file: remove get_unused_fd() macro arch/ia64/kernel/perfmon.c| 2 +- arch/powerpc/platforms/cell/spufs/inode.c | 4 ++-- fs/binfmt_misc.c | 2 +- fs/file.c | 2 +- fs/notify/fanotify/fanotify_user.c| 2 +- include/linux/file.h | 1 - 6 files changed, 6 insertions(+), 7 deletions(-) -- 1.9.3 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCHv7 0/6] Getting rid of get_unused_fd() / enable close-on-exec
[PATCHSETv1] http://lkml.kernel.org/r/cover.1372777600.git.ydrone...@opteya.com Yann Droneaud (6): ia64: use get_unused_fd_flags(0) instead of get_unused_fd() ppc/cell: use get_unused_fd_flags(0) instead of get_unused_fd() binfmt_misc: use get_unused_fd_flags(0) instead of get_unused_fd() file: use get_unused_fd_flags(0) instead of get_unused_fd() fanotify: enable close-on-exec on events' fd when requested in fanotify_init() file: remove macro get_unused_fd() arch/ia64/kernel/perfmon.c| 2 +- arch/powerpc/platforms/cell/spufs/inode.c | 4 ++-- fs/binfmt_misc.c | 2 +- fs/file.c | 2 +- fs/notify/fanotify/fanotify_user.c| 2 +- include/linux/file.h | 1 - 6 files changed, 6 insertions(+), 7 deletions(-) -- 1.9.3 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCHv7 2/6] ppc/cell: use get_unused_fd_flags(0) instead of get_unused_fd()
Macro get_unused_fd() is used to allocate a file descriptor with default flags. Those default flags (0) can be unsafe: O_CLOEXEC must be used by default to not leak file descriptor across exec(). Instead of macro get_unused_fd(), function get_unused_fd_flags() (or anon_inode_getfd()) should be used with flags given by userspace. If not possible, flags should be set to O_CLOEXEC to provide userspace with a default safe behavor. In a further patch, get_unused_fd() will be removed so that new code start using get_unused_fd_flags() (or anon_inode_getfd()) with correct flags. This patch replaces calls to get_unused_fd() with equivalent call to get_unused_fd_flags(0) to preserve current behavor for existing code. The hard coded flag value (0) should be reviewed on a per-subsystem basis, and, if possible, set to O_CLOEXEC. Link: http://lkml.kernel.org/r/cover.1401630396.git.ydrone...@opteya.com Cc: Al Viro v...@zeniv.linux.org.uk Cc: Andrew Morton a...@linux-foundation.org Signed-off-by: Yann Droneaud ydrone...@opteya.com --- arch/powerpc/platforms/cell/spufs/inode.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/arch/powerpc/platforms/cell/spufs/inode.c b/arch/powerpc/platforms/cell/spufs/inode.c index 87ba7cf99cd7..51effcec30d8 100644 --- a/arch/powerpc/platforms/cell/spufs/inode.c +++ b/arch/powerpc/platforms/cell/spufs/inode.c @@ -301,7 +301,7 @@ static int spufs_context_open(struct path *path) int ret; struct file *filp; - ret = get_unused_fd(); + ret = get_unused_fd_flags(0); if (ret 0) return ret; @@ -518,7 +518,7 @@ static int spufs_gang_open(struct path *path) int ret; struct file *filp; - ret = get_unused_fd(); + ret = get_unused_fd_flags(0); if (ret 0) return ret; -- 1.9.3 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
cscope: issue with symlinks in tools/testing/selftests/powerpc/copyloops/
Hi, I'm using cscope to browse kernel sources, but I'm facing warnings from the tool since following commit: http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=22d651dcef536c75f75537290bf3da5038e68b6b commit 22d651dcef536c75f75537290bf3da5038e68b6b Author: Michael Ellerman m...@ellerman.id.au Date: Tue Jan 21 15:22:17 2014 +1100 selftests/powerpc: Import Anton's memcpy / copy_tofrom_user tests Turn Anton's memcpy / copy_tofrom_user test into something that can live in tools/testing/selftests. It requires one turd in arch/powerpc/lib/memcpy_64.S, but it's pretty harmless IMHO. We are sailing very close to the wind with the feature macros. We define them to nothing, which currently means we get a few extra nops and include the unaligned calls. Signed-off-by: Anton Blanchard an...@samba.org Signed-off-by: Michael Ellerman m...@ellerman.id.au Signed-off-by: Benjamin Herrenschmidt b...@kernel.crashing.org cscope reports error when generating the cross-reference database: $ make ALLSOURCE_ARCHS=all O=./obj-cscope/ cscope GEN cscope cscope: cannot find file /home/ydroneaud/src/linux/tools/testing/selftests/powerpc/copyloops/copyuser_power7.S cscope: cannot find file /home/ydroneaud/src/linux/tools/testing/selftests/powerpc/copyloops/memcpy_64.S cscope: cannot find file /home/ydroneaud/src/linux/tools/testing/selftests/powerpc/copyloops/memcpy_power7.S cscope: cannot find file /home/ydroneaud/src/linux/tools/testing/selftests/powerpc/copyloops/copyuser_64.S And when calling cscope from ./obj-cscope/ directory, it reports errors too. Hopefully it doesn't stop it from working, so I'm still able to use cscope to browse kernel sources. It's a rather uncommon side effect of having (for the first time ?) sources files as symlinks: looking for symlinks in the kernel sources returns only: $ find . -type l ./arch/mips/boot/dts/include/dt-bindings ./arch/microblaze/boot/dts/system.dts ./arch/powerpc/boot/dts/include/dt-bindings ./arch/metag/boot/dts/include/dt-bindings ./arch/arm/boot/dts/include/dt-bindings ./tools/testing/selftests/powerpc/copyloops/copyuser_power7.S ./tools/testing/selftests/powerpc/copyloops/memcpy_64.S ./tools/testing/selftests/powerpc/copyloops/memcpy_power7.S ./tools/testing/selftests/powerpc/copyloops/copyuser_64.S ./obj-cscope/source ./Documentation/DocBook/vidioc-g-sliced-vbi-cap.xml ./Documentation/DocBook/vidioc-decoder-cmd.xml ... ./Documentation/DocBook/media-func-ioctl.xml ./Documentation/DocBook/vidioc-enumoutput.xml So one can wonder if having symlinked sources files is an expected supported feature for kbuild and all the various kernel tools/infrastructure ? Regarding cscope specifically, it does not support symlink, and it's the expected behavior according to the bug reports I was able to find: #214 cscope ignores symlinks to files http://sourceforge.net/p/cscope/bugs/214/ #229 -I options doesn't handle symbolic link http://sourceforge.net/p/cscope/bugs/229/ #247 cscope: cannot find file http://sourceforge.net/p/cscope/bugs/247/ #252 cscope: cannot find file *** http://sourceforge.net/p/cscope/bugs/252/ #261 Regression - version 15.7a does not follow symbolic links http://sourceforge.net/p/cscope/bugs/261/ Regards. -- Yann Droneaud OPTEYA ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCHv6 0/6] Getting rid of get_unused_fd() / enable close-on-exec
to use it in a different process context, thus O_CLOEXEC must not be the default. If one, as a subsystem maintainer, cannot tell for sure that no userspace program ever rely current behavior, eg. file descriptor being inherited across exec(), then the default flag *must* be kept 0 to not break application. - This subsystem cannot be turned to use O_CLOEXEC by default: If O_CLOEXEC cannot be made the default, it would be interesting to think to extend the API to have a (set of) function(s) taking a flag parameter so that userspace can atomically request close-on-exec if it need it (and it should need it). - Background: One might want to read Secure File Descriptor Handling [2] by Ulrich Drepper who is responsible of adding O_CLOEXEC flag on open(), and flags alike on other syscalls. One might also want to read PEP-446 Make newly created file descriptors non-inheritable [3] by Victor Stinner since it has lot more background information on file descriptor leaking. One also like to read Excuse me son, but your code is leaking !!! [4] by Dan Walsh for advice. [1] http://lwn.net/Articles/412131/ [2] http://udrepper.livejournal.com/20407.html [3] http://www.python.org/dev/peps/pep-0446/ [4] http://danwalsh.livejournal.com/53603.html - Statistics: In linux-next tag 20140307, they're currently: - 32 calls to fd_install() with one call part of anon_inode_getfd() - 25 calls to get_unused_fd_flags() with one call part of anon_inode_getfd() with another part of get_unused_fd() macro - 11 calls to anon_inode_getfd() - 9 calls to anon_inode_getfile() with one call part of anon_inode_getfd() - 6 calls to get_unused_fd() Changes from patchset v5 [PATCHSETv5] - perf: introduce a flag to enable close-on-exec in perf_event_open() DROPPED: applied upstream, commit a21b0b354d4a. Changes from patchset v4 [PATCHSETv4]: - rewrote cover letter following discussion with perf maintainer. Thanks to Peter Zijlstra. - modified a bit some commit messages. - events: use get_unused_fd_flags(0) instead of get_unused_fd() DROPPED: replaced by following patch. - perf: introduce a flag to enable close-on-exec in perf_event_open() NEW: instead of hard coding the flags to 0, this patch allows userspace to specify close-on-exec flag. - fanotify: use get_unused_fd_flags(0) instead of get_unused_fd() DROPPED: replaced by following patch. - fanotify: enable close-on-exec on events' fd when requested in fanotify_init() NEW: instead of hard coding the flags to 0, this patch enable close-on-exec if userspace request it. Changes from patchset v3 [PATCHSETv3]: - industrialio: use anon_inode_getfd() with O_CLOEXEC flag DROPPED: applied upstream, commit a646fbf0fd11. Changes from patchset v2 [PATCHSETv2]: - android/sw_sync: use get_unused_fd_flags(O_CLOEXEC) instead of get_unused_fd() DROPPED: applied upstream, commit 45acea57335e. - android/sync: use get_unused_fd_flags(O_CLOEXEC) instead of get_unused_fd() DROPPED: applied upstream, commit 9c6cd3b39048. - vfio: use get_unused_fd_flags(0) instead of get_unused_fd() DROPPED: applied upstream, commit a5d550703d2c. Additionally subsystem maintainer applied another patch on top to set the flags to O_CLOEXEC, commit 5d042fbdbb2d. - industrialio: use anon_inode_getfd() with O_CLOEXEC flag NEW: propose to use O_CLOEXEC as default flag. Changes from patchset v1 [PATCHSETv1]: - explicitly added subsystem maintainers as mail recepients. - infiniband: use get_unused_fd_flags(0) instead of get_unused_fd() DROPPED: subsystem maintainer applied another patch using get_unused_fd_flags(O_CLOEXEC) as suggested, commit da183c7af844. - android/sw_sync: use get_unused_fd_flags(0) instead of get_unused_fd() MODIFIED: use get_unused_fd_flags(O_CLOEXEC) as suggested. - android/sync: use get_unused_fd_flags(0) instead of get_unused_fd() MODIFIED: use get_unused_fd_flags(O_CLOEXEC) as suggested. - xfs: use get_unused_fd_flags(0) instead of get_unused_fd() DROPPED: applied asis by subsystem maintainer, commit 862a62937e76. - sctp: use get_unused_fd_flags(0) instead of get_unused_fd() DROPPED: applied asis by subsystem maintainer, commit 8a59bd3e9b29. Links: [PATCHSETv5] http://lkml.kernel.org/r/cover.1388952061.git.ydrone...@opteya.com [PATCHSETv4] http://lkml.kernel.org/r/cover.1383121137.git.ydrone...@opteya.com [PATCHSETv3] http://lkml.kernel.org/r/cover.1378460926.git.ydrone...@opteya.com [PATCHSETv2] http://lkml.kernel.org/r/cover.1376327678.git.ydrone...@opteya.com [PATCHSETv1] http://lkml.kernel.org/r/cover.1372777600.git.ydrone...@opteya.com Yann Droneaud (6): ia64: use get_unused_fd_flags(0) instead of get_unused_fd() ppc/cell: use get_unused_fd_flags(0) instead of get_unused_fd() binfmt_misc: use get_unused_fd_flags(0) instead of get_unused_fd() file: use get_unused_fd_flags(0) instead of get_unused_fd() fanotify: enable close
[PATCHv6 2/6] ppc/cell: use get_unused_fd_flags(0) instead of get_unused_fd()
Macro get_unused_fd() is used to allocate a file descriptor with default flags. Those default flags (0) can be unsafe: O_CLOEXEC must be used by default to not leak file descriptor across exec(). Instead of macro get_unused_fd(), function get_unused_fd_flags() (or anon_inode_getfd()) should be used with flags given by userspace. If not possible, flags should be set to O_CLOEXEC to provide userspace with a default safe behavor. In a further patch, get_unused_fd() will be removed so that new code start using get_unused_fd_flags() (or anon_inode_getfd()) with correct flags. This patch replaces calls to get_unused_fd() with equivalent call to get_unused_fd_flags(0) to preserve current behavor for existing code. The hard coded flag value (0) should be reviewed on a per-subsystem basis, and, if possible, set to O_CLOEXEC. Link: http://lkml.kernel.org/r/cover.1394532336.git.ydrone...@opteya.com Cc: Al Viro v...@zeniv.linux.org.uk Cc: Andrew Morton a...@linux-foundation.org Signed-off-by: Yann Droneaud ydrone...@opteya.com --- arch/powerpc/platforms/cell/spufs/inode.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/arch/powerpc/platforms/cell/spufs/inode.c b/arch/powerpc/platforms/cell/spufs/inode.c index 87ba7cf99cd7..51effcec30d8 100644 --- a/arch/powerpc/platforms/cell/spufs/inode.c +++ b/arch/powerpc/platforms/cell/spufs/inode.c @@ -301,7 +301,7 @@ static int spufs_context_open(struct path *path) int ret; struct file *filp; - ret = get_unused_fd(); + ret = get_unused_fd_flags(0); if (ret 0) return ret; @@ -518,7 +518,7 @@ static int spufs_gang_open(struct path *path) int ret; struct file *filp; - ret = get_unused_fd(); + ret = get_unused_fd_flags(0); if (ret 0) return ret; -- 1.8.5.3 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 02/13] ppc/cell: use get_unused_fd_flags(0) instead of get_unused_fd()
Hi Benjamin, Le lundi 13 janvier 2014 à 10:06 +1100, Benjamin Herrenschmidt a écrit : On Tue, 2013-07-02 at 18:39 +0200, Yann Droneaud wrote: Macro get_unused_fd() is used to allocate a file descriptor with default flags. Those default flags (0) can be unsafe: O_CLOEXEC must be used by default to not leak file descriptor across exec(). Instead of macro get_unused_fd(), functions anon_inode_getfd() or get_unused_fd_flags() should be used with flags given by userspace. If not possible, flags should be set to O_CLOEXEC to provide userspace with a default safe behavor. In a further patch, get_unused_fd() will be removed so that new code start using anon_inode_getfd() or get_unused_fd_flags() with correct flags. This patch replaces calls to get_unused_fd() with equivalent call to get_unused_fd_flags(0) to preserve current behavor for existing code. The hard coded flag value (0) should be reviewed on a per-subsystem basis, and, if possible, set to O_CLOEXEC. Signed-off-by: Yann Droneaud ydrone...@opteya.com Should I merge this (v5 on patchwork) or let Al do it ? Please merge it directly: patches from the previous patchsets were picked individually by each subsystem maintainer after proper review regarding setting close on exec flag by default. Acked-by: Benjamin Herrenschmidt b...@kernel.crashing.org Thanks a lot. --- arch/powerpc/platforms/cell/spufs/inode.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/arch/powerpc/platforms/cell/spufs/inode.c b/arch/powerpc/platforms/cell/spufs/inode.c index f390042..88df441 100644 --- a/arch/powerpc/platforms/cell/spufs/inode.c +++ b/arch/powerpc/platforms/cell/spufs/inode.c @@ -301,7 +301,7 @@ static int spufs_context_open(struct path *path) int ret; struct file *filp; - ret = get_unused_fd(); + ret = get_unused_fd_flags(0); if (ret 0) return ret; @@ -518,7 +518,7 @@ static int spufs_gang_open(struct path *path) int ret; struct file *filp; - ret = get_unused_fd(); + ret = get_unused_fd_flags(0); if (ret 0) return ret; Note: latest patch (from v5 patchset) is at http://lkml.kernel.org/r/fe27abcfab5563d36a3e5e58ff36e5500c39be6a.1388952061.git.ydrone...@opteya.com v5 patchset is at http://lkml.kernel.org/r/cover.1388952061.git.ydrone...@opteya.com Regards. -- Yann Droneaud OPTEYA ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH v5 0/7] Getting rid of get_unused_fd() / enable close-on-exec
to use it in a different process context, thus O_CLOEXEC must not be the default. If one, as a subsystem maintainer, cannot tell for sure that no userspace program ever rely current behavior, eg. file descriptor being inherited across exec(), then the default flag *must* be kept 0 to not break application. - This subsystem cannot be turned to use O_CLOEXEC by default: If O_CLOEXEC cannot be made the default, it would be interesting to think to extend the API to have a (set of) function(s) taking a flag parameter so that userspace can atomically request close-on-exec if it need it (and it should need it). - Background: One might want to read Secure File Descriptor Handling [2] by Ulrich Drepper who is responsible of adding O_CLOEXEC flag on open(), and flags alike on other syscalls. One might also want to read PEP-446 Make newly created file descriptors non-inheritable [3] by Victor Stinner since it has lot more background information on file descriptor leaking. One also like to read Excuse me son, but your code is leaking !!! [4] by Dan Walsh for advice. [1] http://lwn.net/Articles/412131/ [2] http://udrepper.livejournal.com/20407.html [3] http://www.python.org/dev/peps/pep-0446/ [4] http://danwalsh.livejournal.com/53603.html - Statistics: In linux-next tag 20131224, they're currently: - 32 calls to fd_install() with one call part of anon_inode_getfd() - 24 calls to get_unused_fd_flags() with one call part of anon_inode_getfd() with another part of get_unused_fd() macro - 11 calls to anon_inode_getfd() - 8 calls to anon_inode_getfile() with one call part of anon_inode_getfd() - 7 calls to get_unused_fd() Changes from patchset v4 [PATCHSETv4]: - rewrote cover letter following discussion with perf maintainer. Thanks to Peter Zijlstra. - modified a bit some commit messages. - events: use get_unused_fd_flags(0) instead of get_unused_fd() DROPPED: replaced by following patch - perf: introduce a flag to enable close-on-exec in perf_event_open() NEW: instead of hard coding the flags to 0, this patch allows userspace to specify close-on-exec flag. - fanotify: use get_unused_fd_flags(0) instead of get_unused_fd() DROPPED: replaced by following patch - fanotify: enable close-on-exec on events' fd when requested in fanotify_init() NEW: instead of hard coding the flags to 0, this patch enable close-on-exec if userspace request it. Changes from patchset v3 [PATCHSETv3]: - industrialio: use anon_inode_getfd() with O_CLOEXEC flag DROPPED: applied upstream Changes from patchset v2 [PATCHSETv2]: - android/sw_sync: use get_unused_fd_flags(O_CLOEXEC) instead of get_unused_fd() DROPPED: applied upstream - android/sync: use get_unused_fd_flags(O_CLOEXEC) instead of get_unused_fd() DROPPED: applied upstream - vfio: use get_unused_fd_flags(0) instead of get_unused_fd() DROPPED: applied upstream. Additionally subsystem maintainer applied another patch on top to set the flags to O_CLOEXEC. - industrialio: use anon_inode_getfd() with O_CLOEXEC flag NEW: propose to use O_CLOEXEC as default flag. Changes from patchset v1 [PATCHSETv1]: - explicitly added subsystem maintainers as mail recepients. - infiniband: use get_unused_fd_flags(0) instead of get_unused_fd() DROPPED: subsystem maintainer applied another patch using get_unused_fd_flags(O_CLOEXEC) as suggested. - android/sw_sync: use get_unused_fd_flags(0) instead of get_unused_fd() MODIFIED: use get_unused_fd_flags(O_CLOEXEC) as suggested - android/sync: use get_unused_fd_flags(0) instead of get_unused_fd() MODIFIED: use get_unused_fd_flags(O_CLOEXEC) as suggested - xfs: use get_unused_fd_flags(0) instead of get_unused_fd() DROPPED: applied asis by subsystem maintainer. - sctp: use get_unused_fd_flags(0) instead of get_unused_fd() DROPPED: applied asis by subsystem maintainer. Links: [PATCHSETv4] http://lkml.kernel.org/r/cover.1383121137.git.ydrone...@opteya.com [PATCHSETv3] http://lkml.kernel.org/r/cover.1378460926.git.ydrone...@opteya.com [PATCHSETv2] http://lkml.kernel.org/r/cover.1376327678.git.ydrone...@opteya.com [PATCHSETv1] http://lkml.kernel.org/r/cover.1372777600.git.ydrone...@opteya.com PS: Happy new (gregorian calendar's) year 2014 and best wishes ;) Yann Droneaud (7): ia64: use get_unused_fd_flags(0) instead of get_unused_fd() ppc/cell: use get_unused_fd_flags(0) instead of get_unused_fd() binfmt_misc: use get_unused_fd_flags(0) instead of get_unused_fd() file: use get_unused_fd_flags(0) instead of get_unused_fd() fanotify: enable close-on-exec on events' fd when requested in fanotify_init() perf: introduce a flag to enable close-on-exec in perf_event_open() file: remove macro get_unused_fd() arch/ia64/kernel/perfmon.c| 2 +- arch/powerpc/platforms/cell/spufs/inode.c | 4 ++-- fs/binfmt_misc.c | 2 +- fs/file.c
[PATCH v4 0/7] Getting rid of get_unused_fd()
Hi, Please find the fourth revision of my patchset to remove get_unused_fd() macro in order to encourage subsystems to use get_unused_fd_flags() or anon_inode_getfd() with open flags set to O_CLOEXEC were appropriate. The patchset convert all calls to get_unused_fd() to get_unused_fd_flags(0) before removing get_unused_fd() macro. Without get_unused_fd() macro, more subsystems are likely to use anon_inode_getfd() and be teached to provide an API that let userspace choose the opening flags of the file descriptor. Not using O_CLOEXEC by default or not letting userspace provide the open flags should be considered bad practice from security point of view: in most case O_CLOEXEC must be used to not leak file descriptor across exec(). Using O_CLOEXEC by default when flags are not provided by userspace allows userspace to set, using fcntl(), without any risk of race, if the file descriptor is going to be inherited or not across exec(). Status: In linux-next tag 20131029, they're currently: - 32 calls to fd_install() - 23 calls to get_unused_fd_flags() - 11 calls to anon_inode_getfd() - 7 calls to get_unused_fd() Changes from patchset v3 [PATCHSETv3]: - industrialio: use anon_inode_getfd() with O_CLOEXEC flag DROPPED: applied upstream Changes from patchset v2 [PATCHSETv2]: - android/sw_sync: use get_unused_fd_flags(O_CLOEXEC) instead of get_unused_fd() DROPPED: applied upstream - android/sync: use get_unused_fd_flags(O_CLOEXEC) instead of get_unused_fd() DROPPED: applied upstream - vfio: use get_unused_fd_flags(0) instead of get_unused_fd() DROPPED: applied upstream. Additionally subsystem maintainer applied another patch on top to set the flags to O_CLOEXEC. - industrialio: use anon_inode_getfd() with O_CLOEXEC flag NEW: propose to use O_CLOEXEC as default flag. Changes from patchset v1 [PATCHSETv1]: - explicitly added subsystem maintainers as mail recepients. - infiniband: use get_unused_fd_flags(0) instead of get_unused_fd() DROPPED: subsystem maintainer applied another patch using get_unused_fd_flags(O_CLOEXEC) as suggested. - android/sw_sync: use get_unused_fd_flags(0) instead of get_unused_fd() MODIFIED: use get_unused_fd_flags(O_CLOEXEC) as suggested - android/sync: use get_unused_fd_flags(0) instead of get_unused_fd() MODIFIED: use get_unused_fd_flags(O_CLOEXEC) as suggested - xfs: use get_unused_fd_flags(0) instead of get_unused_fd() DROPPED: applied asis by subsystem maintainer. - sctp: use get_unused_fd_flags(0) instead of get_unused_fd() DROPPED: applied asis by subsystem maintainer. Links: [PATCHSETv3] http://lkml.kernel.org/r/cover.1378460926.git.ydrone...@opteya.com [PATCHSETv2] http://lkml.kernel.org/r/cover.1376327678.git.ydrone...@opteya.com [PATCHSETv1] http://lkml.kernel.org/r/cover.1372777600.git.ydrone...@opteya.com Yann Droneaud (7): ia64: use get_unused_fd_flags(0) instead of get_unused_fd() ppc/cell: use get_unused_fd_flags(0) instead of get_unused_fd() binfmt_misc: use get_unused_fd_flags(0) instead of get_unused_fd() file: use get_unused_fd_flags(0) instead of get_unused_fd() fanotify: use get_unused_fd_flags(0) instead of get_unused_fd() events: use get_unused_fd_flags(0) instead of get_unused_fd() file: remove get_unused_fd() arch/ia64/kernel/perfmon.c| 2 +- arch/powerpc/platforms/cell/spufs/inode.c | 4 ++-- fs/binfmt_misc.c | 2 +- fs/file.c | 2 +- fs/notify/fanotify/fanotify_user.c| 2 +- include/linux/file.h | 1 - kernel/events/core.c | 2 +- 7 files changed, 7 insertions(+), 8 deletions(-) -- 1.8.3.1 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH v4 2/7] ppc/cell: use get_unused_fd_flags(0) instead of get_unused_fd()
Macro get_unused_fd() is used to allocate a file descriptor with default flags. Those default flags (0) can be unsafe: O_CLOEXEC must be used by default to not leak file descriptor across exec(). Instead of macro get_unused_fd(), functions anon_inode_getfd() or get_unused_fd_flags() should be used with flags given by userspace. If not possible, flags should be set to O_CLOEXEC to provide userspace with a default safe behavor. In a further patch, get_unused_fd() will be removed so that new code start using anon_inode_getfd() or get_unused_fd_flags() with correct flags. This patch replaces calls to get_unused_fd() with equivalent call to get_unused_fd_flags(0) to preserve current behavor for existing code. The hard coded flag value (0) should be reviewed on a per-subsystem basis, and, if possible, set to O_CLOEXEC. Signed-off-by: Yann Droneaud ydrone...@opteya.com Link: http://lkml.kernel.org/r/cover.1383121137.git.ydrone...@opteya.com --- arch/powerpc/platforms/cell/spufs/inode.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/arch/powerpc/platforms/cell/spufs/inode.c b/arch/powerpc/platforms/cell/spufs/inode.c index 87ba7cf..51effce 100644 --- a/arch/powerpc/platforms/cell/spufs/inode.c +++ b/arch/powerpc/platforms/cell/spufs/inode.c @@ -301,7 +301,7 @@ static int spufs_context_open(struct path *path) int ret; struct file *filp; - ret = get_unused_fd(); + ret = get_unused_fd_flags(0); if (ret 0) return ret; @@ -518,7 +518,7 @@ static int spufs_gang_open(struct path *path) int ret; struct file *filp; - ret = get_unused_fd(); + ret = get_unused_fd_flags(0); if (ret 0) return ret; -- 1.8.3.1 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH v3 0/8] Getting rid of get_unused_fd() / use O_CLOEXEC
Hi, With help from subsystem maintainers, I've managed to get some of get_unused_fd() calls converted to use get_unused_fd_flags(O_CLOEXEC) instead. [ANDROID][IB][VFIO] KVM subsystem maintainers helped me to change calls to anon_inode_getfd() to use O_CLOEXEC by default. [KVM] Some maintainers applied my patches to convert get_unused_fd() to get_unused_fd_flags(0) were using O_CLOEXEC wasn't possible without breaking ABI. [SCTP][XFS] So, still in the hope to get rid of get_unused_fd(), please find a another attempt to remove get_unused_fd() macro and encourage subsystems to use get_unused_fd_flags(O_CLOEXEC) or anon_inode_getfd(O_CLOEXEC) by default were appropriate. The patchset convert all calls to get_unused_fd() to get_unused_fd_flags(0) before removing get_unused_fd() macro. Without get_unused_fd() macro, more subsystems are likely to use anon_inode_getfd() and be teached to provide an API that let userspace choose the opening flags of the file descriptor. Additionally I'm suggesting Industrial IO (IIO) subsystem to use anon_inode_getfd(O_CLOEXEC): it's the only subsystem using anon_inode_getfd() with a fixed set of flags not including O_CLOEXEC. Not using O_CLOEXEC by default or not letting userspace provide the open flags should be considered bad practice from security point of view: in most case O_CLOEXEC must be used to not leak file descriptor across exec(). Using O_CLOEXEC by default when flags are not provided by userspace allows it to choose, without race, if the file descriptor is going to be inherited across exec(). Status: In linux-next tag 20130906, they're currently: - 22 calls to get_unused_fd_flags() (+3) not counting get_unused_fd() and anon_inode_getfd() - 7 calls to get_unused_fd() (-3) - 11 calls to anon_inode_getfd()(0) Changes from patchset v2 [PATCHSETv2]: - android/sw_sync: use get_unused_fd_flags(O_CLOEXEC) instead of get_unused_fd() DROPPED: applied upstream - android/sync: use get_unused_fd_flags(O_CLOEXEC) instead of get_unused_fd() DROPPED: applied upstream - vfio: use get_unused_fd_flags(0) instead of get_unused_fd() DROPPED: applied upstream. Additionally subsystem maintainer applied another patch on top to set the flags to O_CLOEXEC. - industrialio: use anon_inode_getfd() with O_CLOEXEC flag NEW: propose to use O_CLOEXEC as default flag. Links: [ANDROID] http://lkml.kernel.org/r/cacsp8sjxgmk2_kx_+rgzqqqwqkernvf1wt3k5tw991w5dfa...@mail.gmail.com http://lkml.kernel.org/r/CACSP8SjZcpcpEtQHzcGYhf-MP7QGo0XpN7-uN7rmD=vNtopG=w...@mail.gmail.com [IB] http://lkml.kernel.org/r/CAL1RGDXV1_BjSLrQDFdVQ1_D75+bMtOtikHOUp8VBiy_OJUf=w...@mail.gmail.com [VFIO] http://lkml.kernel.org/r/20130822171744.1297.13711.st...@bling.home [KVM] http://lkml.kernel.org/r/5219a8fc.8090...@redhat.com http://lkml.kernel.org/r/3557ef65-4327-4dae-999a-b0ee13c43...@suse.de http://lkml.kernel.org/r/20130826102023.ga8...@redhat.com [SCTP] http://lkml.kernel.org/r/51d312e8.6090...@gmail.com http://lkml.kernel.org/r/20130702.161428.1703028286206350504.da...@davemloft.net [XFS] http://lkml.kernel.org/r/20130709205321.gv20...@sgi.com [PATCHSETv2] http://lkml.kernel.org/r/cover.1376327678.git.ydrone...@opteya.com Yann Droneaud (8): ia64: use get_unused_fd_flags(0) instead of get_unused_fd() ppc/cell: use get_unused_fd_flags(0) instead of get_unused_fd() binfmt_misc: use get_unused_fd_flags(0) instead of get_unused_fd() file: use get_unused_fd_flags(0) instead of get_unused_fd() fanotify: use get_unused_fd_flags(0) instead of get_unused_fd() events: use get_unused_fd_flags(0) instead of get_unused_fd() file: remove get_unused_fd() industrialio: use anon_inode_getfd() with O_CLOEXEC flag arch/ia64/kernel/perfmon.c| 2 +- arch/powerpc/platforms/cell/spufs/inode.c | 4 ++-- drivers/iio/industrialio-event.c | 2 +- fs/binfmt_misc.c | 2 +- fs/file.c | 2 +- fs/notify/fanotify/fanotify_user.c| 2 +- include/linux/file.h | 1 - kernel/events/core.c | 2 +- 8 files changed, 8 insertions(+), 9 deletions(-) -- 1.8.3.1 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH v3 2/8] ppc/cell: use get_unused_fd_flags(0) instead of get_unused_fd()
Macro get_unused_fd() is used to allocate a file descriptor with default flags. Those default flags (0) can be unsafe: O_CLOEXEC must be used by default to not leak file descriptor across exec(). Instead of macro get_unused_fd(), functions anon_inode_getfd() or get_unused_fd_flags() should be used with flags given by userspace. If not possible, flags should be set to O_CLOEXEC to provide userspace with a default safe behavor. In a further patch, get_unused_fd() will be removed so that new code start using anon_inode_getfd() or get_unused_fd_flags() with correct flags. This patch replaces calls to get_unused_fd() with equivalent call to get_unused_fd_flags(0) to preserve current behavor for existing code. The hard coded flag value (0) should be reviewed on a per-subsystem basis, and, if possible, set to O_CLOEXEC. Signed-off-by: Yann Droneaud ydrone...@opteya.com Link: http://lkml.kernel.org/r/cover.1378460926.git.ydrone...@opteya.com --- arch/powerpc/platforms/cell/spufs/inode.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/arch/powerpc/platforms/cell/spufs/inode.c b/arch/powerpc/platforms/cell/spufs/inode.c index 87ba7cf..51effce 100644 --- a/arch/powerpc/platforms/cell/spufs/inode.c +++ b/arch/powerpc/platforms/cell/spufs/inode.c @@ -301,7 +301,7 @@ static int spufs_context_open(struct path *path) int ret; struct file *filp; - ret = get_unused_fd(); + ret = get_unused_fd_flags(0); if (ret 0) return ret; @@ -518,7 +518,7 @@ static int spufs_gang_open(struct path *path) int ret; struct file *filp; - ret = get_unused_fd(); + ret = get_unused_fd_flags(0); if (ret 0) return ret; -- 1.8.3.1 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/2] ppc: kvm: use anon_inode_getfd() with O_CLOEXEC flag
Le 26.08.2013 09:39, Paolo Bonzini a écrit : Il 25/08/2013 17:04, Alexander Graf ha scritto: On 24.08.2013, at 21:14, Yann Droneaud wrote: This patch set O_CLOEXEC flag on all file descriptors created with anon_inode_getfd() to not leak file descriptors across exec(). Signed-off-by: Yann Droneaud ydrone...@opteya.com Link: http://lkml.kernel.org/r/cover.1377372576.git.ydrone...@opteya.com Reviewed-by: Alexander Graf ag...@suse.de Would it make sense to simply inherit the O_CLOEXEC flag from the parent kvm fd instead? That would give user space the power to keep fds across exec() if it wants to. Does it make sense to use non-O_CLOEXEC file descriptors with KVM at all? Besides fork() not being supported by KVM, as described in Documentation/virtual/kvm/api.txt, the VMAs of the parent process go away as soon as you exec(). I'm not sure how you can use the inherited file descriptor in a sensible way after exec(). Sounds a lot like InfiniBand subsystem behavor: IB file descriptors are of no use accross exec() since memory mappings tied to those fds won't be available in the new process: https://lkml.org/lkml/2013/7/8/380 http://mid.gmane.org/f58540dc64fec1ac0e496dfcd3cc1...@meuh.org Regards. -- Yann Droneaud OPTEYA ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH 0/2] kvm: use anon_inode_getfd() with O_CLOEXEC flag
Hi, Following a patchset asking to change calls to get_unused_flag() [1] to use O_CLOEXEC, Alex Williamson [2][3] decided to change VFIO to use the flag. Since it's a related subsystem to KVM, using O_CLOEXEC for file descriptors created by KVM might be applicable too. I'm suggesting to change calls to anon_inode_getfd() to use O_CLOEXEC as default flag. This patchset should be reviewed to not break existing userspace program. BTW, if it's not applicable, I would suggest that new ioctls be added to KVM subsystem, those ioctls would have a flag field added to their arguments. Such flag would let userspace choose the open flag to use. See for example other APIs using anon_inode_getfd() such as fanotify, inotify, signalfd and timerfd. You might be interested to read: - Secure File Descriptor Handling (Ulrich Drepper, 2008) http://udrepper.livejournal.com/20407.html - Excuse me son, but your code is leaking !!! (Dan Walsh, March 2012) http://danwalsh.livejournal.com/53603.html Regards. [1] http://lkml.kernel.org/r/cover.1376327678.git.ydrone...@opteya.com [2] http://lkml.kernel.org/r/1377186804.25163.17.ca...@ul30vt.home [3] http://lkml.kernel.org/r/20130822171744.1297.13711.st...@bling.home Yann Droneaud (2): kvm: use anon_inode_getfd() with O_CLOEXEC flag ppc: kvm: use anon_inode_getfd() with O_CLOEXEC flag arch/powerpc/kvm/book3s_64_mmu_hv.c | 2 +- arch/powerpc/kvm/book3s_64_vio.c| 2 +- arch/powerpc/kvm/book3s_hv.c| 2 +- virt/kvm/kvm_main.c | 6 +++--- 4 files changed, 6 insertions(+), 6 deletions(-) -- 1.8.3.1 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH 2/2] ppc: kvm: use anon_inode_getfd() with O_CLOEXEC flag
KVM uses anon_inode_get() to allocate file descriptors as part
of some of its ioctls. But those ioctls are lacking a flag argument
allowing userspace to choose options for the newly opened file descriptor.
In such case it's advised to use O_CLOEXEC by default so that
userspace is allowed to choose, without race, if the file descriptor
is going to be inherited across exec().
This patch set O_CLOEXEC flag on all file descriptors created
with anon_inode_getfd() to not leak file descriptors across exec().
Signed-off-by: Yann Droneaud ydrone...@opteya.com
Link: http://lkml.kernel.org/r/cover.1377372576.git.ydrone...@opteya.com
---
arch/powerpc/kvm/book3s_64_mmu_hv.c | 2 +-
arch/powerpc/kvm/book3s_64_vio.c| 2 +-
arch/powerpc/kvm/book3s_hv.c| 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c
b/arch/powerpc/kvm/book3s_64_mmu_hv.c
index 710d313..f7c9e8a 100644
--- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
+++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
@@ -1579,7 +1579,7 @@ int kvm_vm_ioctl_get_htab_fd(struct kvm *kvm, struct
kvm_get_htab_fd *ghf)
ctx-first_pass = 1;
rwflag = (ghf-flags KVM_GET_HTAB_WRITE) ? O_WRONLY : O_RDONLY;
- ret = anon_inode_getfd(kvm-htab, kvm_htab_fops, ctx, rwflag);
+ ret = anon_inode_getfd(kvm-htab, kvm_htab_fops, ctx, rwflag |
O_CLOEXEC);
if (ret 0) {
kvm_put_kvm(kvm);
return ret;
diff --git a/arch/powerpc/kvm/book3s_64_vio.c b/arch/powerpc/kvm/book3s_64_vio.c
index b2d3f3b..54cf9bc 100644
--- a/arch/powerpc/kvm/book3s_64_vio.c
+++ b/arch/powerpc/kvm/book3s_64_vio.c
@@ -136,7 +136,7 @@ long kvm_vm_ioctl_create_spapr_tce(struct kvm *kvm,
mutex_unlock(kvm-lock);
return anon_inode_getfd(kvm-spapr-tce, kvm_spapr_tce_fops,
- stt, O_RDWR);
+ stt, O_RDWR | O_CLOEXEC);
fail:
if (stt) {
diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
index e8d51cb..3503829 100644
--- a/arch/powerpc/kvm/book3s_hv.c
+++ b/arch/powerpc/kvm/book3s_hv.c
@@ -1556,7 +1556,7 @@ long kvm_vm_ioctl_allocate_rma(struct kvm *kvm, struct
kvm_allocate_rma *ret)
if (!ri)
return -ENOMEM;
- fd = anon_inode_getfd(kvm-rma, kvm_rma_fops, ri, O_RDWR);
+ fd = anon_inode_getfd(kvm-rma, kvm_rma_fops, ri, O_RDWR | O_CLOEXEC);
if (fd 0)
kvm_release_rma(ri);
--
1.8.3.1
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH v2 00/10] Getting rid of get_unused_fd_flags()
Hi, Macro get_unused_fd() is a shortcut to call function get_unused_fd_flags(), to allocate a file descriptor. The macro use 0 as flags, so the file descriptor is created without O_CLOEXEC flag. This can be seen as an unsafe default eg. in most case O_CLOEXEC must be used to not leak file descriptor across exec(). Newer kernel code should use anon_inode_getfd() or get_unused_fd_flags() with flags provided by userspace. If flags cannot be given by userspace, O_CLOEXEC must be the default flag. Using O_CLOEXEC by default allows userspace to choose, without race, if the file descriptor is going to be inherited across exec(). They are two ways to achieve this: - makes get_unused_fd() use O_CLOEXEC by default It's difficult to get it right: every code using of get_unused_fd() must take this change into account and be fixed as soon as macro get_unused_fd() do the switch. Non updated code will have unexpected behavor and it's likely going to break API contract. - remove get_unused_fd() It's going to break some out of tree, not yet upstream kernel code, but it's easy to notice and fix. Anyway, newer code should use anon_inode_getfd() or get_unused_fd_flags(). The latter option was choosen to ensure no unexpected behavor for out of tree, not yet upstream code. Removing the macro is the safest choice: it's better to break build than trying to make get_unused_fd() use O_CLOEXEC by default and get all user of get_unused_fd() update. Additionnaly, removing the macro is not going to break modules ABI. In linux-next tag 20130815, they're currently: - 19 calls to get_unused_fd_flags() (+4) not counting get_unused_fd() and anon_inode_getfd() - 10 calls to get_unused_fd() (-4) - 11 calls to anon_inode_getfd()(0) The following patchset try to convert all calls to get_unused_fd() to get_unused_fd_flags(0) before removing get_unused_fd() macro. Without get_unused_fd() macro, more subsystems are likely to use anon_inode_getfd() and be teached to provide an API that let userspace choose the opening flags of the file descriptor. Changes from v1 http://lkml.kernel.org/r/cover.1372777600.git.ydrone...@opteya.com: - explicitly added subsystem maintainers as mail recepients. - infiniband: use get_unused_fd_flags(0) instead of get_unused_fd() DROPPED: subsystem maintainer applied another patch using get_unused_fd_flags(O_CLOEXEC) as suggested. - android/sw_sync: use get_unused_fd_flags(0) instead of get_unused_fd() MODIFIED: use get_unused_fd_flags(O_CLOEXEC) as suggested by http://lkml.kernel.org/r/cacsp8sjxgmk2_kx_+rgzqqqwqkernvf1wt3k5tw991w5dfa...@mail.gmail.com - android/sync: use get_unused_fd_flags(0) instead of get_unused_fd() MODIFIED: use get_unused_fd_flags(O_CLOEXEC) as suggested by http://lkml.kernel.org/r/CACSP8SjZcpcpEtQHzcGYhf-MP7QGo0XpN7-uN7rmD=vNtopG=w...@mail.gmail.com - xfs: use get_unused_fd_flags(0) instead of get_unused_fd() DROPPED: applied asis by subsystem maintainer. - sctp: use get_unused_fd_flags(0) instead of get_unused_fd() DROPPED: applied asis by subsystem maintainer. Yann Droneaud (10): ia64: use get_unused_fd_flags(0) instead of get_unused_fd() ppc/cell: use get_unused_fd_flags(0) instead of get_unused_fd() android/sw_sync: use get_unused_fd_flags(O_CLOEXEC) instead of get_unused_fd() android/sync: use get_unused_fd_flags(O_CLOEXEC) instead of get_unused_fd() vfio: use get_unused_fd_flags(0) instead of get_unused_fd() binfmt_misc: use get_unused_fd_flags(0) instead of get_unused_fd() file: use get_unused_fd_flags(0) instead of get_unused_fd() fanotify: use get_unused_fd_flags(0) instead of get_unused_fd() events: use get_unused_fd_flags(0) instead of get_unused_fd() file: remove get_unused_fd() arch/ia64/kernel/perfmon.c| 2 +- arch/powerpc/platforms/cell/spufs/inode.c | 4 ++-- drivers/staging/android/sw_sync.c | 2 +- drivers/staging/android/sync.c| 2 +- drivers/vfio/vfio.c | 2 +- fs/binfmt_misc.c | 2 +- fs/file.c | 2 +- fs/notify/fanotify/fanotify_user.c| 2 +- include/linux/file.h | 1 - kernel/events/core.c | 2 +- 10 files changed, 10 insertions(+), 11 deletions(-) -- 1.8.3.1 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH v2 02/10] ppc/cell: use get_unused_fd_flags(0) instead of get_unused_fd()
Macro get_unused_fd() is used to allocate a file descriptor with default flags. Those default flags (0) can be unsafe: O_CLOEXEC must be used by default to not leak file descriptor across exec(). Instead of macro get_unused_fd(), functions anon_inode_getfd() or get_unused_fd_flags() should be used with flags given by userspace. If not possible, flags should be set to O_CLOEXEC to provide userspace with a default safe behavor. In a further patch, get_unused_fd() will be removed so that new code start using anon_inode_getfd() or get_unused_fd_flags() with correct flags. This patch replaces calls to get_unused_fd() with equivalent call to get_unused_fd_flags(0) to preserve current behavor for existing code. The hard coded flag value (0) should be reviewed on a per-subsystem basis, and, if possible, set to O_CLOEXEC. Signed-off-by: Yann Droneaud ydrone...@opteya.com Link: http://lkml.kernel.org/r/cover.1376327678.git.ydrone...@opteya.com --- arch/powerpc/platforms/cell/spufs/inode.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/arch/powerpc/platforms/cell/spufs/inode.c b/arch/powerpc/platforms/cell/spufs/inode.c index f390042..88df441 100644 --- a/arch/powerpc/platforms/cell/spufs/inode.c +++ b/arch/powerpc/platforms/cell/spufs/inode.c @@ -301,7 +301,7 @@ static int spufs_context_open(struct path *path) int ret; struct file *filp; - ret = get_unused_fd(); + ret = get_unused_fd_flags(0); if (ret 0) return ret; @@ -518,7 +518,7 @@ static int spufs_gang_open(struct path *path) int ret; struct file *filp; - ret = get_unused_fd(); + ret = get_unused_fd_flags(0); if (ret 0) return ret; -- 1.8.3.1 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH 02/13] ppc/cell: use get_unused_fd_flags(0) instead of get_unused_fd()
Macro get_unused_fd() is used to allocate a file descriptor with default flags. Those default flags (0) can be unsafe: O_CLOEXEC must be used by default to not leak file descriptor across exec(). Instead of macro get_unused_fd(), functions anon_inode_getfd() or get_unused_fd_flags() should be used with flags given by userspace. If not possible, flags should be set to O_CLOEXEC to provide userspace with a default safe behavor. In a further patch, get_unused_fd() will be removed so that new code start using anon_inode_getfd() or get_unused_fd_flags() with correct flags. This patch replaces calls to get_unused_fd() with equivalent call to get_unused_fd_flags(0) to preserve current behavor for existing code. The hard coded flag value (0) should be reviewed on a per-subsystem basis, and, if possible, set to O_CLOEXEC. Signed-off-by: Yann Droneaud ydrone...@opteya.com --- arch/powerpc/platforms/cell/spufs/inode.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/arch/powerpc/platforms/cell/spufs/inode.c b/arch/powerpc/platforms/cell/spufs/inode.c index f390042..88df441 100644 --- a/arch/powerpc/platforms/cell/spufs/inode.c +++ b/arch/powerpc/platforms/cell/spufs/inode.c @@ -301,7 +301,7 @@ static int spufs_context_open(struct path *path) int ret; struct file *filp; - ret = get_unused_fd(); + ret = get_unused_fd_flags(0); if (ret 0) return ret; @@ -518,7 +518,7 @@ static int spufs_gang_open(struct path *path) int ret; struct file *filp; - ret = get_unused_fd(); + ret = get_unused_fd_flags(0); if (ret 0) return ret; -- 1.8.3.1 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH 00/13] Getting rid of get_unused_fd()
Hi, Macro get_unused_fd() is a shortcut to call function get_unused_fd_flags(), to allocate a file descriptor. The macro use 0 as flags, so the file descriptor is created without O_CLOEXEC flag. This can be seen as an unsafe default eg. in most case O_CLOEXEC must be used to not leak file descriptor across exec(). Newer kernel code should use anon_inode_getfd() or get_unused_fd_flags() with flags provided by userspace. If flags cannot be given by userspace, O_CLOEXEC must be the default flag. Using O_CLOEXEC by default allows userspace to choose, without race, if the file descriptor is going to be inherited across exec(). They are two ways to achieve this: - makes get_unused_fd() use O_CLOEXEC by default It's difficult to get it right: every code using of get_unused_fd() must take this change into account and be fixed as soon as macro get_unused_fd() do the switch. Non updated code will have unexpected behavor and it's likely going to break API contract. - remove get_unused_fd() It's going to break some out of tree, not yet upstream kernel code, but it's easy to notice and fix. Anyway, newer code should use anon_inode_getfd() or get_unused_fd_flags(). The latter option was choosen to ensure no unexpected behavor for out of tree, not yet upstream code. Removing the macro is the safest choice: it's better to break build than trying to make get_unused_fd() use O_CLOEXEC by default and get all user of get_unused_fd() update. Additionnaly, removing the macro is not going to break modules ABI. In linux-next tag 20130702, they're currently: - 15 calls to get_unused_fd_flags() not counting get_unused_fd() and anon_inode_getfd() - 14 calls to get_unused_fd() - 11 calls to anon_inode_getfd() The following patchset try to convert all calls to get_unused_fd() to get_unused_fd_flags(0) before removing get_unused_fd() macro. Without get_unused_fd() macro, more subsystems are likely to use anon_inode_getfd() and be teached to provide an API that let userspace choose the opening flags of the file descriptor. Yann Droneaud (13): ia64: use get_unused_fd_flags(0) instead of get_unused_fd() ppc/cell: use get_unused_fd_flags(0) instead of get_unused_fd() infiniband: use get_unused_fd_flags(0) instead of get_unused_fd() android/sw_sync: use get_unused_fd_flags(0) instead of get_unused_fd() android/sync: use get_unused_fd_flags(0) instead of get_unused_fd() vfio: use get_unused_fd_flags(0) instead of get_unused_fd() binfmt_misc: use get_unused_fd_flags(0) instead of get_unused_fd() file: use get_unused_fd_flags(0) instead of get_unused_fd() fanotify: use get_unused_fd_flags(0) instead of get_unused_fd() xfs: use get_unused_fd_flags(0) instead of get_unused_fd() events: use get_unused_fd_flags(0) instead of get_unused_fd() sctp: use get_unused_fd_flags(0) instead of get_unused_fd() file: remove get_unused_fd() arch/ia64/kernel/perfmon.c| 2 +- arch/powerpc/platforms/cell/spufs/inode.c | 4 ++-- drivers/infiniband/core/uverbs_cmd.c | 4 ++-- drivers/staging/android/sw_sync.c | 2 +- drivers/staging/android/sync.c| 2 +- drivers/vfio/vfio.c | 2 +- fs/binfmt_misc.c | 2 +- fs/file.c | 2 +- fs/notify/fanotify/fanotify_user.c| 2 +- fs/xfs/xfs_ioctl.c| 2 +- include/linux/file.h | 1 - kernel/events/core.c | 2 +- net/sctp/socket.c | 2 +- 13 files changed, 14 insertions(+), 15 deletions(-) -- 1.8.3.1 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
