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 <[email protected]>
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;
(2011/06/10 12:55), [email protected] 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 <[email protected]>
>
> 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.
On Thu, 9 Jun 2011 23:55:08 -0400
[email protected] 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.
[email protected] 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 <[email protected]>
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 <[email protected]>
Cc: Geert Uytterhoeven <[email protected]>
Cc: Chris Zankel <[email protected]>
Cc: Ulrich Drepper <[email protected]>
Signed-off-by: Andrew Morton <[email protected]>
---
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;
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 <[email protected]>
--- 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);
On Thu, Jun 9, 2011 at 8:55 PM, <[email protected]> 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
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.
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.
On Wed, Jun 29, 2011 at 1:15 AM, Ulrich Drepper <[email protected]> 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
On Wed, Jun 29, 2011 at 9:22 AM, Linus Torvalds
<[email protected]> 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
-----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-----
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.
On Thu, Jun 30, 2011 at 6:39 AM, Ulrich Drepper <[email protected]> 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