[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 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()

[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