[PATCH] Add cloexec information to fdinfo

[PATCH] Add cloexec information to fdinfo

[drepper]

There is one piece of information about a file descriptor which is
currently not visible from the outside: the close-on-exec flag.  The
/proc/PID/fdinfo/* files have the mode information but this is
missing.  Is the following patch acceptable?

What I don't know is whether the RCU locking is needed given that
real locks are taken.  Someone with more knowledge could just
remove those two lines.


Signed-off-by: Ulrich Drepper <drepper@gmail.com>

 base.c |   17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 14def99..bda3651 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -1924,12 +1924,23 @@ static int proc_fd_info(struct inode *inode, struct path *path, char *info)
 				*path = file->f_path;
 				path_get(&file->f_path);
 			}
-			if (info)
+			if (info) {
+				int cloexec;
+				struct fdtable *fdt;
+
+				rcu_read_lock();
+				fdt = files_fdtable(files);
+				cloexec = FD_ISSET(fd, fdt->close_on_exec);
+				rcu_read_unlock();
+
 				snprintf(info, PROC_FDINFO_MAX,
 					 "pos:\t%lli\n"
-					 "flags:\t0%o\n",
+					 "flags:\t0%o\n"
+					 "cloexec: %d\n",
 					 (long long) file->f_pos,
-					 file->f_flags);
+					 file->f_flags,
+					 cloexec);
+			}
 			spin_unlock(&files->file_lock);
 			put_files_struct(files);
 			return 0;

Re: [PATCH] Add cloexec information to fdinfo

[KOSAKI Motohiro]

(2011/06/10 12:55), drepper@akkadia.org wrote:
> There is one piece of information about a file descriptor which is
> currently not visible from the outside: the close-on-exec flag.  The
> /proc/PID/fdinfo/* files have the mode information but this is
> missing.  Is the following patch acceptable?
> 
> What I don't know is whether the RCU locking is needed given that
> real locks are taken.  Someone with more knowledge could just
> remove those two lines.
> 
> 
> Signed-off-by: Ulrich Drepper <drepper@gmail.com>
> 
>  base.c |   17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 14def99..bda3651 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -1924,12 +1924,23 @@ static int proc_fd_info(struct inode *inode, struct path *path, char *info)
>  				*path = file->f_path;
>  				path_get(&file->f_path);
>  			}
> -			if (info)
> +			if (info) {
> +				int cloexec;
> +				struct fdtable *fdt;
> +
> +				rcu_read_lock();
> +				fdt = files_fdtable(files);
> +				cloexec = FD_ISSET(fd, fdt->close_on_exec);
> +				rcu_read_unlock();
> +
>  				snprintf(info, PROC_FDINFO_MAX,
>  					 "pos:\t%lli\n"
> -					 "flags:\t0%o\n",
> +					 "flags:\t0%o\n"
> +					 "cloexec: %d\n",
>  					 (long long) file->f_pos,
> -					 file->f_flags);
> +					 file->f_flags,
> +					 cloexec);
> +			}
>  			spin_unlock(&files->file_lock);
>  			put_files_struct(files);
>  			return 0;

I think this is correct and useful. However I don't know fdtable life cycle detail.
cc to linux-fsdevel.

Re: [PATCH] Add cloexec information to fdinfo

[Andrew Morton]

On Thu, 9 Jun 2011 23:55:08 -0400
drepper@akkadia.org wrote:

> There is one piece of information about a file descriptor which is
> currently not visible from the outside: the close-on-exec flag.  The
> /proc/PID/fdinfo/* files have the mode information but this is
> missing.  Is the following patch acceptable?
> 
> What I don't know is whether the RCU locking is needed given that
> real locks are taken.  Someone with more knowledge could just
> remove those two lines.

The locking looks OK to me.

Hopefully /prod/pid/fdinfo is documented somewhere. 
linux-man@vger.kernel.org appears to be the email address we use to rub
the documentation lamp.

> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 14def99..bda3651 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -1924,12 +1924,23 @@ static int proc_fd_info(struct inode *inode, struct path *path, char *info)
>  				*path = file->f_path;
>  				path_get(&file->f_path);
>  			}
> -			if (info)
> +			if (info) {
> +				int cloexec;
> +				struct fdtable *fdt;
> +
> +				rcu_read_lock();
> +				fdt = files_fdtable(files);
> +				cloexec = FD_ISSET(fd, fdt->close_on_exec);

Does FD_ISSET return 0 or 1?  Or 0 or non-zero?

For x86 it's the former.

<checks the architectures>

arch/h8300/include/asm/posix_types.h: busted
#define __FD_ISSET(d, set)      ((set)->fds_bits[__FDELT(d)] & __FDMASK(d))

arch/m68k/include/asm/posix_types.h: busted
#define __FD_ISSET(d, set)      ((set)->fds_bits[__FDELT(d)] & __FDMASK(d))

arch/xtensa/include/asm/posix_types.h: busted
#define __FD_ISSET(d, set)      ((set)->fds_bits[__FDELT(d)] & __FDMASK(d))


From: Andrew Morton <akpm@linux-foundation.org>

Harmonise these return values with other architectures.  In some cases
this affects all compilers and in other cases non-gcc compilers only.

Cc: Yoshinori Sato <ysato@users.sourceforge.jp>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Chris Zankel <chris@zankel.net>
Cc: Ulrich Drepper <drepper@gmail.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 arch/h8300/include/asm/posix_types.h  |    2 +-
 arch/m68k/include/asm/posix_types.h   |    2 +-
 arch/xtensa/include/asm/posix_types.h |    2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff -puN arch/h8300/include/asm/posix_types.h~h8300-m68k-xtensa-__fd_isset-should-return-0-1 arch/h8300/include/asm/posix_types.h
--- a/arch/h8300/include/asm/posix_types.h~h8300-m68k-xtensa-__fd_isset-should-return-0-1
+++ a/arch/h8300/include/asm/posix_types.h
@@ -50,7 +50,7 @@ typedef struct {
 #define	__FD_CLR(d, set)	((set)->fds_bits[__FDELT(d)] &= ~__FDMASK(d))
 
 #undef	__FD_ISSET
-#define	__FD_ISSET(d, set)	((set)->fds_bits[__FDELT(d)] & __FDMASK(d))
+#define	__FD_ISSET(d, set)	(!!((set)->fds_bits[__FDELT(d)] & __FDMASK(d)))
 
 #undef	__FD_ZERO
 #define __FD_ZERO(fdsetp) (memset (fdsetp, 0, sizeof(*(fd_set *)fdsetp)))
diff -puN arch/m68k/include/asm/posix_types.h~h8300-m68k-xtensa-__fd_isset-should-return-0-1 arch/m68k/include/asm/posix_types.h
--- a/arch/m68k/include/asm/posix_types.h~h8300-m68k-xtensa-__fd_isset-should-return-0-1
+++ a/arch/m68k/include/asm/posix_types.h
@@ -51,7 +51,7 @@ typedef struct {
 #define	__FD_CLR(d, set)	((set)->fds_bits[__FDELT(d)] &= ~__FDMASK(d))
 
 #undef	__FD_ISSET
-#define	__FD_ISSET(d, set)	((set)->fds_bits[__FDELT(d)] & __FDMASK(d))
+#define	__FD_ISSET(d, set)	(!!((set)->fds_bits[__FDELT(d)] & __FDMASK(d)))
 
 #undef	__FD_ZERO
 #define __FD_ZERO(fdsetp) (memset (fdsetp, 0, sizeof(*(fd_set *)fdsetp)))
diff -puN arch/xtensa/include/asm/posix_types.h~h8300-m68k-xtensa-__fd_isset-should-return-0-1 arch/xtensa/include/asm/posix_types.h
--- a/arch/xtensa/include/asm/posix_types.h~h8300-m68k-xtensa-__fd_isset-should-return-0-1
+++ a/arch/xtensa/include/asm/posix_types.h
@@ -58,7 +58,7 @@ typedef struct {
 
 #define	__FD_SET(d, set)	((set)->fds_bits[__FDELT(d)] |= __FDMASK(d))
 #define	__FD_CLR(d, set)	((set)->fds_bits[__FDELT(d)] &= ~__FDMASK(d))
-#define	__FD_ISSET(d, set)	((set)->fds_bits[__FDELT(d)] & __FDMASK(d))
+#define	__FD_ISSET(d, set)	(!!((set)->fds_bits[__FDELT(d)] & __FDMASK(d)))
 #define	__FD_ZERO(set)	\
   ((void) memset ((void *) (set), 0, sizeof (__kernel_fd_set)))
 
_


> +				rcu_read_unlock();
> +
>  				snprintf(info, PROC_FDINFO_MAX,
>  					 "pos:\t%lli\n"
> -					 "flags:\t0%o\n",
> +					 "flags:\t0%o\n"
> +					 "cloexec: %d\n",

Should use a tab here for consistency.

--- a/fs/proc/base.c~proc-pid-fdinfo-add-cloexec-information-fix
+++ a/fs/proc/base.c
@@ -1936,7 +1936,7 @@ static int proc_fd_info(struct inode *in
 				snprintf(info, PROC_FDINFO_MAX,
 					 "pos:\t%lli\n"
 					 "flags:\t0%o\n"
-					 "cloexec: %d\n",
+					 "cloexec:\t%d\n",
 					 (long long) file->f_pos,
 					 file->f_flags,
 					 cloexec);
_

>  					 (long long) file->f_pos,
> -					 file->f_flags);
> +					 file->f_flags,
> +					 cloexec);
> +			}
>  			spin_unlock(&files->file_lock);
>  			put_files_struct(files);
>  			return 0;

Re: [PATCH] Add cloexec information to fdinfo

[Ulrich Drepper]

[-- Attachment #1: Type: text/plain, Size: 1381 bytes --]

On 06/20/2011 05:31 PM, Andrew Morton wrote:
> Does FD_ISSET return 0 or 1?  Or 0 or non-zero?
> 
> For x86 it's the former.
> 
> <checks the architectures>
> 
> arch/h8300/include/asm/posix_types.h: busted
> #define __FD_ISSET(d, set)      ((set)->fds_bits[__FDELT(d)] & __FDMASK(d))
> 
> arch/m68k/include/asm/posix_types.h: busted
> #define __FD_ISSET(d, set)      ((set)->fds_bits[__FDELT(d)] & __FDMASK(d))
> 
> arch/xtensa/include/asm/posix_types.h: busted
> #define __FD_ISSET(d, set)      ((set)->fds_bits[__FDELT(d)] & __FDMASK(d))

I agree, this is an issue.  Existing code works around this.  We should
use the attached patch on top of the existing one.


This doesn't invalidate your patch to harmonize the architectures but
the above is what existing code does.  I didn't use a tab in the output
since the initial string is long enough to force the subsequent text in
a new column.  Using a single space seemed more visually pleasing.



Signed-off-by: Ulrich Drepper <drepper@gmail.com>

--- a/fs/proc/base.c	2011-06-28 02:54:01.888793757 -0400
+++ b/fs/proc/base.c	2011-06-28 02:54:30.668664335 -0400
@@ -1939,7 +1939,7 @@
 					 "cloexec:\t%d\n",
 					 (long long) file->f_pos,
 					 file->f_flags,
-					 cloexec);
+					 cloexec ? FD_CLOEXEC : 0);
 			}
 			spin_unlock(&files->file_lock);
 			put_files_struct(files);


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] Add cloexec information to fdinfo

[Linus Torvalds]

On Thu, Jun 9, 2011 at 8:55 PM,  <drepper@akkadia.org> wrote:
> There is one piece of information about a file descriptor which is
> currently not visible from the outside: the close-on-exec flag.  The
> /proc/PID/fdinfo/* files have the mode information but this is
> missing.  Is the following patch acceptable?

The description makes no mention of why this would be needed?

Also, it's by no means the only thing not visible in /proc. Things
like file locking status, leases, file descriptor ownership, signal
number associated with the setown etc.

So the kernel has _lots_ of internal file knowledge, and not all of it
makes sense to export to user space. What is so special about
close-on-exec?

                        Linus

Re: [PATCH] Add cloexec information to fdinfo

[Ulrich Drepper]

[-- Attachment #1: Type: text/plain, Size: 799 bytes --]

On 06/28/2011 01:23 PM, Linus Torvalds wrote:
> The description makes no mention of why this would be needed?

In this specific case I was trying to re-implement the pfiles program.
For normal file descriptors (not sockets) this was the last piece of
information which wasn't available.  This is all part of my "give
Solaris users no reason to not switch" effort.  I intend to offer the
code to the util-linux-ng maintainers.


> Also, it's by no means the only thing not visible in /proc. Things
> like file locking status, leases, file descriptor ownership, signal
> number associated with the setown etc.

True, and maybe that will be useful as well.

I might actually have some more patches to get to socket information but
I haven't fully checked out yet what is available.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] Add cloexec information to fdinfo

[Pádraig Brady]

On 29/06/11 09:15, Ulrich Drepper wrote:
> On 06/28/2011 01:23 PM, Linus Torvalds wrote:
>> The description makes no mention of why this would be needed?
> 
> In this specific case I was trying to re-implement the pfiles program.
> For normal file descriptors (not sockets) this was the last piece of
> information which wasn't available.  This is all part of my "give
> Solaris users no reason to not switch" effort.  I intend to offer the
> code to the util-linux-ng maintainers.

For the curious (me):

solaris:~ > pfiles $$
8418:   -bash
  Current rlimit: 256 file descriptors
   0: S_IFCHR mode:0620 dev:349,28 ino:130202 uid:11003 gid:7 rdev:24,116
      O_RDWR|O_NOCTTY|O_LARGEFILE
      /dev/pts/116
   1: S_IFCHR mode:0620 dev:349,28 ino:130202 uid:11003 gid:7 rdev:24,116
      O_RDWR|O_NOCTTY|O_LARGEFILE
      /dev/pts/116
   2: S_IFCHR mode:0620 dev:349,28 ino:130202 uid:11003 gid:7 rdev:24,116
      O_RDWR|O_NOCTTY|O_LARGEFILE
      /dev/pts/116
   3: S_IFDOOR mode:0444 dev:347,0 ino:189 uid:0 gid:0 size:0
      O_RDONLY|O_LARGEFILE FD_CLOEXEC  door to nscd[11276]
      /var/run/name_service_door
 255: S_IFCHR mode:0620 dev:349,28 ino:130202 uid:11003 gid:7 rdev:24,116
      O_RDWR|O_NOCTTY|O_LARGEFILE FD_CLOEXEC
      /dev/pts/116

I guess `lsof +f g` could be suitably embellished as well.

cheers,
Pádraig.

Re: [PATCH] Add cloexec information to fdinfo

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 1307 bytes --]

On Wed, Jun 29, 2011 at 1:15 AM, Ulrich Drepper <drepper@akkadia.org> wrote:
>
> In this specific case I was trying to re-implement the pfiles program.
> For normal file descriptors (not sockets) this was the last piece of
> information which wasn't available.  This is all part of my "give
> Solaris users no reason to not switch" effort.  I intend to offer the
> code to the util-linux-ng maintainers.

Ok.

So the part I really dislike about the patch is how it makes O_CLOEXEC
so magically different from the other O_xyz flags.

Wouldn't it be much nicer to just show it in the "flags:" line?

Also, I don't think you need the rcu locking, since we hold the
files->file_lock here anyway.

>> Also, it's by no means the only thing not visible in /proc. Things
>> like file locking status, leases, file descriptor ownership, signal
>> number associated with the setown etc.
>
> True, and maybe that will be useful as well.
>
> I might actually have some more patches to get to socket information but
> I haven't fully checked out yet what is available.

So at least O_CLOEXEC seems to have a real reason, and fits the
existing model. I think a patch like the attached would be ok. Does
that work for you? It's entirely untested here.

                                Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 960 bytes --]

 fs/proc/base.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index fc5bc2767692..e2a016f24d38 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -1920,6 +1920,14 @@ static int proc_fd_info(struct inode *inode, struct path *path, char *info)
 		spin_lock(&files->file_lock);
 		file = fcheck_files(files, fd);
 		if (file) {
+			unsigned int f_flags;
+			struct fdtable *fdt;
+
+			fdt = files_fdtable(files);
+			f_flags = file->f_flags & ~O_CLOEXEC;
+			if (FD_ISSET(fd, fdt->close_on_exec))
+				f_flags |= O_CLOEXEC;
+
 			if (path) {
 				*path = file->f_path;
 				path_get(&file->f_path);
@@ -1929,7 +1937,7 @@ static int proc_fd_info(struct inode *inode, struct path *path, char *info)
 					 "pos:\t%lli\n"
 					 "flags:\t0%o\n",
 					 (long long) file->f_pos,
-					 file->f_flags);
+					 f_flags);
 			spin_unlock(&files->file_lock);
 			put_files_struct(files);
 			return 0;

Re: [PATCH] Add cloexec information to fdinfo

[Linus Torvalds]

On Wed, Jun 29, 2011 at 9:22 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> So at least O_CLOEXEC seems to have a real reason, and fits the
> existing model. I think a patch like the attached would be ok. Does
> that work for you? It's entirely untested here.

Ok, minimally tested now, it seems to work from the one trivial
test-case I wrote for it.

I'm not going to commit it, but if others think this is a reasonable
thing to do and remind me during the next merge window, we can do it
then.

                        Linus

Re: [PATCH] Add cloexec information to fdinfo

[Ulrich Drepper]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 06/29/2011 02:05 PM, Linus Torvalds wrote:
> Ok, minimally tested now, it seems to work from the one trivial
> test-case I wrote for it.
> 
> I'm not going to commit it, but if others think this is a reasonable
> thing to do and remind me during the next merge window, we can do it
> then.

It provides the information, yes, but it also hides some information.
If you do it this way we cannot distinguish code which uses O_CLOEXEC at
open-time from uses of fcntl(FD_CLOEXEC).

This is not needed for the specific application I have in mind.  But if
you, for instance, want to track down code which uses fcntl instead of
doing the work at open-time using O_CLOEXEC we would need this information.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAk4L5qQACgkQ2ijCOnn/RHSplwCgrA7RXs9gI7sr4e7SuvYsI04b
dmEAnA9eD4k9ZdFoJuQvGoXkBkjolfZn
=6GVj
-----END PGP SIGNATURE-----

Re: [PATCH] Add cloexec information to fdinfo

[Ulrich Drepper]

[-- Attachment #1: Type: text/plain, Size: 622 bytes --]

On 06/29/2011 10:59 PM, Ulrich Drepper wrote:
> It provides the information, yes, but it also hides some information.
> If you do it this way we cannot distinguish code which uses O_CLOEXEC at
> open-time from uses of fcntl(FD_CLOEXEC).

One more reason to at least not use the patch as you have it right now.
 If a file descriptor was opened with O_CLOEXEC but subsequently the bit
has been reset using fcntl() the f_flags value would still have the
O_CLOEXEC bit set.

If you don't like the separate cloexec line, at least reset the
O_CLOEXEC bit in the f_flags output if the FD_ISSET() test returns zero.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] Add cloexec information to fdinfo

[Linus Torvalds]

On Thu, Jun 30, 2011 at 6:39 AM, Ulrich Drepper <drepper@akkadia.org> wrote:
>
> One more reason to at least not use the patch as you have it right now.
>  If a file descriptor was opened with O_CLOEXEC but subsequently the bit
> has been reset using fcntl() the f_flags value would still have the
> O_CLOEXEC bit set.

Umm. Look at my patch again. It masks the O_CLOEXEC bit *exactly*
because the bit is useless as it is now.

Which is why your previous objection was so silly too and I didn't
even bother replying to the blather. O_CLOEXEC at open time has no
special magical meaning. The only meaningful value for that bit is the
current one.

Which is exactly what my patch did.

> If you don't like the separate cloexec line, at least reset the
> O_CLOEXEC bit in the f_flags output if the FD_ISSET() test returns zero.

How about you just read the patch again?

This issue is closed as far as I am concerned. I repeat from my previous email:

  "I'm not going to commit it, but if others think this is a reasonable
   thing to do and remind me during the next merge window, we can
   do it then."

Gaah.

                      Linus

Re: [PATCH] Add cloexec information to fdinfo

[Ulrich Drepper]

[-- Attachment #1: Type: text/plain, Size: 299 bytes --]

On 06/30/2011 12:07 PM, Linus Torvalds wrote:
>   "I'm not going to commit it, but if others think this is a reasonable
>    thing to do and remind me during the next merge window, we can
>    do it then."

Can you please accept this message as a reminder to include your change?

Thanks.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] Add cloexec information to fdinfo

[Linus Torvalds]

On Fri, Aug 5, 2011 at 4:49 AM, Ulrich Drepper <drepper@akkadia.org> wrote:
>
> Can you please accept this message as a reminder to include your change?

Thanks, done,

                          Linus