* [PATCH 00/13] Getting rid of get_unused_fd()
@ 2013-07-02 16:39 Yann Droneaud
2013-07-02 16:39 ` [PATCH 02/13] ppc/cell: use get_unused_fd_flags(0) instead " Yann Droneaud
0 siblings, 1 reply; 5+ messages in thread
From: Yann Droneaud @ 2013-07-02 16:39 UTC (permalink / raw)
To: linux-kernel, linux-ia64, linuxppc-dev, cbe-oss-dev, linux-rdma,
devel, kvm, linux-fsdevel, xfs, linux-sctp, netdev, Eric Paris,
Peter Zijlstra, Paul Mackerras, Ingo Molnar,
Arnaldo Carvalho de Melo
Cc: Yann Droneaud
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 02/13] ppc/cell: use get_unused_fd_flags(0) instead of get_unused_fd()
2013-07-02 16:39 [PATCH 00/13] Getting rid of get_unused_fd() Yann Droneaud
@ 2013-07-02 16:39 ` Yann Droneaud
2014-01-12 23:06 ` Benjamin Herrenschmidt
0 siblings, 1 reply; 5+ messages in thread
From: Yann Droneaud @ 2013-07-02 16:39 UTC (permalink / raw)
To: linux-kernel, linuxppc-dev, cbe-oss-dev; +Cc: Yann Droneaud
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 <ydroneaud@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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 02/13] ppc/cell: use get_unused_fd_flags(0) instead of get_unused_fd()
2013-07-02 16:39 ` [PATCH 02/13] ppc/cell: use get_unused_fd_flags(0) instead " Yann Droneaud
@ 2014-01-12 23:06 ` Benjamin Herrenschmidt
2014-01-13 9:30 ` Yann Droneaud
0 siblings, 1 reply; 5+ messages in thread
From: Benjamin Herrenschmidt @ 2014-01-12 23:06 UTC (permalink / raw)
To: Yann Droneaud; +Cc: cbe-oss-dev, linuxppc-dev, linux-kernel
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 <ydroneaud@opteya.com>
Should I merge this (v5 on patchwork) or let Al do it ?
Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
> 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;
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 02/13] ppc/cell: use get_unused_fd_flags(0) instead of get_unused_fd()
2014-01-12 23:06 ` Benjamin Herrenschmidt
@ 2014-01-13 9:30 ` Yann Droneaud
2014-01-20 17:01 ` Yann Droneaud
0 siblings, 1 reply; 5+ messages in thread
From: Yann Droneaud @ 2014-01-13 9:30 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: cbe-oss-dev, linuxppc-dev, linux-kernel
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 <ydroneaud@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 <benh@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.ydroneaud@opteya.com
v5 patchset is at
http://lkml.kernel.org/r/cover.1388952061.git.ydroneaud@opteya.com
Regards.
--
Yann Droneaud
OPTEYA
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 02/13] ppc/cell: use get_unused_fd_flags(0) instead of get_unused_fd()
2014-01-13 9:30 ` Yann Droneaud
@ 2014-01-20 17:01 ` Yann Droneaud
0 siblings, 0 replies; 5+ messages in thread
From: Yann Droneaud @ 2014-01-20 17:01 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: cbe-oss-dev, Yann Droneaud, linuxppc-dev, linux-kernel
Hi,
Le lundi 13 janvier 2014 à 10:30 +0100, Yann Droneaud a écrit :
> 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 <ydroneaud@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 <benh@kernel.crashing.org>
> >
>
> Thanks a lot.
> Note:
> latest patch (from v5 patchset) is at
> http://lkml.kernel.org/r/fe27abcfab5563d36a3e5e58ff36e5500c39be6a.1388952061.git.ydroneaud@opteya.com
> v5 patchset is at
> http://lkml.kernel.org/r/cover.1388952061.git.ydroneaud@opteya.com
>
I have not yet seen the patch in your trees at
https://git.kernel.org/cgit/linux/kernel/git/benh/powerpc.git/
I hope you would pick the patch, if possible in its latest version
(unfortunately I'm not able to give the link to the astest patch in
patchwork).
Regards.
--
Yann Droneaud
OPTEYA
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-01-20 17:01 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-07-02 16:39 [PATCH 00/13] Getting rid of get_unused_fd() Yann Droneaud
2013-07-02 16:39 ` [PATCH 02/13] ppc/cell: use get_unused_fd_flags(0) instead " Yann Droneaud
2014-01-12 23:06 ` Benjamin Herrenschmidt
2014-01-13 9:30 ` Yann Droneaud
2014-01-20 17:01 ` Yann Droneaud
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).