[PATCH 00/13] Getting rid of get_unused_fd()

[PATCH 00/13] Getting rid of get_unused_fd()

[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

[PATCH 00/13] Getting rid of get_unused_fd()

[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

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 00/13] Getting rid of get_unused_fd()

[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

[PATCH 01/13] ia64: use get_unused_fd_flags(0) instead of get_unused_fd()

[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/ia64/kernel/perfmon.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/ia64/kernel/perfmon.c b/arch/ia64/kernel/perfmon.c
index 5a9ff1c..64757c1 100644
--- a/arch/ia64/kernel/perfmon.c
+++ b/arch/ia64/kernel/perfmon.c
@@ -2668,7 +2668,7 @@ pfm_context_create(pfm_context_t *ctx, void *arg, int count, struct pt_regs *reg
 
 	ret = -ENOMEM;
 
-	fd = get_unused_fd();
+	fd = get_unused_fd_flags(0);
 	if (fd < 0)
 		return fd;
 
-- 
1.8.3.1

[PATCH 01/13] ia64: use get_unused_fd_flags(0) instead of get_unused_fd()

[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/ia64/kernel/perfmon.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/ia64/kernel/perfmon.c b/arch/ia64/kernel/perfmon.c
index 5a9ff1c..64757c1 100644
--- a/arch/ia64/kernel/perfmon.c
+++ b/arch/ia64/kernel/perfmon.c
@@ -2668,7 +2668,7 @@ pfm_context_create(pfm_context_t *ctx, void *arg, int count, struct pt_regs *reg
 
 	ret = -ENOMEM;
 
-	fd = get_unused_fd();
+	fd = get_unused_fd_flags(0);
 	if (fd < 0)
 		return fd;
 
-- 
1.8.3.1

[PATCH 02/13] ppc/cell: use get_unused_fd_flags(0) instead of get_unused_fd()

[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

Re: [PATCH 02/13] ppc/cell: use get_unused_fd_flags(0) instead of get_unused_fd()

[Benjamin Herrenschmidt]

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;
>  

Re: [PATCH 02/13] ppc/cell: use get_unused_fd_flags(0) instead of get_unused_fd()

[Benjamin Herrenschmidt]

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;
>  

Re: [PATCH 02/13] ppc/cell: use get_unused_fd_flags(0) instead of get_unused_fd()

[Yann Droneaud]

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

Re: [PATCH 02/13] ppc/cell: use get_unused_fd_flags(0) instead of get_unused_fd()

[Yann Droneaud]

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

Re: [PATCH 02/13] ppc/cell: use get_unused_fd_flags(0) instead of get_unused_fd()

[Yann Droneaud]

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

Re: [PATCH 02/13] ppc/cell: use get_unused_fd_flags(0) instead of get_unused_fd()

[Yann Droneaud]

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

[PATCH 03/13] infiniband: use get_unused_fd_flags(0) instead of get_unused_fd()

[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>
---
 drivers/infiniband/core/uverbs_cmd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
index a7d00f6..9c893bc 100644
--- a/drivers/infiniband/core/uverbs_cmd.c
+++ b/drivers/infiniband/core/uverbs_cmd.c
@@ -334,7 +334,7 @@ ssize_t ib_uverbs_get_context(struct ib_uverbs_file *file,
 
 	resp.num_comp_vectors = file->device->num_comp_vectors;
 
-	ret = get_unused_fd();
+	ret = get_unused_fd_flags(0);
 	if (ret < 0)
 		goto err_free;
 	resp.async_fd = ret;
@@ -1184,7 +1184,7 @@ ssize_t ib_uverbs_create_comp_channel(struct ib_uverbs_file *file,
 	if (copy_from_user(&cmd, buf, sizeof cmd))
 		return -EFAULT;
 
-	ret = get_unused_fd();
+	ret = get_unused_fd_flags(0);
 	if (ret < 0)
 		return ret;
 	resp.fd = ret;
-- 
1.8.3.1

Re: [PATCH 03/13] infiniband: use get_unused_fd_flags(0) instead of get_unused_fd()

[Roland Dreier]

Thanks, I just applied a patch to convert to
get_unused_fd_flags(O_CLOEXEC) in uverbs, since there isn't anything
useful that can be done with uverbs fds across an exec.

 - R.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 03/13] infiniband: use get_unused_fd_flags(0) instead of get_unused_fd()

[Roland Dreier]

Thanks, I just applied a patch to convert to
get_unused_fd_flags(O_CLOEXEC) in uverbs, since there isn't anything
useful that can be done with uverbs fds across an exec.

 - R.

Re: [PATCH 03/13] infiniband: use get_unused_fd_flags(0) instead of get_unused_fd()

[Yann Droneaud]

Le 08.07.2013 20:23, Roland Dreier a écrit :
> Thanks, I just applied a patch to convert to
> get_unused_fd_flags(O_CLOEXEC) in uverbs, since there isn't anything
> useful that can be done with uverbs fds across an exec.
> 

Thanks.

In fact, InfiniBand was my main target and I kept this change (setting 
O_CLOEXEC)
for another batch.

It's following my patch on libibverbs "[PATCH] open files with close on 
exec flag"
http://thread.gmane.org/gmane.linux.drivers.rdma/15727
http://marc.info/?l=linux-rdma&m=136908991102575&w=2

This patch was already quoting Dan Walsh, "Excuse me son, but your code 
is leaking !!!"
http://danwalsh.livejournal.com/53603.html but I couldn't resist to post 
it again.

BTW, I was working on the rationnal/commit message for setting flags to 
O_CLOEXEC
on kernel side, please find the draft if revelant.

--------------------8<----------------------

InfiniBand verbs/RDMA: use O_CLOEXEC

This subsystem is allocating new file descriptor through the InfiniBand 
verbs / RDMA API.

Thoses file descriptors are created after a write() from userspace to a 
special device file.
No read operation is needed to get the file descriptor: it is returned 
to userspace
in a buffer whose address was stored as part of the buffer passed to 
write().
If the write() succeed, the response buffer is updated and the new file 
descriptor is available.

But such file descriptors are mostly hidden to application developpers
by libibverbs / librdma_cm libraries API.
In fact, application developpers could use InfiniBand verbs / RDMA 
without
using directly the file descriptors.

Here's how are created the two file descriptors (using mlx4 as example):

- ibv_context.async_fd:

   ---- kernel ----

   ib_uverbs_get_context() : linux/drivers/infiniband/core/uverbs_cmd.c
   uverbs_cmd_table[IB_USER_VERBS_CMD_GET_CONTEXT]() : 
linux/drivers/infiniband/core/uverbs_main.c
   ib_uverbs_write() : linux/drivers/infiniband/core/uverbs_main.c
   uverbs_mmap_fops.write : linux/drivers/infiniband/core/uverbs_main.c

   ---- userspace ----

   ibv_cmd_get_context() : libibverbs/src/cmd.c
   mlx4_alloc_context() : libmlx4/src/mlx4.c
   mlx4_dev_ops.alloc_context : libmlx4/src/mlx4.c
   __ibv_open_device() : libibverbs/src/device.c

- ibv_comp_channel.fd:

   ---- kernel ----

   ib_uverbs_create_comp_channel() : 
linux/drivers/infiniband/core/uverbs_cmd.c
   uverbs_cmd_table[IB_USER_VERBS_CMD_CREATE_COMP_CHANNEL]() : 
linux/drivers/infiniband/core/uverbs_main.c
   ib_uverbs_write() : linux/drivers/infiniband/core/uverbs_main.c
   uverbs_mmap_fops.write : linux/drivers/infiniband/core/uverbs_main.c

   ---- userspace ----

   ibv_create_comp_channel() : libibverbs/src/verbs.c

But those file descriptors are of no use for another program executed 
through exec():

- without the memory mappings for special memory pages,
   the file descriptor are of no use ...

- the userland libraries/drivers are not ready to
   found the devices already opened.

[ In fact, supporting fork() is already a challenge for thoses API. ]

So those file descriptors can safely be opened with O_CLOEXEC without
disturbing users of InfiniBand verbs /RDMA


--------------------8<----------------------


Regards.

-- 
Yann Droneaud
OPTEYA

[PATCH 04/13] android/sw_sync: use get_unused_fd_flags(0) instead of get_unused_fd()

[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>
---
 drivers/staging/android/sw_sync.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/android/sw_sync.c b/drivers/staging/android/sw_sync.c
index 765c757..a4ed0b9 100644
--- a/drivers/staging/android/sw_sync.c
+++ b/drivers/staging/android/sw_sync.c
@@ -163,7 +163,7 @@ static int sw_sync_release(struct inode *inode, struct file *file)
 static long sw_sync_ioctl_create_fence(struct sw_sync_timeline *obj,
 				       unsigned long arg)
 {
-	int fd = get_unused_fd();
+	int fd = get_unused_fd_flags(0);
 	int err;
 	struct sync_pt *pt;
 	struct sync_fence *fence;
-- 
1.8.3.1

Re: [PATCH 04/13] android/sw_sync: use get_unused_fd_flags(0) instead of get_unused_fd()

[Erik Gilling]

On Tue, Jul 2, 2013 at 9:39 AM, Yann Droneaud <ydroneaud@opteya.com> wrote:
>  static long sw_sync_ioctl_create_fence(struct sw_sync_timeline *obj,
>                                        unsigned long arg)
>  {
> -       int fd = get_unused_fd();
> +       int fd = get_unused_fd_flags(0);

I think we should use O_CLOEXEC here.  Colin and I can't think of any
case where one would want keep fences across exec.

Cheers,
   Erik

[PATCH 05/13] android/sync: use get_unused_fd_flags(0) instead of get_unused_fd()

[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>
---
 drivers/staging/android/sync.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/android/sync.c b/drivers/staging/android/sync.c
index 2996077..c40c743 100644
--- a/drivers/staging/android/sync.c
+++ b/drivers/staging/android/sync.c
@@ -697,7 +697,7 @@ static long sync_fence_ioctl_wait(struct sync_fence *fence, unsigned long arg)
 
 static long sync_fence_ioctl_merge(struct sync_fence *fence, unsigned long arg)
 {
-	int fd = get_unused_fd();
+	int fd = get_unused_fd_flags(0);
 	int err;
 	struct sync_fence *fence2, *fence3;
 	struct sync_merge_data data;
-- 
1.8.3.1

Re: [PATCH 05/13] android/sync: use get_unused_fd_flags(0) instead of get_unused_fd()

[Erik Gilling]

On Tue, Jul 2, 2013 at 9:39 AM, Yann Droneaud <ydroneaud@opteya.com> wrote:
>  static long sync_fence_ioctl_merge(struct sync_fence *fence, unsigned long arg)
>  {
> -       int fd = get_unused_fd();
> +       int fd = get_unused_fd_flags(0);

I think we should use O_CLOEXEC here.  Colin and I can't think of any
case where one would want keep fences across exec.

Cheers,
   Erik

[PATCH 06/13] vfio: use get_unused_fd_flags(0) instead of get_unused_fd()

[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>
---
 drivers/vfio/vfio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index c488da5..bb4e9fd 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -1126,7 +1126,7 @@ static int vfio_group_get_device_fd(struct vfio_group *group, char *buf)
 		 * We can't use anon_inode_getfd() because we need to modify
 		 * the f_mode flags directly to allow more than just ioctls
 		 */
-		ret = get_unused_fd();
+		ret = get_unused_fd_flags(0);
 		if (ret < 0) {
 			device->ops->release(device->device_data);
 			break;
-- 
1.8.3.1

[PATCH 07/13] binfmt_misc: use get_unused_fd_flags(0) instead of get_unused_fd()

[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>
---
 fs/binfmt_misc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c
index 1c740e1..052f6dc 100644
--- a/fs/binfmt_misc.c
+++ b/fs/binfmt_misc.c
@@ -138,7 +138,7 @@ static int load_misc_binary(struct linux_binprm *bprm)
 		/* if the binary should be opened on behalf of the
 		 * interpreter than keep it open and assign descriptor
 		 * to it */
- 		fd_binary = get_unused_fd();
+		fd_binary = get_unused_fd_flags(0);
  		if (fd_binary < 0) {
  			retval = fd_binary;
  			goto _ret;
-- 
1.8.3.1

[PATCH 08/13] file: use get_unused_fd_flags(0) instead of get_unused_fd()

[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>
---
 fs/file.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/file.c b/fs/file.c
index 4a78f98..1420d28 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -897,7 +897,7 @@ SYSCALL_DEFINE1(dup, unsigned int, fildes)
 	struct file *file = fget_raw(fildes);
 
 	if (file) {
-		ret = get_unused_fd();
+		ret = get_unused_fd_flags(0);
 		if (ret >= 0)
 			fd_install(ret, file);
 		else
-- 
1.8.3.1

[PATCH 09/13] fanotify: use get_unused_fd_flags(0) instead of get_unused_fd()

[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>
---
 fs/notify/fanotify/fanotify_user.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index e44cb64..644b9a7 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -69,7 +69,7 @@ static int create_fd(struct fsnotify_group *group,
 
 	pr_debug("%s: group=%p event=%p\n", __func__, group, event);
 
-	client_fd = get_unused_fd();
+	client_fd = get_unused_fd_flags(0);
 	if (client_fd < 0)
 		return client_fd;
 
-- 
1.8.3.1

[PATCH 10/13] xfs: use get_unused_fd_flags(0) instead of get_unused_fd()

[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>
---
 fs/xfs/xfs_ioctl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 5e99968..dc5b659 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -248,7 +248,7 @@ xfs_open_by_handle(
 		goto out_dput;
 	}
 
-	fd = get_unused_fd();
+	fd = get_unused_fd_flags(0);
 	if (fd < 0) {
 		error = fd;
 		goto out_dput;
-- 
1.8.3.1

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 10/13] xfs: use get_unused_fd_flags(0) instead of get_unused_fd()

[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>
---
 fs/xfs/xfs_ioctl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 5e99968..dc5b659 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -248,7 +248,7 @@ xfs_open_by_handle(
 		goto out_dput;
 	}
 
-	fd = get_unused_fd();
+	fd = get_unused_fd_flags(0);
 	if (fd < 0) {
 		error = fd;
 		goto out_dput;
-- 
1.8.3.1

Re: [PATCH 10/13] xfs: use get_unused_fd_flags(0) instead of get_unused_fd()

[Ben Myers]

On Tue, Jul 02, 2013 at 06:39:34PM +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>
>
> ---
>  fs/xfs/xfs_ioctl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 5e99968..dc5b659 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -248,7 +248,7 @@ xfs_open_by_handle(
>  		goto out_dput;
>  	}
>  
> -	fd = get_unused_fd();
> +	fd = get_unused_fd_flags(0);

O_CLOEXEC should be fine in this case.

Reviewed-by: Ben Myers <bpm@sgi.com>

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 10/13] xfs: use get_unused_fd_flags(0) instead of get_unused_fd()

[Ben Myers]

On Tue, Jul 02, 2013 at 06:39:34PM +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>
>
> ---
>  fs/xfs/xfs_ioctl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 5e99968..dc5b659 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -248,7 +248,7 @@ xfs_open_by_handle(
>  		goto out_dput;
>  	}
>  
> -	fd = get_unused_fd();
> +	fd = get_unused_fd_flags(0);

O_CLOEXEC should be fine in this case.

Reviewed-by: Ben Myers <bpm@sgi.com>

Re: [PATCH 10/13] xfs: use get_unused_fd_flags(0) instead of get_unused_fd()

[Ben Myers]

Yann,

On Mon, Jul 08, 2013 at 05:41:33PM -0500, Ben Myers wrote:
> On Tue, Jul 02, 2013 at 06:39:34PM +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>
> >
> > ---
> >  fs/xfs/xfs_ioctl.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> > index 5e99968..dc5b659 100644
> > --- a/fs/xfs/xfs_ioctl.c
> > +++ b/fs/xfs/xfs_ioctl.c
> > @@ -248,7 +248,7 @@ xfs_open_by_handle(
> >  		goto out_dput;
> >  	}
> >  
> > -	fd = get_unused_fd();
> > +	fd = get_unused_fd_flags(0);
> 
> O_CLOEXEC should be fine in this case.
> 
> Reviewed-by: Ben Myers <bpm@sgi.com>

Applied at git://oss.sgi.com/xfs/xfs.git.  Looks like I was wrong about
O_CLOEXEC being ok here.  There may be applications which open_by_handle then
fork/exec and expect to still be able to use that file descriptor.

Thanks,
Ben

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 10/13] xfs: use get_unused_fd_flags(0) instead of get_unused_fd()

[Ben Myers]

Yann,

On Mon, Jul 08, 2013 at 05:41:33PM -0500, Ben Myers wrote:
> On Tue, Jul 02, 2013 at 06:39:34PM +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>
> >
> > ---
> >  fs/xfs/xfs_ioctl.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> > index 5e99968..dc5b659 100644
> > --- a/fs/xfs/xfs_ioctl.c
> > +++ b/fs/xfs/xfs_ioctl.c
> > @@ -248,7 +248,7 @@ xfs_open_by_handle(
> >  		goto out_dput;
> >  	}
> >  
> > -	fd = get_unused_fd();
> > +	fd = get_unused_fd_flags(0);
> 
> O_CLOEXEC should be fine in this case.
> 
> Reviewed-by: Ben Myers <bpm@sgi.com>

Applied at git://oss.sgi.com/xfs/xfs.git.  Looks like I was wrong about
O_CLOEXEC being ok here.  There may be applications which open_by_handle then
fork/exec and expect to still be able to use that file descriptor.

Thanks,
Ben

Re: [PATCH 10/13] xfs: use get_unused_fd_flags(0) instead of get_unused_fd()

[Yann Droneaud]

Hi,

Le 09.07.2013 22:53, Ben Myers a écrit :
> On Mon, Jul 08, 2013 at 05:41:33PM -0500, Ben Myers wrote:
>> On Tue, Jul 02, 2013 at 06:39:34PM +0200, Yann Droneaud wrote:

>> > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
>> > index 5e99968..dc5b659 100644
>> > --- a/fs/xfs/xfs_ioctl.c
>> > +++ b/fs/xfs/xfs_ioctl.c
>> > @@ -248,7 +248,7 @@ xfs_open_by_handle(
>> >  		goto out_dput;
>> >  	}
>> >
>> > -	fd = get_unused_fd();
>> > +	fd = get_unused_fd_flags(0);
>> 
>> O_CLOEXEC should be fine in this case.
>> 
>> Reviewed-by: Ben Myers <bpm@sgi.com>
> 
> Applied at git://oss.sgi.com/xfs/xfs.git.  Looks like I was wrong about
> O_CLOEXEC being ok here.  There may be applications which 
> open_by_handle then
> fork/exec and expect to still be able to use that file descriptor.
> 

OK, it's very important to not cause regression here.

For the record, xfs_open_by_handle() is not related to 
open_by_handle_at() syscall.

It's an ioctl (XFS_IOC_OPEN_BY_HANDLE) which is used by xfsprogs's 
libhandle
in functions open_by_fshandle() and open_by_handle().

http://sources.debian.net/src/xfsprogs/3.1.9/libhandle/handle.c?hl=284#L284
http://sources.debian.net/src/xfsprogs/3.1.9/libhandle/handle.c?hl=308#L308

According to codesearch.debian.org, libhandle's open_by_handle() is only 
used
by xfsdump

http://sources.debian.net/src/xfsdump/3.1.1/restore/tree.c?hl=2534#L2534

So there's no many *known* users of this features ... but it's more 
important
not to break *unknown* users of it.

BTW, thanks for the review and applying.

Regards.

-- 
Yann Droneaud
OPTEYA

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 10/13] xfs: use get_unused_fd_flags(0) instead of get_unused_fd()

[Yann Droneaud]

Hi,

Le 09.07.2013 22:53, Ben Myers a écrit :
> On Mon, Jul 08, 2013 at 05:41:33PM -0500, Ben Myers wrote:
>> On Tue, Jul 02, 2013 at 06:39:34PM +0200, Yann Droneaud wrote:

>> > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
>> > index 5e99968..dc5b659 100644
>> > --- a/fs/xfs/xfs_ioctl.c
>> > +++ b/fs/xfs/xfs_ioctl.c
>> > @@ -248,7 +248,7 @@ xfs_open_by_handle(
>> >  		goto out_dput;
>> >  	}
>> >
>> > -	fd = get_unused_fd();
>> > +	fd = get_unused_fd_flags(0);
>> 
>> O_CLOEXEC should be fine in this case.
>> 
>> Reviewed-by: Ben Myers <bpm@sgi.com>
> 
> Applied at git://oss.sgi.com/xfs/xfs.git.  Looks like I was wrong about
> O_CLOEXEC being ok here.  There may be applications which 
> open_by_handle then
> fork/exec and expect to still be able to use that file descriptor.
> 

OK, it's very important to not cause regression here.

For the record, xfs_open_by_handle() is not related to 
open_by_handle_at() syscall.

It's an ioctl (XFS_IOC_OPEN_BY_HANDLE) which is used by xfsprogs's 
libhandle
in functions open_by_fshandle() and open_by_handle().

http://sources.debian.net/src/xfsprogs/3.1.9/libhandle/handle.c?hl=284#L284
http://sources.debian.net/src/xfsprogs/3.1.9/libhandle/handle.c?hl=308#L308

According to codesearch.debian.org, libhandle's open_by_handle() is only 
used
by xfsdump

http://sources.debian.net/src/xfsdump/3.1.1/restore/tree.c?hl=2534#L2534

So there's no many *known* users of this features ... but it's more 
important
not to break *unknown* users of it.

BTW, thanks for the review and applying.

Regards.

-- 
Yann Droneaud
OPTEYA

Re: [PATCH 10/13] xfs: use get_unused_fd_flags(0) instead of get_unused_fd()

[Dave Chinner]

On Wed, Jul 10, 2013 at 12:00:57PM +0200, Yann Droneaud wrote:
> Hi,
> 
> Le 09.07.2013 22:53, Ben Myers a écrit :
> >On Mon, Jul 08, 2013 at 05:41:33PM -0500, Ben Myers wrote:
> >>On Tue, Jul 02, 2013 at 06:39:34PM +0200, Yann Droneaud wrote:
> 
> >>> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> >>> index 5e99968..dc5b659 100644
> >>> --- a/fs/xfs/xfs_ioctl.c
> >>> +++ b/fs/xfs/xfs_ioctl.c
> >>> @@ -248,7 +248,7 @@ xfs_open_by_handle(
> >>>  		goto out_dput;
> >>>  	}
> >>>
> >>> -	fd = get_unused_fd();
> >>> +	fd = get_unused_fd_flags(0);
> >>
> >>O_CLOEXEC should be fine in this case.
> >>
> >>Reviewed-by: Ben Myers <bpm@sgi.com>
> >
> >Applied at git://oss.sgi.com/xfs/xfs.git.  Looks like I was wrong about
> >O_CLOEXEC being ok here.  There may be applications which
> >open_by_handle then
> >fork/exec and expect to still be able to use that file descriptor.
> >
> 
> OK, it's very important to not cause regression here.
> 
> For the record, xfs_open_by_handle() is not related to
> open_by_handle_at() syscall.
> 
> It's an ioctl (XFS_IOC_OPEN_BY_HANDLE) which is used by xfsprogs's
> libhandle
> in functions open_by_fshandle() and open_by_handle().
> 
> http://sources.debian.net/src/xfsprogs/3.1.9/libhandle/handle.c?hl=284#L284
> http://sources.debian.net/src/xfsprogs/3.1.9/libhandle/handle.c?hl=308#L308
> 
> According to codesearch.debian.org, libhandle's open_by_handle() is
> only used
> by xfsdump
> 
> http://sources.debian.net/src/xfsdump/3.1.1/restore/tree.c?hl=2534#L2534
> 
> So there's no many *known* users of this features ... but it's more
> important
> not to break *unknown* users of it.

There are commercial products (i.e. proprietary, closed source) that
use it. SGI has one (DMF) and there are a couple of other backup
programs that have used it in the past, too.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 10/13] xfs: use get_unused_fd_flags(0) instead of get_unused_fd()

[Dave Chinner]

On Wed, Jul 10, 2013 at 12:00:57PM +0200, Yann Droneaud wrote:
> Hi,
> 
> Le 09.07.2013 22:53, Ben Myers a écrit :
> >On Mon, Jul 08, 2013 at 05:41:33PM -0500, Ben Myers wrote:
> >>On Tue, Jul 02, 2013 at 06:39:34PM +0200, Yann Droneaud wrote:
> 
> >>> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> >>> index 5e99968..dc5b659 100644
> >>> --- a/fs/xfs/xfs_ioctl.c
> >>> +++ b/fs/xfs/xfs_ioctl.c
> >>> @@ -248,7 +248,7 @@ xfs_open_by_handle(
> >>>  		goto out_dput;
> >>>  	}
> >>>
> >>> -	fd = get_unused_fd();
> >>> +	fd = get_unused_fd_flags(0);
> >>
> >>O_CLOEXEC should be fine in this case.
> >>
> >>Reviewed-by: Ben Myers <bpm@sgi.com>
> >
> >Applied at git://oss.sgi.com/xfs/xfs.git.  Looks like I was wrong about
> >O_CLOEXEC being ok here.  There may be applications which
> >open_by_handle then
> >fork/exec and expect to still be able to use that file descriptor.
> >
> 
> OK, it's very important to not cause regression here.
> 
> For the record, xfs_open_by_handle() is not related to
> open_by_handle_at() syscall.
> 
> It's an ioctl (XFS_IOC_OPEN_BY_HANDLE) which is used by xfsprogs's
> libhandle
> in functions open_by_fshandle() and open_by_handle().
> 
> http://sources.debian.net/src/xfsprogs/3.1.9/libhandle/handle.c?hl=284#L284
> http://sources.debian.net/src/xfsprogs/3.1.9/libhandle/handle.c?hl=308#L308
> 
> According to codesearch.debian.org, libhandle's open_by_handle() is
> only used
> by xfsdump
> 
> http://sources.debian.net/src/xfsdump/3.1.1/restore/tree.c?hl=2534#L2534
> 
> So there's no many *known* users of this features ... but it's more
> important
> not to break *unknown* users of it.

There are commercial products (i.e. proprietary, closed source) that
use it. SGI has one (DMF) and there are a couple of other backup
programs that have used it in the past, too.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

[PATCH 11/13] events: use get_unused_fd_flags(0) instead of get_unused_fd()

[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>
---
 kernel/events/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index 47bb0f7..c2feeb5 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -6810,7 +6810,7 @@ SYSCALL_DEFINE5(perf_event_open,
 	if ((flags & PERF_FLAG_PID_CGROUP) && (pid == -1 || cpu == -1))
 		return -EINVAL;
 
-	event_fd = get_unused_fd();
+	event_fd = get_unused_fd_flags(0);
 	if (event_fd < 0)
 		return event_fd;
 
-- 
1.8.3.1

[PATCH 12/13] sctp: use get_unused_fd_flags(0) instead of get_unused_fd()

[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>
---
 net/sctp/socket.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 66fcdcf..caa5919 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -4320,7 +4320,7 @@ static int sctp_getsockopt_peeloff(struct sock *sk, int len, char __user *optval
 		goto out;
 
 	/* Map the socket to an unused fd that can be returned to the user.  */
-	retval = get_unused_fd();
+	retval = get_unused_fd_flags(0);
 	if (retval < 0) {
 		sock_release(newsock);
 		goto out;
-- 
1.8.3.1

[PATCH 12/13] sctp: use get_unused_fd_flags(0) instead of get_unused_fd()

[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>
---
 net/sctp/socket.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 66fcdcf..caa5919 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -4320,7 +4320,7 @@ static int sctp_getsockopt_peeloff(struct sock *sk, int len, char __user *optval
 		goto out;
 
 	/* Map the socket to an unused fd that can be returned to the user.  */
-	retval = get_unused_fd();
+	retval = get_unused_fd_flags(0);
 	if (retval < 0) {
 		sock_release(newsock);
 		goto out;
-- 
1.8.3.1

Re: [PATCH 12/13] sctp: use get_unused_fd_flags(0) instead of get_unused_fd()

[Vlad Yasevich]

On 07/02/2013 12:39 PM, 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>

Acked-by: Vlad Yasevich <vyasevich@gmail.com>

-vlad

> ---
>   net/sctp/socket.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index 66fcdcf..caa5919 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -4320,7 +4320,7 @@ static int sctp_getsockopt_peeloff(struct sock *sk, int len, char __user *optval
>   		goto out;
>
>   	/* Map the socket to an unused fd that can be returned to the user.  */
> -	retval = get_unused_fd();
> +	retval = get_unused_fd_flags(0);
>   	if (retval < 0) {
>   		sock_release(newsock);
>   		goto out;
>

Re: [PATCH 12/13] sctp: use get_unused_fd_flags(0) instead of get_unused_fd()

[Vlad Yasevich]

On 07/02/2013 12:39 PM, 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>

Acked-by: Vlad Yasevich <vyasevich@gmail.com>

-vlad

> ---
>   net/sctp/socket.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index 66fcdcf..caa5919 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -4320,7 +4320,7 @@ static int sctp_getsockopt_peeloff(struct sock *sk, int len, char __user *optval
>   		goto out;
>
>   	/* Map the socket to an unused fd that can be returned to the user.  */
> -	retval = get_unused_fd();
> +	retval = get_unused_fd_flags(0);
>   	if (retval < 0) {
>   		sock_release(newsock);
>   		goto out;
>

Re: [PATCH 12/13] sctp: use get_unused_fd_flags(0) instead of get_unused_fd()

[David Miller]

From: Vlad Yasevich <vyasevich@gmail.com>
Date: Tue, 02 Jul 2013 13:50:32 -0400

> On 07/02/2013 12:39 PM, 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>
> 
> Acked-by: Vlad Yasevich <vyasevich@gmail.com>

Applied to net-next, thanks.

Re: [PATCH 12/13] sctp: use get_unused_fd_flags(0) instead of get_unused_fd()

[David Miller]

From: Vlad Yasevich <vyasevich@gmail.com>
Date: Tue, 02 Jul 2013 13:50:32 -0400

> On 07/02/2013 12:39 PM, 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>
> 
> Acked-by: Vlad Yasevich <vyasevich@gmail.com>

Applied to net-next, thanks.

[PATCH 13/13] file: remove get_unused_fd()

[Yann Droneaud]

Macro get_unused_fd() allocates a file descriptor without O_CLOEXEC flag.

This can be seen as an unsafe default: in most case O_CLOEXEC
must be used to not leak file descriptor across exec().

Using O_CLOEXEC by default allows userspace to choose, without race,
if the file descriptor is going to be inherited across exec().

This patch removes get_unused_fd() so that newer kernel code 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.

Signed-off-by: Yann Droneaud <ydroneaud@opteya.com>
---
 include/linux/file.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/include/linux/file.h b/include/linux/file.h
index cbacf4f..8666002 100644
--- a/include/linux/file.h
+++ b/include/linux/file.h
@@ -63,7 +63,6 @@ extern void set_close_on_exec(unsigned int fd, int flag);
 extern bool get_close_on_exec(unsigned int fd);
 extern void put_filp(struct file *);
 extern int get_unused_fd_flags(unsigned flags);
-#define get_unused_fd() get_unused_fd_flags(0)
 extern void put_unused_fd(unsigned int fd);
 
 extern void fd_install(unsigned int fd, struct file *file);
-- 
1.8.3.1