[PATCH 0/2] procfs: Add file path and size to /proc/<pid>/fdinfo

[PATCH 0/2] procfs: Add file path and size to /proc/<pid>/fdinfo

[Kalesh Singh]

Processes can pin shared memory by keeping a handle to it through a
file descriptor; for instance dmabufs, memfd, and ashmem (in Android).

In the case of a memory leak, to identify the process pinning the
memory, userspace needs to:
  - Iterate the /proc/<pid>/fd/* for each process
  - Do a readlink on each entry to identify the type of memory from
    the file path.
  - stat() each entry to get the size of the memory.

The file permissions on /proc/<pid>/fd/* only allows for the owner
or root to perform the operations above; and so is not suitable for
capturing the system-wide state in a production environment.

This issue was addressed for dmabufs by making /proc/*/fdinfo/*
accessible to a process with PTRACE_MODE_READ_FSCREDS credentials[1]
To allow the same kind of tracking for other types of shared memory,
add the following fields to /proc/<pid>/fdinfo/<fd>:

path - This allows identifying the type of memory based on common
       prefixes: e.g. "/memfd...", "/dmabuf...", "/dev/ashmem..."

       This was not an issued when dmabuf tracking was introduced
       because the exp_name field of dmabuf fdinfo could be used
       to distinguish dmabuf fds from other types.

size - To track the amount of memory that is being pinned.

       dmabufs expose size as an additional field in fdinfo. Remove
       this and make it a common field for all fds.

Access to /proc/<pid>/fdinfo is governed by PTRACE_MODE_READ_FSCREDS
-- the same as for /proc/<pid>/maps which also exposes the path and
size for mapped memory regions.

This allows for a system process with PTRACE_MODE_READ_FSCREDS to
account the pinned per-process memory via fdinfo.

-----

There was some concern about exposing the file path in the RFC[2], to that
effect the change was split into separte patches. Also retrieving the file
path from fdinfo is guarded by the same capability (PTRACE_MODE_READ) as
/proc/<pid>/maps which also exposes file path, so this may not be an issue.

[1] https://lore.kernel.org/r/20210308170651.919148-1-kaleshsingh@google.com/
[2] https://lore.kernel.org/r/20220519214021.3572840-1-kaleshsingh@google.com/


Kalesh Singh (2):
  procfs: Add 'size' to /proc/<pid>/fdinfo/
  procfs: Add 'path' to /proc/<pid>/fdinfo/

 Documentation/filesystems/proc.rst | 22 ++++++++++++++++++++--
 drivers/dma-buf/dma-buf.c          |  1 -
 fs/proc/fd.c                       | 13 +++++++++----
 3 files changed, 29 insertions(+), 7 deletions(-)


base-commit: 8ab2afa23bd197df47819a87f0265c0ac95c5b6a
-- 
2.36.1.255.ge46751e96f-goog

[PATCH 1/2] procfs: Add 'size' to /proc/<pid>/fdinfo/

[Kalesh Singh]

To be able to account the amount of memory a process is keeping pinned
by open file descriptors add a 'size' field to fdinfo output.

dmabufs fds already expose a 'size' field for this reason, remove this
and make it a common field for all fds. This allows tracking of
other types of memory (e.g. memfd and ashmem in Android).

Signed-off-by: Kalesh Singh <kaleshsingh@google.com>
---

Changes from rfc:
  - Split adding 'size' and 'path' into a separate patches, per Christian
  - Split fdinfo seq_printf into separate lines, per Christian
  - Fix indentation (use tabs) in documentaion, per Randy

 Documentation/filesystems/proc.rst | 12 ++++++++++--
 drivers/dma-buf/dma-buf.c          |  1 -
 fs/proc/fd.c                       |  9 +++++----
 3 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/Documentation/filesystems/proc.rst b/Documentation/filesystems/proc.rst
index 1bc91fb8c321..779c05528e87 100644
--- a/Documentation/filesystems/proc.rst
+++ b/Documentation/filesystems/proc.rst
@@ -1886,13 +1886,14 @@ if precise results are needed.
 3.8	/proc/<pid>/fdinfo/<fd> - Information about opened file
 ---------------------------------------------------------------
 This file provides information associated with an opened file. The regular
-files have at least four fields -- 'pos', 'flags', 'mnt_id' and 'ino'.
+files have at least five fields -- 'pos', 'flags', 'mnt_id', 'ino', and 'size'.
+
 The 'pos' represents the current offset of the opened file in decimal
 form [see lseek(2) for details], 'flags' denotes the octal O_xxx mask the
 file has been created with [see open(2) for details] and 'mnt_id' represents
 mount ID of the file system containing the opened file [see 3.5
 /proc/<pid>/mountinfo for details]. 'ino' represents the inode number of
-the file.
+the file, and 'size' represents the size of the file in bytes.
 
 A typical output is::
 
@@ -1900,6 +1901,7 @@ A typical output is::
 	flags:	0100002
 	mnt_id:	19
 	ino:	63107
+	size:	0
 
 All locks associated with a file descriptor are shown in its fdinfo too::
 
@@ -1917,6 +1919,7 @@ Eventfd files
 	flags:	04002
 	mnt_id:	9
 	ino:	63107
+	size:   0
 	eventfd-count:	5a
 
 where 'eventfd-count' is hex value of a counter.
@@ -1930,6 +1933,7 @@ Signalfd files
 	flags:	04002
 	mnt_id:	9
 	ino:	63107
+	size:   0
 	sigmask:	0000000000000200
 
 where 'sigmask' is hex value of the signal mask associated
@@ -1944,6 +1948,7 @@ Epoll files
 	flags:	02
 	mnt_id:	9
 	ino:	63107
+	size:   0
 	tfd:        5 events:       1d data: ffffffffffffffff pos:0 ino:61af sdev:7
 
 where 'tfd' is a target file descriptor number in decimal form,
@@ -1962,6 +1967,7 @@ For inotify files the format is the following::
 	flags:	02000000
 	mnt_id:	9
 	ino:	63107
+	size:   0
 	inotify wd:3 ino:9e7e sdev:800013 mask:800afce ignored_mask:0 fhandle-bytes:8 fhandle-type:1 f_handle:7e9e0000640d1b6d
 
 where 'wd' is a watch descriptor in decimal form, i.e. a target file
@@ -1985,6 +1991,7 @@ For fanotify files the format is::
 	flags:	02
 	mnt_id:	9
 	ino:	63107
+	size:   0
 	fanotify flags:10 event-flags:0
 	fanotify mnt_id:12 mflags:40 mask:38 ignored_mask:40000003
 	fanotify ino:4f969 sdev:800013 mflags:0 mask:3b ignored_mask:40000000 fhandle-bytes:8 fhandle-type:1 f_handle:69f90400c275b5b4
@@ -2010,6 +2017,7 @@ Timerfd files
 	flags:	02
 	mnt_id:	9
 	ino:	63107
+	size:   0
 	clockid: 0
 	ticks: 0
 	settime flags: 01
diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index 32f55640890c..5f2ae38c960f 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -378,7 +378,6 @@ static void dma_buf_show_fdinfo(struct seq_file *m, struct file *file)
 {
 	struct dma_buf *dmabuf = file->private_data;
 
-	seq_printf(m, "size:\t%zu\n", dmabuf->size);
 	/* Don't count the temporary reference taken inside procfs seq_show */
 	seq_printf(m, "count:\t%ld\n", file_count(dmabuf->file) - 1);
 	seq_printf(m, "exp_name:\t%s\n", dmabuf->exp_name);
diff --git a/fs/proc/fd.c b/fs/proc/fd.c
index 913bef0d2a36..464bc3f55759 100644
--- a/fs/proc/fd.c
+++ b/fs/proc/fd.c
@@ -54,10 +54,11 @@ static int seq_show(struct seq_file *m, void *v)
 	if (ret)
 		return ret;
 
-	seq_printf(m, "pos:\t%lli\nflags:\t0%o\nmnt_id:\t%i\nino:\t%lu\n",
-		   (long long)file->f_pos, f_flags,
-		   real_mount(file->f_path.mnt)->mnt_id,
-		   file_inode(file)->i_ino);
+	seq_printf(m, "pos:\t%lli\n", (long long)file->f_pos);
+	seq_printf(m, "flags:\t0%o\n", f_flags);
+	seq_printf(m, "mnt_id:\t%i\n", real_mount(file->f_path.mnt)->mnt_id);
+	seq_printf(m, "ino:\t%lu\n", file_inode(file)->i_ino);
+	seq_printf(m, "size:\t%lli\n", (long long)file_inode(file)->i_size);
 
 	/* show_fd_locks() never deferences files so a stale value is safe */
 	show_fd_locks(m, file, files);
-- 
2.36.1.255.ge46751e96f-goog

[PATCH 2/2] procfs: Add 'path' to /proc/<pid>/fdinfo/

[Kalesh Singh]

In order to identify the type of memory a process has pinned through
its open fds, add the file path to fdinfo output. This allows
identifying memory types based on common prefixes. e.g. "/memfd...",
"/dmabuf...", "/dev/ashmem...".

Access to /proc/<pid>/fdinfo is governed by PTRACE_MODE_READ_FSCREDS
the same as /proc/<pid>/maps which also exposes the file path of
mappings; so the security permissions for accessing path is consistent
with that of /proc/<pid>/maps.

Signed-off-by: Kalesh Singh <kaleshsingh@google.com>
---

Changes from rfc:
  - Split adding 'size' and 'path' into a separate patches, per Christian
  - Fix indentation (use tabs) in documentaion, per Randy

 Documentation/filesystems/proc.rst | 14 ++++++++++++--
 fs/proc/fd.c                       |  4 ++++
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/Documentation/filesystems/proc.rst b/Documentation/filesystems/proc.rst
index 779c05528e87..591f12d30d97 100644
--- a/Documentation/filesystems/proc.rst
+++ b/Documentation/filesystems/proc.rst
@@ -1886,14 +1886,16 @@ if precise results are needed.
 3.8	/proc/<pid>/fdinfo/<fd> - Information about opened file
 ---------------------------------------------------------------
 This file provides information associated with an opened file. The regular
-files have at least five fields -- 'pos', 'flags', 'mnt_id', 'ino', and 'size'.
+files have at least six fields -- 'pos', 'flags', 'mnt_id', 'ino', 'size',
+and 'path'.
 
 The 'pos' represents the current offset of the opened file in decimal
 form [see lseek(2) for details], 'flags' denotes the octal O_xxx mask the
 file has been created with [see open(2) for details] and 'mnt_id' represents
 mount ID of the file system containing the opened file [see 3.5
 /proc/<pid>/mountinfo for details]. 'ino' represents the inode number of
-the file, and 'size' represents the size of the file in bytes.
+the file, 'size' represents the size of the file in bytes, and 'path'
+represents the file path.
 
 A typical output is::
 
@@ -1902,6 +1904,7 @@ A typical output is::
 	mnt_id:	19
 	ino:	63107
 	size:	0
+	path:	/dev/null
 
 All locks associated with a file descriptor are shown in its fdinfo too::
 
@@ -1920,6 +1923,7 @@ Eventfd files
 	mnt_id:	9
 	ino:	63107
 	size:   0
+	path:	anon_inode:[eventfd]
 	eventfd-count:	5a
 
 where 'eventfd-count' is hex value of a counter.
@@ -1934,6 +1938,7 @@ Signalfd files
 	mnt_id:	9
 	ino:	63107
 	size:   0
+	path:	anon_inode:[signalfd]
 	sigmask:	0000000000000200
 
 where 'sigmask' is hex value of the signal mask associated
@@ -1949,6 +1954,7 @@ Epoll files
 	mnt_id:	9
 	ino:	63107
 	size:   0
+	path:	anon_inode:[eventpoll]
 	tfd:        5 events:       1d data: ffffffffffffffff pos:0 ino:61af sdev:7
 
 where 'tfd' is a target file descriptor number in decimal form,
@@ -1968,6 +1974,7 @@ For inotify files the format is the following::
 	mnt_id:	9
 	ino:	63107
 	size:   0
+	path:	anon_inode:inotify
 	inotify wd:3 ino:9e7e sdev:800013 mask:800afce ignored_mask:0 fhandle-bytes:8 fhandle-type:1 f_handle:7e9e0000640d1b6d
 
 where 'wd' is a watch descriptor in decimal form, i.e. a target file
@@ -1992,6 +1999,7 @@ For fanotify files the format is::
 	mnt_id:	9
 	ino:	63107
 	size:   0
+	path:	anon_inode:[fanotify]
 	fanotify flags:10 event-flags:0
 	fanotify mnt_id:12 mflags:40 mask:38 ignored_mask:40000003
 	fanotify ino:4f969 sdev:800013 mflags:0 mask:3b ignored_mask:40000000 fhandle-bytes:8 fhandle-type:1 f_handle:69f90400c275b5b4
@@ -2018,6 +2026,7 @@ Timerfd files
 	mnt_id:	9
 	ino:	63107
 	size:   0
+	path:	anon_inode:[timerfd]
 	clockid: 0
 	ticks: 0
 	settime flags: 01
@@ -2042,6 +2051,7 @@ DMA Buffer files
 	mnt_id:	9
 	ino:	63107
 	size:   32768
+	path:	/dmabuf:
 	count:  2
 	exp_name:  system-heap
 
diff --git a/fs/proc/fd.c b/fs/proc/fd.c
index 464bc3f55759..8889a8ba09d4 100644
--- a/fs/proc/fd.c
+++ b/fs/proc/fd.c
@@ -60,6 +60,10 @@ static int seq_show(struct seq_file *m, void *v)
 	seq_printf(m, "ino:\t%lu\n", file_inode(file)->i_ino);
 	seq_printf(m, "size:\t%lli\n", (long long)file_inode(file)->i_size);
 
+	seq_puts(m, "path:\t");
+	seq_file_path(m, file, "\n");
+	seq_putc(m, '\n');
+
 	/* show_fd_locks() never deferences files so a stale value is safe */
 	show_fd_locks(m, file, files);
 	if (seq_has_overflowed(m))
-- 
2.36.1.255.ge46751e96f-goog

Re: [PATCH 2/2] procfs: Add 'path' to /proc/<pid>/fdinfo/

[Stephen Brennan]

On 5/31/22 14:25, Kalesh Singh wrote:
> In order to identify the type of memory a process has pinned through
> its open fds, add the file path to fdinfo output. This allows
> identifying memory types based on common prefixes. e.g. "/memfd...",
> "/dmabuf...", "/dev/ashmem...".
> 
> Access to /proc/<pid>/fdinfo is governed by PTRACE_MODE_READ_FSCREDS
> the same as /proc/<pid>/maps which also exposes the file path of
> mappings; so the security permissions for accessing path is consistent
> with that of /proc/<pid>/maps.

Hi Kalesh,

I think I see the value in the size field, but I'm curious about path,
which is available via readlink /proc/<pid>/fd/<n>, since those are
symlinks to the file themselves.

File paths can contain fun characters like newlines or colons, which
could make parsing out filenames in this text file... fun. How would your
userspace parsing logic handle "/home/stephen/filename\nsize:\t4096"? The
readlink(2) API makes that easy already.

Is the goal avoiding races (e.g. file descriptor 3 is closed and reopened
to a different path between reading fdinfo and stating the fd)?

Stephen

> Signed-off-by: Kalesh Singh <kaleshsingh@google.com>
> ---
> 
> Changes from rfc:
>   - Split adding 'size' and 'path' into a separate patches, per Christian
>   - Fix indentation (use tabs) in documentaion, per Randy
> 
>  Documentation/filesystems/proc.rst | 14 ++++++++++++--
>  fs/proc/fd.c                       |  4 ++++
>  2 files changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/filesystems/proc.rst b/Documentation/filesystems/proc.rst
> index 779c05528e87..591f12d30d97 100644
> --- a/Documentation/filesystems/proc.rst
> +++ b/Documentation/filesystems/proc.rst
> @@ -1886,14 +1886,16 @@ if precise results are needed.
>  3.8	/proc/<pid>/fdinfo/<fd> - Information about opened file
>  ---------------------------------------------------------------
>  This file provides information associated with an opened file. The regular
> -files have at least five fields -- 'pos', 'flags', 'mnt_id', 'ino', and 'size'.
> +files have at least six fields -- 'pos', 'flags', 'mnt_id', 'ino', 'size',
> +and 'path'.
>  
>  The 'pos' represents the current offset of the opened file in decimal
>  form [see lseek(2) for details], 'flags' denotes the octal O_xxx mask the
>  file has been created with [see open(2) for details] and 'mnt_id' represents
>  mount ID of the file system containing the opened file [see 3.5
>  /proc/<pid>/mountinfo for details]. 'ino' represents the inode number of
> -the file, and 'size' represents the size of the file in bytes.
> +the file, 'size' represents the size of the file in bytes, and 'path'
> +represents the file path.
>  
>  A typical output is::
>  
> @@ -1902,6 +1904,7 @@ A typical output is::
>  	mnt_id:	19
>  	ino:	63107
>  	size:	0
> +	path:	/dev/null
>  
>  All locks associated with a file descriptor are shown in its fdinfo too::
>  
> @@ -1920,6 +1923,7 @@ Eventfd files
>  	mnt_id:	9
>  	ino:	63107
>  	size:   0
> +	path:	anon_inode:[eventfd]
>  	eventfd-count:	5a
>  
>  where 'eventfd-count' is hex value of a counter.
> @@ -1934,6 +1938,7 @@ Signalfd files
>  	mnt_id:	9
>  	ino:	63107
>  	size:   0
> +	path:	anon_inode:[signalfd]
>  	sigmask:	0000000000000200
>  
>  where 'sigmask' is hex value of the signal mask associated
> @@ -1949,6 +1954,7 @@ Epoll files
>  	mnt_id:	9
>  	ino:	63107
>  	size:   0
> +	path:	anon_inode:[eventpoll]
>  	tfd:        5 events:       1d data: ffffffffffffffff pos:0 ino:61af sdev:7
>  
>  where 'tfd' is a target file descriptor number in decimal form,
> @@ -1968,6 +1974,7 @@ For inotify files the format is the following::
>  	mnt_id:	9
>  	ino:	63107
>  	size:   0
> +	path:	anon_inode:inotify
>  	inotify wd:3 ino:9e7e sdev:800013 mask:800afce ignored_mask:0 fhandle-bytes:8 fhandle-type:1 f_handle:7e9e0000640d1b6d
>  
>  where 'wd' is a watch descriptor in decimal form, i.e. a target file
> @@ -1992,6 +1999,7 @@ For fanotify files the format is::
>  	mnt_id:	9
>  	ino:	63107
>  	size:   0
> +	path:	anon_inode:[fanotify]
>  	fanotify flags:10 event-flags:0
>  	fanotify mnt_id:12 mflags:40 mask:38 ignored_mask:40000003
>  	fanotify ino:4f969 sdev:800013 mflags:0 mask:3b ignored_mask:40000000 fhandle-bytes:8 fhandle-type:1 f_handle:69f90400c275b5b4
> @@ -2018,6 +2026,7 @@ Timerfd files
>  	mnt_id:	9
>  	ino:	63107
>  	size:   0
> +	path:	anon_inode:[timerfd]
>  	clockid: 0
>  	ticks: 0
>  	settime flags: 01
> @@ -2042,6 +2051,7 @@ DMA Buffer files
>  	mnt_id:	9
>  	ino:	63107
>  	size:   32768
> +	path:	/dmabuf:
>  	count:  2
>  	exp_name:  system-heap
>  
> diff --git a/fs/proc/fd.c b/fs/proc/fd.c
> index 464bc3f55759..8889a8ba09d4 100644
> --- a/fs/proc/fd.c
> +++ b/fs/proc/fd.c
> @@ -60,6 +60,10 @@ static int seq_show(struct seq_file *m, void *v)
>  	seq_printf(m, "ino:\t%lu\n", file_inode(file)->i_ino);
>  	seq_printf(m, "size:\t%lli\n", (long long)file_inode(file)->i_size);
>  
> +	seq_puts(m, "path:\t");
> +	seq_file_path(m, file, "\n");
> +	seq_putc(m, '\n');
> +
>  	/* show_fd_locks() never deferences files so a stale value is safe */
>  	show_fd_locks(m, file, files);
>  	if (seq_has_overflowed(m))

Re: [PATCH 2/2] procfs: Add 'path' to /proc/<pid>/fdinfo/

[Kalesh Singh]

On Tue, May 31, 2022 at 3:07 PM Stephen Brennan
<stephen.s.brennan@oracle.com> wrote:
>
> On 5/31/22 14:25, Kalesh Singh wrote:
> > In order to identify the type of memory a process has pinned through
> > its open fds, add the file path to fdinfo output. This allows
> > identifying memory types based on common prefixes. e.g. "/memfd...",
> > "/dmabuf...", "/dev/ashmem...".
> >
> > Access to /proc/<pid>/fdinfo is governed by PTRACE_MODE_READ_FSCREDS
> > the same as /proc/<pid>/maps which also exposes the file path of
> > mappings; so the security permissions for accessing path is consistent
> > with that of /proc/<pid>/maps.
>
> Hi Kalesh,

Hi Stephen,

Thanks for taking a look.

>
> I think I see the value in the size field, but I'm curious about path,
> which is available via readlink /proc/<pid>/fd/<n>, since those are
> symlinks to the file themselves.

This could work if we are root, but the file permissions wouldn't
allow us to do the readlink on other processes otherwise. We want to
be able to capture the system state in production environments from
some trusted process with ptrace read capability.

>
> File paths can contain fun characters like newlines or colons, which
> could make parsing out filenames in this text file... fun. How would your
> userspace parsing logic handle "/home/stephen/filename\nsize:\t4096"? The
> readlink(2) API makes that easy already.

I think since we have escaped the "\n" (seq_file_path(m, file, "\n")),
then user space might parse this line like:

if (strncmp(line, "path:\t", 6) == 0)
        char* path = line + 6;


Thanks,
Kalesh

>
> Is the goal avoiding races (e.g. file descriptor 3 is closed and reopened
> to a different path between reading fdinfo and stating the fd)?
>
> Stephen
>
> > Signed-off-by: Kalesh Singh <kaleshsingh@google.com>
> > ---
> >
> > Changes from rfc:
> >   - Split adding 'size' and 'path' into a separate patches, per Christian
> >   - Fix indentation (use tabs) in documentaion, per Randy
> >
> >  Documentation/filesystems/proc.rst | 14 ++++++++++++--
> >  fs/proc/fd.c                       |  4 ++++
> >  2 files changed, 16 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/filesystems/proc.rst b/Documentation/filesystems/proc.rst
> > index 779c05528e87..591f12d30d97 100644
> > --- a/Documentation/filesystems/proc.rst
> > +++ b/Documentation/filesystems/proc.rst
> > @@ -1886,14 +1886,16 @@ if precise results are needed.
> >  3.8  /proc/<pid>/fdinfo/<fd> - Information about opened file
> >  ---------------------------------------------------------------
> >  This file provides information associated with an opened file. The regular
> > -files have at least five fields -- 'pos', 'flags', 'mnt_id', 'ino', and 'size'.
> > +files have at least six fields -- 'pos', 'flags', 'mnt_id', 'ino', 'size',
> > +and 'path'.
> >
> >  The 'pos' represents the current offset of the opened file in decimal
> >  form [see lseek(2) for details], 'flags' denotes the octal O_xxx mask the
> >  file has been created with [see open(2) for details] and 'mnt_id' represents
> >  mount ID of the file system containing the opened file [see 3.5
> >  /proc/<pid>/mountinfo for details]. 'ino' represents the inode number of
> > -the file, and 'size' represents the size of the file in bytes.
> > +the file, 'size' represents the size of the file in bytes, and 'path'
> > +represents the file path.
> >
> >  A typical output is::
> >
> > @@ -1902,6 +1904,7 @@ A typical output is::
> >       mnt_id: 19
> >       ino:    63107
> >       size:   0
> > +     path:   /dev/null
> >
> >  All locks associated with a file descriptor are shown in its fdinfo too::
> >
> > @@ -1920,6 +1923,7 @@ Eventfd files
> >       mnt_id: 9
> >       ino:    63107
> >       size:   0
> > +     path:   anon_inode:[eventfd]
> >       eventfd-count:  5a
> >
> >  where 'eventfd-count' is hex value of a counter.
> > @@ -1934,6 +1938,7 @@ Signalfd files
> >       mnt_id: 9
> >       ino:    63107
> >       size:   0
> > +     path:   anon_inode:[signalfd]
> >       sigmask:        0000000000000200
> >
> >  where 'sigmask' is hex value of the signal mask associated
> > @@ -1949,6 +1954,7 @@ Epoll files
> >       mnt_id: 9
> >       ino:    63107
> >       size:   0
> > +     path:   anon_inode:[eventpoll]
> >       tfd:        5 events:       1d data: ffffffffffffffff pos:0 ino:61af sdev:7
> >
> >  where 'tfd' is a target file descriptor number in decimal form,
> > @@ -1968,6 +1974,7 @@ For inotify files the format is the following::
> >       mnt_id: 9
> >       ino:    63107
> >       size:   0
> > +     path:   anon_inode:inotify
> >       inotify wd:3 ino:9e7e sdev:800013 mask:800afce ignored_mask:0 fhandle-bytes:8 fhandle-type:1 f_handle:7e9e0000640d1b6d
> >
> >  where 'wd' is a watch descriptor in decimal form, i.e. a target file
> > @@ -1992,6 +1999,7 @@ For fanotify files the format is::
> >       mnt_id: 9
> >       ino:    63107
> >       size:   0
> > +     path:   anon_inode:[fanotify]
> >       fanotify flags:10 event-flags:0
> >       fanotify mnt_id:12 mflags:40 mask:38 ignored_mask:40000003
> >       fanotify ino:4f969 sdev:800013 mflags:0 mask:3b ignored_mask:40000000 fhandle-bytes:8 fhandle-type:1 f_handle:69f90400c275b5b4
> > @@ -2018,6 +2026,7 @@ Timerfd files
> >       mnt_id: 9
> >       ino:    63107
> >       size:   0
> > +     path:   anon_inode:[timerfd]
> >       clockid: 0
> >       ticks: 0
> >       settime flags: 01
> > @@ -2042,6 +2051,7 @@ DMA Buffer files
> >       mnt_id: 9
> >       ino:    63107
> >       size:   32768
> > +     path:   /dmabuf:
> >       count:  2
> >       exp_name:  system-heap
> >
> > diff --git a/fs/proc/fd.c b/fs/proc/fd.c
> > index 464bc3f55759..8889a8ba09d4 100644
> > --- a/fs/proc/fd.c
> > +++ b/fs/proc/fd.c
> > @@ -60,6 +60,10 @@ static int seq_show(struct seq_file *m, void *v)
> >       seq_printf(m, "ino:\t%lu\n", file_inode(file)->i_ino);
> >       seq_printf(m, "size:\t%lli\n", (long long)file_inode(file)->i_size);
> >
> > +     seq_puts(m, "path:\t");
> > +     seq_file_path(m, file, "\n");
> > +     seq_putc(m, '\n');
> > +
> >       /* show_fd_locks() never deferences files so a stale value is safe */
> >       show_fd_locks(m, file, files);
> >       if (seq_has_overflowed(m))
>
> --
> To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com.
>

Re: [PATCH 2/2] procfs: Add 'path' to /proc/<pid>/fdinfo/

[Stephen Brennan]

Kalesh Singh <kaleshsingh@google.com> writes:
> On Tue, May 31, 2022 at 3:07 PM Stephen Brennan
> <stephen.s.brennan@oracle.com> wrote:
>>
>> On 5/31/22 14:25, Kalesh Singh wrote:
>> > In order to identify the type of memory a process has pinned through
>> > its open fds, add the file path to fdinfo output. This allows
>> > identifying memory types based on common prefixes. e.g. "/memfd...",
>> > "/dmabuf...", "/dev/ashmem...".
>> >
>> > Access to /proc/<pid>/fdinfo is governed by PTRACE_MODE_READ_FSCREDS
>> > the same as /proc/<pid>/maps which also exposes the file path of
>> > mappings; so the security permissions for accessing path is consistent
>> > with that of /proc/<pid>/maps.
>>
>> Hi Kalesh,
>
> Hi Stephen,
>
> Thanks for taking a look.
>
>>
>> I think I see the value in the size field, but I'm curious about path,
>> which is available via readlink /proc/<pid>/fd/<n>, since those are
>> symlinks to the file themselves.
>
> This could work if we are root, but the file permissions wouldn't
> allow us to do the readlink on other processes otherwise. We want to
> be able to capture the system state in production environments from
> some trusted process with ptrace read capability.

Interesting, thanks for explaining. It seems weird to have a duplicate
interface for the same information but such is life.

>>
>> File paths can contain fun characters like newlines or colons, which
>> could make parsing out filenames in this text file... fun. How would your
>> userspace parsing logic handle "/home/stephen/filename\nsize:\t4096"? The
>> readlink(2) API makes that easy already.
>
> I think since we have escaped the "\n" (seq_file_path(m, file, "\n")),

I really should have read through that function before commenting,
thanks for teaching me something new :)

Stephen

> then user space might parse this line like:
>
> if (strncmp(line, "path:\t", 6) == 0)
>         char* path = line + 6;
>
>
> Thanks,
> Kalesh
>
>>
>> Is the goal avoiding races (e.g. file descriptor 3 is closed and reopened
>> to a different path between reading fdinfo and stating the fd)?
>>
>> Stephen
>>
>> > Signed-off-by: Kalesh Singh <kaleshsingh@google.com>
>> > ---
>> >
>> > Changes from rfc:
>> >   - Split adding 'size' and 'path' into a separate patches, per Christian
>> >   - Fix indentation (use tabs) in documentaion, per Randy
>> >
>> >  Documentation/filesystems/proc.rst | 14 ++++++++++++--
>> >  fs/proc/fd.c                       |  4 ++++
>> >  2 files changed, 16 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/Documentation/filesystems/proc.rst b/Documentation/filesystems/proc.rst
>> > index 779c05528e87..591f12d30d97 100644
>> > --- a/Documentation/filesystems/proc.rst
>> > +++ b/Documentation/filesystems/proc.rst
>> > @@ -1886,14 +1886,16 @@ if precise results are needed.
>> >  3.8  /proc/<pid>/fdinfo/<fd> - Information about opened file
>> >  ---------------------------------------------------------------
>> >  This file provides information associated with an opened file. The regular
>> > -files have at least five fields -- 'pos', 'flags', 'mnt_id', 'ino', and 'size'.
>> > +files have at least six fields -- 'pos', 'flags', 'mnt_id', 'ino', 'size',
>> > +and 'path'.
>> >
>> >  The 'pos' represents the current offset of the opened file in decimal
>> >  form [see lseek(2) for details], 'flags' denotes the octal O_xxx mask the
>> >  file has been created with [see open(2) for details] and 'mnt_id' represents
>> >  mount ID of the file system containing the opened file [see 3.5
>> >  /proc/<pid>/mountinfo for details]. 'ino' represents the inode number of
>> > -the file, and 'size' represents the size of the file in bytes.
>> > +the file, 'size' represents the size of the file in bytes, and 'path'
>> > +represents the file path.
>> >
>> >  A typical output is::
>> >
>> > @@ -1902,6 +1904,7 @@ A typical output is::
>> >       mnt_id: 19
>> >       ino:    63107
>> >       size:   0
>> > +     path:   /dev/null
>> >
>> >  All locks associated with a file descriptor are shown in its fdinfo too::
>> >
>> > @@ -1920,6 +1923,7 @@ Eventfd files
>> >       mnt_id: 9
>> >       ino:    63107
>> >       size:   0
>> > +     path:   anon_inode:[eventfd]
>> >       eventfd-count:  5a
>> >
>> >  where 'eventfd-count' is hex value of a counter.
>> > @@ -1934,6 +1938,7 @@ Signalfd files
>> >       mnt_id: 9
>> >       ino:    63107
>> >       size:   0
>> > +     path:   anon_inode:[signalfd]
>> >       sigmask:        0000000000000200
>> >
>> >  where 'sigmask' is hex value of the signal mask associated
>> > @@ -1949,6 +1954,7 @@ Epoll files
>> >       mnt_id: 9
>> >       ino:    63107
>> >       size:   0
>> > +     path:   anon_inode:[eventpoll]
>> >       tfd:        5 events:       1d data: ffffffffffffffff pos:0 ino:61af sdev:7
>> >
>> >  where 'tfd' is a target file descriptor number in decimal form,
>> > @@ -1968,6 +1974,7 @@ For inotify files the format is the following::
>> >       mnt_id: 9
>> >       ino:    63107
>> >       size:   0
>> > +     path:   anon_inode:inotify
>> >       inotify wd:3 ino:9e7e sdev:800013 mask:800afce ignored_mask:0 fhandle-bytes:8 fhandle-type:1 f_handle:7e9e0000640d1b6d
>> >
>> >  where 'wd' is a watch descriptor in decimal form, i.e. a target file
>> > @@ -1992,6 +1999,7 @@ For fanotify files the format is::
>> >       mnt_id: 9
>> >       ino:    63107
>> >       size:   0
>> > +     path:   anon_inode:[fanotify]
>> >       fanotify flags:10 event-flags:0
>> >       fanotify mnt_id:12 mflags:40 mask:38 ignored_mask:40000003
>> >       fanotify ino:4f969 sdev:800013 mflags:0 mask:3b ignored_mask:40000000 fhandle-bytes:8 fhandle-type:1 f_handle:69f90400c275b5b4
>> > @@ -2018,6 +2026,7 @@ Timerfd files
>> >       mnt_id: 9
>> >       ino:    63107
>> >       size:   0
>> > +     path:   anon_inode:[timerfd]
>> >       clockid: 0
>> >       ticks: 0
>> >       settime flags: 01
>> > @@ -2042,6 +2051,7 @@ DMA Buffer files
>> >       mnt_id: 9
>> >       ino:    63107
>> >       size:   32768
>> > +     path:   /dmabuf:
>> >       count:  2
>> >       exp_name:  system-heap
>> >
>> > diff --git a/fs/proc/fd.c b/fs/proc/fd.c
>> > index 464bc3f55759..8889a8ba09d4 100644
>> > --- a/fs/proc/fd.c
>> > +++ b/fs/proc/fd.c
>> > @@ -60,6 +60,10 @@ static int seq_show(struct seq_file *m, void *v)
>> >       seq_printf(m, "ino:\t%lu\n", file_inode(file)->i_ino);
>> >       seq_printf(m, "size:\t%lli\n", (long long)file_inode(file)->i_size);
>> >
>> > +     seq_puts(m, "path:\t");
>> > +     seq_file_path(m, file, "\n");
>> > +     seq_putc(m, '\n');
>> > +
>> >       /* show_fd_locks() never deferences files so a stale value is safe */
>> >       show_fd_locks(m, file, files);
>> >       if (seq_has_overflowed(m))
>>
>> --
>> To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com.
>>

Re: [PATCH 1/2] procfs: Add 'size' to /proc/<pid>/fdinfo/

[Christian König]

Am 31.05.22 um 23:25 schrieb Kalesh Singh:
> To be able to account the amount of memory a process is keeping pinned
> by open file descriptors add a 'size' field to fdinfo output.
>
> dmabufs fds already expose a 'size' field for this reason, remove this
> and make it a common field for all fds. This allows tracking of
> other types of memory (e.g. memfd and ashmem in Android).
>
> Signed-off-by: Kalesh Singh <kaleshsingh@google.com>

At least for the DMA-buf part feel free to add an Reviewed-by: Christian 
König <christian.koenig@amd.com> for this.

Regards,
Christian.

> ---
>
> Changes from rfc:
>    - Split adding 'size' and 'path' into a separate patches, per Christian
>    - Split fdinfo seq_printf into separate lines, per Christian
>    - Fix indentation (use tabs) in documentaion, per Randy
>
>   Documentation/filesystems/proc.rst | 12 ++++++++++--
>   drivers/dma-buf/dma-buf.c          |  1 -
>   fs/proc/fd.c                       |  9 +++++----
>   3 files changed, 15 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/filesystems/proc.rst b/Documentation/filesystems/proc.rst
> index 1bc91fb8c321..779c05528e87 100644
> --- a/Documentation/filesystems/proc.rst
> +++ b/Documentation/filesystems/proc.rst
> @@ -1886,13 +1886,14 @@ if precise results are needed.
>   3.8	/proc/<pid>/fdinfo/<fd> - Information about opened file
>   ---------------------------------------------------------------
>   This file provides information associated with an opened file. The regular
> -files have at least four fields -- 'pos', 'flags', 'mnt_id' and 'ino'.
> +files have at least five fields -- 'pos', 'flags', 'mnt_id', 'ino', and 'size'.
> +
>   The 'pos' represents the current offset of the opened file in decimal
>   form [see lseek(2) for details], 'flags' denotes the octal O_xxx mask the
>   file has been created with [see open(2) for details] and 'mnt_id' represents
>   mount ID of the file system containing the opened file [see 3.5
>   /proc/<pid>/mountinfo for details]. 'ino' represents the inode number of
> -the file.
> +the file, and 'size' represents the size of the file in bytes.
>   
>   A typical output is::
>   
> @@ -1900,6 +1901,7 @@ A typical output is::
>   	flags:	0100002
>   	mnt_id:	19
>   	ino:	63107
> +	size:	0
>   
>   All locks associated with a file descriptor are shown in its fdinfo too::
>   
> @@ -1917,6 +1919,7 @@ Eventfd files
>   	flags:	04002
>   	mnt_id:	9
>   	ino:	63107
> +	size:   0
>   	eventfd-count:	5a
>   
>   where 'eventfd-count' is hex value of a counter.
> @@ -1930,6 +1933,7 @@ Signalfd files
>   	flags:	04002
>   	mnt_id:	9
>   	ino:	63107
> +	size:   0
>   	sigmask:	0000000000000200
>   
>   where 'sigmask' is hex value of the signal mask associated
> @@ -1944,6 +1948,7 @@ Epoll files
>   	flags:	02
>   	mnt_id:	9
>   	ino:	63107
> +	size:   0
>   	tfd:        5 events:       1d data: ffffffffffffffff pos:0 ino:61af sdev:7
>   
>   where 'tfd' is a target file descriptor number in decimal form,
> @@ -1962,6 +1967,7 @@ For inotify files the format is the following::
>   	flags:	02000000
>   	mnt_id:	9
>   	ino:	63107
> +	size:   0
>   	inotify wd:3 ino:9e7e sdev:800013 mask:800afce ignored_mask:0 fhandle-bytes:8 fhandle-type:1 f_handle:7e9e0000640d1b6d
>   
>   where 'wd' is a watch descriptor in decimal form, i.e. a target file
> @@ -1985,6 +1991,7 @@ For fanotify files the format is::
>   	flags:	02
>   	mnt_id:	9
>   	ino:	63107
> +	size:   0
>   	fanotify flags:10 event-flags:0
>   	fanotify mnt_id:12 mflags:40 mask:38 ignored_mask:40000003
>   	fanotify ino:4f969 sdev:800013 mflags:0 mask:3b ignored_mask:40000000 fhandle-bytes:8 fhandle-type:1 f_handle:69f90400c275b5b4
> @@ -2010,6 +2017,7 @@ Timerfd files
>   	flags:	02
>   	mnt_id:	9
>   	ino:	63107
> +	size:   0
>   	clockid: 0
>   	ticks: 0
>   	settime flags: 01
> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> index 32f55640890c..5f2ae38c960f 100644
> --- a/drivers/dma-buf/dma-buf.c
> +++ b/drivers/dma-buf/dma-buf.c
> @@ -378,7 +378,6 @@ static void dma_buf_show_fdinfo(struct seq_file *m, struct file *file)
>   {
>   	struct dma_buf *dmabuf = file->private_data;
>   
> -	seq_printf(m, "size:\t%zu\n", dmabuf->size);
>   	/* Don't count the temporary reference taken inside procfs seq_show */
>   	seq_printf(m, "count:\t%ld\n", file_count(dmabuf->file) - 1);
>   	seq_printf(m, "exp_name:\t%s\n", dmabuf->exp_name);
> diff --git a/fs/proc/fd.c b/fs/proc/fd.c
> index 913bef0d2a36..464bc3f55759 100644
> --- a/fs/proc/fd.c
> +++ b/fs/proc/fd.c
> @@ -54,10 +54,11 @@ static int seq_show(struct seq_file *m, void *v)
>   	if (ret)
>   		return ret;
>   
> -	seq_printf(m, "pos:\t%lli\nflags:\t0%o\nmnt_id:\t%i\nino:\t%lu\n",
> -		   (long long)file->f_pos, f_flags,
> -		   real_mount(file->f_path.mnt)->mnt_id,
> -		   file_inode(file)->i_ino);
> +	seq_printf(m, "pos:\t%lli\n", (long long)file->f_pos);
> +	seq_printf(m, "flags:\t0%o\n", f_flags);
> +	seq_printf(m, "mnt_id:\t%i\n", real_mount(file->f_path.mnt)->mnt_id);
> +	seq_printf(m, "ino:\t%lu\n", file_inode(file)->i_ino);
> +	seq_printf(m, "size:\t%lli\n", (long long)file_inode(file)->i_size);
>   
>   	/* show_fd_locks() never deferences files so a stale value is safe */
>   	show_fd_locks(m, file, files);

Re: [PATCH 1/2] procfs: Add 'size' to /proc/<pid>/fdinfo/

[Kalesh Singh]

On Wed, Jun 1, 2022 at 6:55 AM Christian König <christian.koenig@amd.com> wrote:
>
> Am 31.05.22 um 23:25 schrieb Kalesh Singh:
> > To be able to account the amount of memory a process is keeping pinned
> > by open file descriptors add a 'size' field to fdinfo output.
> >
> > dmabufs fds already expose a 'size' field for this reason, remove this
> > and make it a common field for all fds. This allows tracking of
> > other types of memory (e.g. memfd and ashmem in Android).
> >
> > Signed-off-by: Kalesh Singh <kaleshsingh@google.com>
>
> At least for the DMA-buf part feel free to add an Reviewed-by: Christian
> König <christian.koenig@amd.com> for this.

Thanks for the review, Christian.

--Kalesh

>
> Regards,
> Christian.
>
> > ---
> >
> > Changes from rfc:
> >    - Split adding 'size' and 'path' into a separate patches, per Christian
> >    - Split fdinfo seq_printf into separate lines, per Christian
> >    - Fix indentation (use tabs) in documentaion, per Randy
> >
> >   Documentation/filesystems/proc.rst | 12 ++++++++++--
> >   drivers/dma-buf/dma-buf.c          |  1 -
> >   fs/proc/fd.c                       |  9 +++++----
> >   3 files changed, 15 insertions(+), 7 deletions(-)
> >
> > diff --git a/Documentation/filesystems/proc.rst b/Documentation/filesystems/proc.rst
> > index 1bc91fb8c321..779c05528e87 100644
> > --- a/Documentation/filesystems/proc.rst
> > +++ b/Documentation/filesystems/proc.rst
> > @@ -1886,13 +1886,14 @@ if precise results are needed.
> >   3.8 /proc/<pid>/fdinfo/<fd> - Information about opened file
> >   ---------------------------------------------------------------
> >   This file provides information associated with an opened file. The regular
> > -files have at least four fields -- 'pos', 'flags', 'mnt_id' and 'ino'.
> > +files have at least five fields -- 'pos', 'flags', 'mnt_id', 'ino', and 'size'.
> > +
> >   The 'pos' represents the current offset of the opened file in decimal
> >   form [see lseek(2) for details], 'flags' denotes the octal O_xxx mask the
> >   file has been created with [see open(2) for details] and 'mnt_id' represents
> >   mount ID of the file system containing the opened file [see 3.5
> >   /proc/<pid>/mountinfo for details]. 'ino' represents the inode number of
> > -the file.
> > +the file, and 'size' represents the size of the file in bytes.
> >
> >   A typical output is::
> >
> > @@ -1900,6 +1901,7 @@ A typical output is::
> >       flags:  0100002
> >       mnt_id: 19
> >       ino:    63107
> > +     size:   0
> >
> >   All locks associated with a file descriptor are shown in its fdinfo too::
> >
> > @@ -1917,6 +1919,7 @@ Eventfd files
> >       flags:  04002
> >       mnt_id: 9
> >       ino:    63107
> > +     size:   0
> >       eventfd-count:  5a
> >
> >   where 'eventfd-count' is hex value of a counter.
> > @@ -1930,6 +1933,7 @@ Signalfd files
> >       flags:  04002
> >       mnt_id: 9
> >       ino:    63107
> > +     size:   0
> >       sigmask:        0000000000000200
> >
> >   where 'sigmask' is hex value of the signal mask associated
> > @@ -1944,6 +1948,7 @@ Epoll files
> >       flags:  02
> >       mnt_id: 9
> >       ino:    63107
> > +     size:   0
> >       tfd:        5 events:       1d data: ffffffffffffffff pos:0 ino:61af sdev:7
> >
> >   where 'tfd' is a target file descriptor number in decimal form,
> > @@ -1962,6 +1967,7 @@ For inotify files the format is the following::
> >       flags:  02000000
> >       mnt_id: 9
> >       ino:    63107
> > +     size:   0
> >       inotify wd:3 ino:9e7e sdev:800013 mask:800afce ignored_mask:0 fhandle-bytes:8 fhandle-type:1 f_handle:7e9e0000640d1b6d
> >
> >   where 'wd' is a watch descriptor in decimal form, i.e. a target file
> > @@ -1985,6 +1991,7 @@ For fanotify files the format is::
> >       flags:  02
> >       mnt_id: 9
> >       ino:    63107
> > +     size:   0
> >       fanotify flags:10 event-flags:0
> >       fanotify mnt_id:12 mflags:40 mask:38 ignored_mask:40000003
> >       fanotify ino:4f969 sdev:800013 mflags:0 mask:3b ignored_mask:40000000 fhandle-bytes:8 fhandle-type:1 f_handle:69f90400c275b5b4
> > @@ -2010,6 +2017,7 @@ Timerfd files
> >       flags:  02
> >       mnt_id: 9
> >       ino:    63107
> > +     size:   0
> >       clockid: 0
> >       ticks: 0
> >       settime flags: 01
> > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> > index 32f55640890c..5f2ae38c960f 100644
> > --- a/drivers/dma-buf/dma-buf.c
> > +++ b/drivers/dma-buf/dma-buf.c
> > @@ -378,7 +378,6 @@ static void dma_buf_show_fdinfo(struct seq_file *m, struct file *file)
> >   {
> >       struct dma_buf *dmabuf = file->private_data;
> >
> > -     seq_printf(m, "size:\t%zu\n", dmabuf->size);
> >       /* Don't count the temporary reference taken inside procfs seq_show */
> >       seq_printf(m, "count:\t%ld\n", file_count(dmabuf->file) - 1);
> >       seq_printf(m, "exp_name:\t%s\n", dmabuf->exp_name);
> > diff --git a/fs/proc/fd.c b/fs/proc/fd.c
> > index 913bef0d2a36..464bc3f55759 100644
> > --- a/fs/proc/fd.c
> > +++ b/fs/proc/fd.c
> > @@ -54,10 +54,11 @@ static int seq_show(struct seq_file *m, void *v)
> >       if (ret)
> >               return ret;
> >
> > -     seq_printf(m, "pos:\t%lli\nflags:\t0%o\nmnt_id:\t%i\nino:\t%lu\n",
> > -                (long long)file->f_pos, f_flags,
> > -                real_mount(file->f_path.mnt)->mnt_id,
> > -                file_inode(file)->i_ino);
> > +     seq_printf(m, "pos:\t%lli\n", (long long)file->f_pos);
> > +     seq_printf(m, "flags:\t0%o\n", f_flags);
> > +     seq_printf(m, "mnt_id:\t%i\n", real_mount(file->f_path.mnt)->mnt_id);
> > +     seq_printf(m, "ino:\t%lu\n", file_inode(file)->i_ino);
> > +     seq_printf(m, "size:\t%lli\n", (long long)file_inode(file)->i_size);
> >
> >       /* show_fd_locks() never deferences files so a stale value is safe */
> >       show_fd_locks(m, file, files);
>

Re: [Linaro-mm-sig] Re: [PATCH 2/2] procfs: Add 'path' to /proc/<pid>/fdinfo/

[Christian König]

Am 01.06.22 um 00:48 schrieb Stephen Brennan:
> Kalesh Singh <kaleshsingh@google.com> writes:
>> On Tue, May 31, 2022 at 3:07 PM Stephen Brennan
>> <stephen.s.brennan@oracle.com> wrote:
>>> On 5/31/22 14:25, Kalesh Singh wrote:
>>>> In order to identify the type of memory a process has pinned through
>>>> its open fds, add the file path to fdinfo output. This allows
>>>> identifying memory types based on common prefixes. e.g. "/memfd...",
>>>> "/dmabuf...", "/dev/ashmem...".
>>>>
>>>> Access to /proc/<pid>/fdinfo is governed by PTRACE_MODE_READ_FSCREDS
>>>> the same as /proc/<pid>/maps which also exposes the file path of
>>>> mappings; so the security permissions for accessing path is consistent
>>>> with that of /proc/<pid>/maps.
>>> Hi Kalesh,
>> Hi Stephen,
>>
>> Thanks for taking a look.
>>
>>> I think I see the value in the size field, but I'm curious about path,
>>> which is available via readlink /proc/<pid>/fd/<n>, since those are
>>> symlinks to the file themselves.
>> This could work if we are root, but the file permissions wouldn't
>> allow us to do the readlink on other processes otherwise. We want to
>> be able to capture the system state in production environments from
>> some trusted process with ptrace read capability.
> Interesting, thanks for explaining. It seems weird to have a duplicate
> interface for the same information but such is life.

Yeah, the size change is really straight forward but for this one I'm 
not 100% sure either.

Probably best to ping some core fs developer before going further with it.

BTW: Any preferred branch to push this upstream? If not I can take it 
through drm-misc-next.

Regards,
Christian.

>
>>> File paths can contain fun characters like newlines or colons, which
>>> could make parsing out filenames in this text file... fun. How would your
>>> userspace parsing logic handle "/home/stephen/filename\nsize:\t4096"? The
>>> readlink(2) API makes that easy already.
>> I think since we have escaped the "\n" (seq_file_path(m, file, "\n")),
> I really should have read through that function before commenting,
> thanks for teaching me something new :)
>
> Stephen
>
>> then user space might parse this line like:
>>
>> if (strncmp(line, "path:\t", 6) == 0)
>>          char* path = line + 6;
>>
>>
>> Thanks,
>> Kalesh
>>
>>> Is the goal avoiding races (e.g. file descriptor 3 is closed and reopened
>>> to a different path between reading fdinfo and stating the fd)?
>>>
>>> Stephen
>>>
>>>> Signed-off-by: Kalesh Singh <kaleshsingh@google.com>
>>>> ---
>>>>
>>>> Changes from rfc:
>>>>    - Split adding 'size' and 'path' into a separate patches, per Christian
>>>>    - Fix indentation (use tabs) in documentaion, per Randy
>>>>
>>>>   Documentation/filesystems/proc.rst | 14 ++++++++++++--
>>>>   fs/proc/fd.c                       |  4 ++++
>>>>   2 files changed, 16 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/Documentation/filesystems/proc.rst b/Documentation/filesystems/proc.rst
>>>> index 779c05528e87..591f12d30d97 100644
>>>> --- a/Documentation/filesystems/proc.rst
>>>> +++ b/Documentation/filesystems/proc.rst
>>>> @@ -1886,14 +1886,16 @@ if precise results are needed.
>>>>   3.8  /proc/<pid>/fdinfo/<fd> - Information about opened file
>>>>   ---------------------------------------------------------------
>>>>   This file provides information associated with an opened file. The regular
>>>> -files have at least five fields -- 'pos', 'flags', 'mnt_id', 'ino', and 'size'.
>>>> +files have at least six fields -- 'pos', 'flags', 'mnt_id', 'ino', 'size',
>>>> +and 'path'.
>>>>
>>>>   The 'pos' represents the current offset of the opened file in decimal
>>>>   form [see lseek(2) for details], 'flags' denotes the octal O_xxx mask the
>>>>   file has been created with [see open(2) for details] and 'mnt_id' represents
>>>>   mount ID of the file system containing the opened file [see 3.5
>>>>   /proc/<pid>/mountinfo for details]. 'ino' represents the inode number of
>>>> -the file, and 'size' represents the size of the file in bytes.
>>>> +the file, 'size' represents the size of the file in bytes, and 'path'
>>>> +represents the file path.
>>>>
>>>>   A typical output is::
>>>>
>>>> @@ -1902,6 +1904,7 @@ A typical output is::
>>>>        mnt_id: 19
>>>>        ino:    63107
>>>>        size:   0
>>>> +     path:   /dev/null
>>>>
>>>>   All locks associated with a file descriptor are shown in its fdinfo too::
>>>>
>>>> @@ -1920,6 +1923,7 @@ Eventfd files
>>>>        mnt_id: 9
>>>>        ino:    63107
>>>>        size:   0
>>>> +     path:   anon_inode:[eventfd]
>>>>        eventfd-count:  5a
>>>>
>>>>   where 'eventfd-count' is hex value of a counter.
>>>> @@ -1934,6 +1938,7 @@ Signalfd files
>>>>        mnt_id: 9
>>>>        ino:    63107
>>>>        size:   0
>>>> +     path:   anon_inode:[signalfd]
>>>>        sigmask:        0000000000000200
>>>>
>>>>   where 'sigmask' is hex value of the signal mask associated
>>>> @@ -1949,6 +1954,7 @@ Epoll files
>>>>        mnt_id: 9
>>>>        ino:    63107
>>>>        size:   0
>>>> +     path:   anon_inode:[eventpoll]
>>>>        tfd:        5 events:       1d data: ffffffffffffffff pos:0 ino:61af sdev:7
>>>>
>>>>   where 'tfd' is a target file descriptor number in decimal form,
>>>> @@ -1968,6 +1974,7 @@ For inotify files the format is the following::
>>>>        mnt_id: 9
>>>>        ino:    63107
>>>>        size:   0
>>>> +     path:   anon_inode:inotify
>>>>        inotify wd:3 ino:9e7e sdev:800013 mask:800afce ignored_mask:0 fhandle-bytes:8 fhandle-type:1 f_handle:7e9e0000640d1b6d
>>>>
>>>>   where 'wd' is a watch descriptor in decimal form, i.e. a target file
>>>> @@ -1992,6 +1999,7 @@ For fanotify files the format is::
>>>>        mnt_id: 9
>>>>        ino:    63107
>>>>        size:   0
>>>> +     path:   anon_inode:[fanotify]
>>>>        fanotify flags:10 event-flags:0
>>>>        fanotify mnt_id:12 mflags:40 mask:38 ignored_mask:40000003
>>>>        fanotify ino:4f969 sdev:800013 mflags:0 mask:3b ignored_mask:40000000 fhandle-bytes:8 fhandle-type:1 f_handle:69f90400c275b5b4
>>>> @@ -2018,6 +2026,7 @@ Timerfd files
>>>>        mnt_id: 9
>>>>        ino:    63107
>>>>        size:   0
>>>> +     path:   anon_inode:[timerfd]
>>>>        clockid: 0
>>>>        ticks: 0
>>>>        settime flags: 01
>>>> @@ -2042,6 +2051,7 @@ DMA Buffer files
>>>>        mnt_id: 9
>>>>        ino:    63107
>>>>        size:   32768
>>>> +     path:   /dmabuf:
>>>>        count:  2
>>>>        exp_name:  system-heap
>>>>
>>>> diff --git a/fs/proc/fd.c b/fs/proc/fd.c
>>>> index 464bc3f55759..8889a8ba09d4 100644
>>>> --- a/fs/proc/fd.c
>>>> +++ b/fs/proc/fd.c
>>>> @@ -60,6 +60,10 @@ static int seq_show(struct seq_file *m, void *v)
>>>>        seq_printf(m, "ino:\t%lu\n", file_inode(file)->i_ino);
>>>>        seq_printf(m, "size:\t%lli\n", (long long)file_inode(file)->i_size);
>>>>
>>>> +     seq_puts(m, "path:\t");
>>>> +     seq_file_path(m, file, "\n");
>>>> +     seq_putc(m, '\n');
>>>> +
>>>>        /* show_fd_locks() never deferences files so a stale value is safe */
>>>>        show_fd_locks(m, file, files);
>>>>        if (seq_has_overflowed(m))
>>> --
>>> To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com.
>>>
> _______________________________________________
> Linaro-mm-sig mailing list -- linaro-mm-sig@lists.linaro.org
> To unsubscribe send an email to linaro-mm-sig-leave@lists.linaro.org

RE: [PATCH 2/2] procfs: Add 'path' to /proc/<pid>/fdinfo/

[David Laight]

From: Kalesh Singh
> Sent: 31 May 2022 23:30
...
> > File paths can contain fun characters like newlines or colons, which
> > could make parsing out filenames in this text file... fun. How would your
> > userspace parsing logic handle "/home/stephen/filename\nsize:\t4096"? The
> > readlink(2) API makes that easy already.
> 
> I think since we have escaped the "\n" (seq_file_path(m, file, "\n")),
> then user space might parse this line like:
> 
> if (strncmp(line, "path:\t", 6) == 0)
>         char* path = line + 6;

The real annoyance is other things doing scans of the filesystem
that accidentally 'bump into' strange names.

While anything serious probably gets it right how many times
Do you run 'find' to quickly search for something?

Spaces in filenames (popularised by some other os) are a PITA.
Not to mention leading and trailing spaces!
Anyone using filenames that only contain spaces does need shooting.

Deliberately adding non-printables isn't really a good idea.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [Linaro-mm-sig] Re: [PATCH 2/2] procfs: Add 'path' to /proc/<pid>/fdinfo/

[Kalesh Singh]

On Wed, Jun 1, 2022 at 8:02 AM Christian König
<ckoenig.leichtzumerken@gmail.com> wrote:
>
> Am 01.06.22 um 00:48 schrieb Stephen Brennan:
> > Kalesh Singh <kaleshsingh@google.com> writes:
> >> On Tue, May 31, 2022 at 3:07 PM Stephen Brennan
> >> <stephen.s.brennan@oracle.com> wrote:
> >>> On 5/31/22 14:25, Kalesh Singh wrote:
> >>>> In order to identify the type of memory a process has pinned through
> >>>> its open fds, add the file path to fdinfo output. This allows
> >>>> identifying memory types based on common prefixes. e.g. "/memfd...",
> >>>> "/dmabuf...", "/dev/ashmem...".
> >>>>
> >>>> Access to /proc/<pid>/fdinfo is governed by PTRACE_MODE_READ_FSCREDS
> >>>> the same as /proc/<pid>/maps which also exposes the file path of
> >>>> mappings; so the security permissions for accessing path is consistent
> >>>> with that of /proc/<pid>/maps.
> >>> Hi Kalesh,
> >> Hi Stephen,
> >>
> >> Thanks for taking a look.
> >>
> >>> I think I see the value in the size field, but I'm curious about path,
> >>> which is available via readlink /proc/<pid>/fd/<n>, since those are
> >>> symlinks to the file themselves.
> >> This could work if we are root, but the file permissions wouldn't
> >> allow us to do the readlink on other processes otherwise. We want to
> >> be able to capture the system state in production environments from
> >> some trusted process with ptrace read capability.
> > Interesting, thanks for explaining. It seems weird to have a duplicate
> > interface for the same information but such is life.
>
> Yeah, the size change is really straight forward but for this one I'm
> not 100% sure either.

The 2 concerns I think are:
  1. Fun characters in the path names
  2. If exposing the path is appropriate to begin with.

One way I think we can address both is to only expose the path for
anon inodes. Then we have well-known path formats and we don't expose
much about which files a process is accessing since these aren't real
paths.

+       if (is_anon_inode(inode)) {
+               seq_puts(m, "path:\t");
+               seq_file_path(m, file, "\n");
+               seq_putc(m, '\n');
+       }

Interested to hear thoughts on it.

>
> Probably best to ping some core fs developer before going further with it.

linux-fsdevel is cc'd here. Adding Al Vrio as well. Please let me know
if there are other parties I should include.

>
> BTW: Any preferred branch to push this upstream? If not I can take it
> through drm-misc-next.

No other dependencies for this, so drm-misc-next is good.

Thanks,
Kalesh

>
> Regards,
> Christian.
>
> >
> >>> File paths can contain fun characters like newlines or colons, which
> >>> could make parsing out filenames in this text file... fun. How would your
> >>> userspace parsing logic handle "/home/stephen/filename\nsize:\t4096"? The
> >>> readlink(2) API makes that easy already.
> >> I think since we have escaped the "\n" (seq_file_path(m, file, "\n")),
> > I really should have read through that function before commenting,
> > thanks for teaching me something new :)
> >
> > Stephen
> >
> >> then user space might parse this line like:
> >>
> >> if (strncmp(line, "path:\t", 6) == 0)
> >>          char* path = line + 6;
> >>
> >>
> >> Thanks,
> >> Kalesh
> >>
> >>> Is the goal avoiding races (e.g. file descriptor 3 is closed and reopened
> >>> to a different path between reading fdinfo and stating the fd)?
> >>>
> >>> Stephen
> >>>
> >>>> Signed-off-by: Kalesh Singh <kaleshsingh@google.com>
> >>>> ---
> >>>>
> >>>> Changes from rfc:
> >>>>    - Split adding 'size' and 'path' into a separate patches, per Christian
> >>>>    - Fix indentation (use tabs) in documentaion, per Randy
> >>>>
> >>>>   Documentation/filesystems/proc.rst | 14 ++++++++++++--
> >>>>   fs/proc/fd.c                       |  4 ++++
> >>>>   2 files changed, 16 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/Documentation/filesystems/proc.rst b/Documentation/filesystems/proc.rst
> >>>> index 779c05528e87..591f12d30d97 100644
> >>>> --- a/Documentation/filesystems/proc.rst
> >>>> +++ b/Documentation/filesystems/proc.rst
> >>>> @@ -1886,14 +1886,16 @@ if precise results are needed.
> >>>>   3.8  /proc/<pid>/fdinfo/<fd> - Information about opened file
> >>>>   ---------------------------------------------------------------
> >>>>   This file provides information associated with an opened file. The regular
> >>>> -files have at least five fields -- 'pos', 'flags', 'mnt_id', 'ino', and 'size'.
> >>>> +files have at least six fields -- 'pos', 'flags', 'mnt_id', 'ino', 'size',
> >>>> +and 'path'.
> >>>>
> >>>>   The 'pos' represents the current offset of the opened file in decimal
> >>>>   form [see lseek(2) for details], 'flags' denotes the octal O_xxx mask the
> >>>>   file has been created with [see open(2) for details] and 'mnt_id' represents
> >>>>   mount ID of the file system containing the opened file [see 3.5
> >>>>   /proc/<pid>/mountinfo for details]. 'ino' represents the inode number of
> >>>> -the file, and 'size' represents the size of the file in bytes.
> >>>> +the file, 'size' represents the size of the file in bytes, and 'path'
> >>>> +represents the file path.
> >>>>
> >>>>   A typical output is::
> >>>>
> >>>> @@ -1902,6 +1904,7 @@ A typical output is::
> >>>>        mnt_id: 19
> >>>>        ino:    63107
> >>>>        size:   0
> >>>> +     path:   /dev/null
> >>>>
> >>>>   All locks associated with a file descriptor are shown in its fdinfo too::
> >>>>
> >>>> @@ -1920,6 +1923,7 @@ Eventfd files
> >>>>        mnt_id: 9
> >>>>        ino:    63107
> >>>>        size:   0
> >>>> +     path:   anon_inode:[eventfd]
> >>>>        eventfd-count:  5a
> >>>>
> >>>>   where 'eventfd-count' is hex value of a counter.
> >>>> @@ -1934,6 +1938,7 @@ Signalfd files
> >>>>        mnt_id: 9
> >>>>        ino:    63107
> >>>>        size:   0
> >>>> +     path:   anon_inode:[signalfd]
> >>>>        sigmask:        0000000000000200
> >>>>
> >>>>   where 'sigmask' is hex value of the signal mask associated
> >>>> @@ -1949,6 +1954,7 @@ Epoll files
> >>>>        mnt_id: 9
> >>>>        ino:    63107
> >>>>        size:   0
> >>>> +     path:   anon_inode:[eventpoll]
> >>>>        tfd:        5 events:       1d data: ffffffffffffffff pos:0 ino:61af sdev:7
> >>>>
> >>>>   where 'tfd' is a target file descriptor number in decimal form,
> >>>> @@ -1968,6 +1974,7 @@ For inotify files the format is the following::
> >>>>        mnt_id: 9
> >>>>        ino:    63107
> >>>>        size:   0
> >>>> +     path:   anon_inode:inotify
> >>>>        inotify wd:3 ino:9e7e sdev:800013 mask:800afce ignored_mask:0 fhandle-bytes:8 fhandle-type:1 f_handle:7e9e0000640d1b6d
> >>>>
> >>>>   where 'wd' is a watch descriptor in decimal form, i.e. a target file
> >>>> @@ -1992,6 +1999,7 @@ For fanotify files the format is::
> >>>>        mnt_id: 9
> >>>>        ino:    63107
> >>>>        size:   0
> >>>> +     path:   anon_inode:[fanotify]
> >>>>        fanotify flags:10 event-flags:0
> >>>>        fanotify mnt_id:12 mflags:40 mask:38 ignored_mask:40000003
> >>>>        fanotify ino:4f969 sdev:800013 mflags:0 mask:3b ignored_mask:40000000 fhandle-bytes:8 fhandle-type:1 f_handle:69f90400c275b5b4
> >>>> @@ -2018,6 +2026,7 @@ Timerfd files
> >>>>        mnt_id: 9
> >>>>        ino:    63107
> >>>>        size:   0
> >>>> +     path:   anon_inode:[timerfd]
> >>>>        clockid: 0
> >>>>        ticks: 0
> >>>>        settime flags: 01
> >>>> @@ -2042,6 +2051,7 @@ DMA Buffer files
> >>>>        mnt_id: 9
> >>>>        ino:    63107
> >>>>        size:   32768
> >>>> +     path:   /dmabuf:
> >>>>        count:  2
> >>>>        exp_name:  system-heap
> >>>>
> >>>> diff --git a/fs/proc/fd.c b/fs/proc/fd.c
> >>>> index 464bc3f55759..8889a8ba09d4 100644
> >>>> --- a/fs/proc/fd.c
> >>>> +++ b/fs/proc/fd.c
> >>>> @@ -60,6 +60,10 @@ static int seq_show(struct seq_file *m, void *v)
> >>>>        seq_printf(m, "ino:\t%lu\n", file_inode(file)->i_ino);
> >>>>        seq_printf(m, "size:\t%lli\n", (long long)file_inode(file)->i_size);
> >>>>
> >>>> +     seq_puts(m, "path:\t");
> >>>> +     seq_file_path(m, file, "\n");
> >>>> +     seq_putc(m, '\n');
> >>>> +
> >>>>        /* show_fd_locks() never deferences files so a stale value is safe */
> >>>>        show_fd_locks(m, file, files);
> >>>>        if (seq_has_overflowed(m))
> >>> --
> >>> To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com.
> >>>
> > _______________________________________________
> > Linaro-mm-sig mailing list -- linaro-mm-sig@lists.linaro.org
> > To unsubscribe send an email to linaro-mm-sig-leave@lists.linaro.org
>

Re: [Linaro-mm-sig] Re: [PATCH 2/2] procfs: Add 'path' to /proc/<pid>/fdinfo/

[Kalesh Singh]

On Wed, Jun 1, 2022 at 8:31 PM Kalesh Singh <kaleshsingh@google.com> wrote:
>
> On Wed, Jun 1, 2022 at 8:02 AM Christian König
> <ckoenig.leichtzumerken@gmail.com> wrote:
> >
> > Am 01.06.22 um 00:48 schrieb Stephen Brennan:
> > > Kalesh Singh <kaleshsingh@google.com> writes:
> > >> On Tue, May 31, 2022 at 3:07 PM Stephen Brennan
> > >> <stephen.s.brennan@oracle.com> wrote:
> > >>> On 5/31/22 14:25, Kalesh Singh wrote:
> > >>>> In order to identify the type of memory a process has pinned through
> > >>>> its open fds, add the file path to fdinfo output. This allows
> > >>>> identifying memory types based on common prefixes. e.g. "/memfd...",
> > >>>> "/dmabuf...", "/dev/ashmem...".
> > >>>>
> > >>>> Access to /proc/<pid>/fdinfo is governed by PTRACE_MODE_READ_FSCREDS
> > >>>> the same as /proc/<pid>/maps which also exposes the file path of
> > >>>> mappings; so the security permissions for accessing path is consistent
> > >>>> with that of /proc/<pid>/maps.
> > >>> Hi Kalesh,
> > >> Hi Stephen,
> > >>
> > >> Thanks for taking a look.
> > >>
> > >>> I think I see the value in the size field, but I'm curious about path,
> > >>> which is available via readlink /proc/<pid>/fd/<n>, since those are
> > >>> symlinks to the file themselves.
> > >> This could work if we are root, but the file permissions wouldn't
> > >> allow us to do the readlink on other processes otherwise. We want to
> > >> be able to capture the system state in production environments from
> > >> some trusted process with ptrace read capability.
> > > Interesting, thanks for explaining. It seems weird to have a duplicate
> > > interface for the same information but such is life.
> >
> > Yeah, the size change is really straight forward but for this one I'm
> > not 100% sure either.
>
> The 2 concerns I think are:
>   1. Fun characters in the path names
>   2. If exposing the path is appropriate to begin with.
>
> One way I think we can address both is to only expose the path for
> anon inodes. Then we have well-known path formats and we don't expose
> much about which files a process is accessing since these aren't real
> paths.
>
> +       if (is_anon_inode(inode)) {
> +               seq_puts(m, "path:\t");
> +               seq_file_path(m, file, "\n");
> +               seq_putc(m, '\n');
> +       }
>
> Interested to hear thoughts on it.

Adding Christoph,

To be able to identify types of shared memory processes pin through
FDs in production builds, we would like to add a 'path' field to
fdinfo of anon inodes. We could then use the common prefixes
("/dmabuf", "/memfd", ...) to identify different types.

Would appreciate any feedback from the FS perspective.

Thanks,
Kalesh

>
> >
> > Probably best to ping some core fs developer before going further with it.
>
> linux-fsdevel is cc'd here. Adding Al Vrio as well. Please let me know
> if there are other parties I should include.
>
> >
> > BTW: Any preferred branch to push this upstream? If not I can take it
> > through drm-misc-next.
>
> No other dependencies for this, so drm-misc-next is good.
>
> Thanks,
> Kalesh
>
> >
> > Regards,
> > Christian.
> >
> > >
> > >>> File paths can contain fun characters like newlines or colons, which
> > >>> could make parsing out filenames in this text file... fun. How would your
> > >>> userspace parsing logic handle "/home/stephen/filename\nsize:\t4096"? The
> > >>> readlink(2) API makes that easy already.
> > >> I think since we have escaped the "\n" (seq_file_path(m, file, "\n")),
> > > I really should have read through that function before commenting,
> > > thanks for teaching me something new :)
> > >
> > > Stephen
> > >
> > >> then user space might parse this line like:
> > >>
> > >> if (strncmp(line, "path:\t", 6) == 0)
> > >>          char* path = line + 6;
> > >>
> > >>
> > >> Thanks,
> > >> Kalesh
> > >>
> > >>> Is the goal avoiding races (e.g. file descriptor 3 is closed and reopened
> > >>> to a different path between reading fdinfo and stating the fd)?
> > >>>
> > >>> Stephen
> > >>>
> > >>>> Signed-off-by: Kalesh Singh <kaleshsingh@google.com>
> > >>>> ---
> > >>>>
> > >>>> Changes from rfc:
> > >>>>    - Split adding 'size' and 'path' into a separate patches, per Christian
> > >>>>    - Fix indentation (use tabs) in documentaion, per Randy
> > >>>>
> > >>>>   Documentation/filesystems/proc.rst | 14 ++++++++++++--
> > >>>>   fs/proc/fd.c                       |  4 ++++
> > >>>>   2 files changed, 16 insertions(+), 2 deletions(-)
> > >>>>
> > >>>> diff --git a/Documentation/filesystems/proc.rst b/Documentation/filesystems/proc.rst
> > >>>> index 779c05528e87..591f12d30d97 100644
> > >>>> --- a/Documentation/filesystems/proc.rst
> > >>>> +++ b/Documentation/filesystems/proc.rst
> > >>>> @@ -1886,14 +1886,16 @@ if precise results are needed.
> > >>>>   3.8  /proc/<pid>/fdinfo/<fd> - Information about opened file
> > >>>>   ---------------------------------------------------------------
> > >>>>   This file provides information associated with an opened file. The regular
> > >>>> -files have at least five fields -- 'pos', 'flags', 'mnt_id', 'ino', and 'size'.
> > >>>> +files have at least six fields -- 'pos', 'flags', 'mnt_id', 'ino', 'size',
> > >>>> +and 'path'.
> > >>>>
> > >>>>   The 'pos' represents the current offset of the opened file in decimal
> > >>>>   form [see lseek(2) for details], 'flags' denotes the octal O_xxx mask the
> > >>>>   file has been created with [see open(2) for details] and 'mnt_id' represents
> > >>>>   mount ID of the file system containing the opened file [see 3.5
> > >>>>   /proc/<pid>/mountinfo for details]. 'ino' represents the inode number of
> > >>>> -the file, and 'size' represents the size of the file in bytes.
> > >>>> +the file, 'size' represents the size of the file in bytes, and 'path'
> > >>>> +represents the file path.
> > >>>>
> > >>>>   A typical output is::
> > >>>>
> > >>>> @@ -1902,6 +1904,7 @@ A typical output is::
> > >>>>        mnt_id: 19
> > >>>>        ino:    63107
> > >>>>        size:   0
> > >>>> +     path:   /dev/null
> > >>>>
> > >>>>   All locks associated with a file descriptor are shown in its fdinfo too::
> > >>>>
> > >>>> @@ -1920,6 +1923,7 @@ Eventfd files
> > >>>>        mnt_id: 9
> > >>>>        ino:    63107
> > >>>>        size:   0
> > >>>> +     path:   anon_inode:[eventfd]
> > >>>>        eventfd-count:  5a
> > >>>>
> > >>>>   where 'eventfd-count' is hex value of a counter.
> > >>>> @@ -1934,6 +1938,7 @@ Signalfd files
> > >>>>        mnt_id: 9
> > >>>>        ino:    63107
> > >>>>        size:   0
> > >>>> +     path:   anon_inode:[signalfd]
> > >>>>        sigmask:        0000000000000200
> > >>>>
> > >>>>   where 'sigmask' is hex value of the signal mask associated
> > >>>> @@ -1949,6 +1954,7 @@ Epoll files
> > >>>>        mnt_id: 9
> > >>>>        ino:    63107
> > >>>>        size:   0
> > >>>> +     path:   anon_inode:[eventpoll]
> > >>>>        tfd:        5 events:       1d data: ffffffffffffffff pos:0 ino:61af sdev:7
> > >>>>
> > >>>>   where 'tfd' is a target file descriptor number in decimal form,
> > >>>> @@ -1968,6 +1974,7 @@ For inotify files the format is the following::
> > >>>>        mnt_id: 9
> > >>>>        ino:    63107
> > >>>>        size:   0
> > >>>> +     path:   anon_inode:inotify
> > >>>>        inotify wd:3 ino:9e7e sdev:800013 mask:800afce ignored_mask:0 fhandle-bytes:8 fhandle-type:1 f_handle:7e9e0000640d1b6d
> > >>>>
> > >>>>   where 'wd' is a watch descriptor in decimal form, i.e. a target file
> > >>>> @@ -1992,6 +1999,7 @@ For fanotify files the format is::
> > >>>>        mnt_id: 9
> > >>>>        ino:    63107
> > >>>>        size:   0
> > >>>> +     path:   anon_inode:[fanotify]
> > >>>>        fanotify flags:10 event-flags:0
> > >>>>        fanotify mnt_id:12 mflags:40 mask:38 ignored_mask:40000003
> > >>>>        fanotify ino:4f969 sdev:800013 mflags:0 mask:3b ignored_mask:40000000 fhandle-bytes:8 fhandle-type:1 f_handle:69f90400c275b5b4
> > >>>> @@ -2018,6 +2026,7 @@ Timerfd files
> > >>>>        mnt_id: 9
> > >>>>        ino:    63107
> > >>>>        size:   0
> > >>>> +     path:   anon_inode:[timerfd]
> > >>>>        clockid: 0
> > >>>>        ticks: 0
> > >>>>        settime flags: 01
> > >>>> @@ -2042,6 +2051,7 @@ DMA Buffer files
> > >>>>        mnt_id: 9
> > >>>>        ino:    63107
> > >>>>        size:   32768
> > >>>> +     path:   /dmabuf:
> > >>>>        count:  2
> > >>>>        exp_name:  system-heap
> > >>>>
> > >>>> diff --git a/fs/proc/fd.c b/fs/proc/fd.c
> > >>>> index 464bc3f55759..8889a8ba09d4 100644
> > >>>> --- a/fs/proc/fd.c
> > >>>> +++ b/fs/proc/fd.c
> > >>>> @@ -60,6 +60,10 @@ static int seq_show(struct seq_file *m, void *v)
> > >>>>        seq_printf(m, "ino:\t%lu\n", file_inode(file)->i_ino);
> > >>>>        seq_printf(m, "size:\t%lli\n", (long long)file_inode(file)->i_size);
> > >>>>
> > >>>> +     seq_puts(m, "path:\t");
> > >>>> +     seq_file_path(m, file, "\n");
> > >>>> +     seq_putc(m, '\n');
> > >>>> +
> > >>>>        /* show_fd_locks() never deferences files so a stale value is safe */
> > >>>>        show_fd_locks(m, file, files);
> > >>>>        if (seq_has_overflowed(m))
> > >>> --
> > >>> To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com.
> > >>>
> > > _______________________________________________
> > > Linaro-mm-sig mailing list -- linaro-mm-sig@lists.linaro.org
> > > To unsubscribe send an email to linaro-mm-sig-leave@lists.linaro.org
> >

Re: [Linaro-mm-sig] Re: [PATCH 2/2] procfs: Add 'path' to /proc/<pid>/fdinfo/

[Kalesh Singh]

On Wed, Jun 15, 2022 at 10:00 AM Kalesh Singh <kaleshsingh@google.com> wrote:
>
> On Wed, Jun 1, 2022 at 8:31 PM Kalesh Singh <kaleshsingh@google.com> wrote:
> >
> > On Wed, Jun 1, 2022 at 8:02 AM Christian König
> > <ckoenig.leichtzumerken@gmail.com> wrote:
> > >
> > > Am 01.06.22 um 00:48 schrieb Stephen Brennan:
> > > > Kalesh Singh <kaleshsingh@google.com> writes:
> > > >> On Tue, May 31, 2022 at 3:07 PM Stephen Brennan
> > > >> <stephen.s.brennan@oracle.com> wrote:
> > > >>> On 5/31/22 14:25, Kalesh Singh wrote:
> > > >>>> In order to identify the type of memory a process has pinned through
> > > >>>> its open fds, add the file path to fdinfo output. This allows
> > > >>>> identifying memory types based on common prefixes. e.g. "/memfd...",
> > > >>>> "/dmabuf...", "/dev/ashmem...".
> > > >>>>
> > > >>>> Access to /proc/<pid>/fdinfo is governed by PTRACE_MODE_READ_FSCREDS
> > > >>>> the same as /proc/<pid>/maps which also exposes the file path of
> > > >>>> mappings; so the security permissions for accessing path is consistent
> > > >>>> with that of /proc/<pid>/maps.
> > > >>> Hi Kalesh,
> > > >> Hi Stephen,
> > > >>
> > > >> Thanks for taking a look.
> > > >>
> > > >>> I think I see the value in the size field, but I'm curious about path,
> > > >>> which is available via readlink /proc/<pid>/fd/<n>, since those are
> > > >>> symlinks to the file themselves.
> > > >> This could work if we are root, but the file permissions wouldn't
> > > >> allow us to do the readlink on other processes otherwise. We want to
> > > >> be able to capture the system state in production environments from
> > > >> some trusted process with ptrace read capability.
> > > > Interesting, thanks for explaining. It seems weird to have a duplicate
> > > > interface for the same information but such is life.
> > >
> > > Yeah, the size change is really straight forward but for this one I'm
> > > not 100% sure either.
> >
> > The 2 concerns I think are:
> >   1. Fun characters in the path names
> >   2. If exposing the path is appropriate to begin with.
> >
> > One way I think we can address both is to only expose the path for
> > anon inodes. Then we have well-known path formats and we don't expose
> > much about which files a process is accessing since these aren't real
> > paths.
> >
> > +       if (is_anon_inode(inode)) {
> > +               seq_puts(m, "path:\t");
> > +               seq_file_path(m, file, "\n");
> > +               seq_putc(m, '\n');
> > +       }
> >
> > Interested to hear thoughts on it.
>
> Adding Christoph,
>
> To be able to identify types of shared memory processes pin through
> FDs in production builds, we would like to add a 'path' field to
> fdinfo of anon inodes. We could then use the common prefixes
> ("/dmabuf", "/memfd", ...) to identify different types.
>
> Would appreciate any feedback from the FS perspective.

Hi all,

If there are no objections to this, then I plan to respin the patch
for just anonymous inodes. Please let me know if there are further
concerns.

Thanks,
Kalesh

>
> Thanks,
> Kalesh
>
> >
> > >
> > > Probably best to ping some core fs developer before going further with it.
> >
> > linux-fsdevel is cc'd here. Adding Al Vrio as well. Please let me know
> > if there are other parties I should include.
> >
> > >
> > > BTW: Any preferred branch to push this upstream? If not I can take it
> > > through drm-misc-next.
> >
> > No other dependencies for this, so drm-misc-next is good.
> >
> > Thanks,
> > Kalesh
> >
> > >
> > > Regards,
> > > Christian.
> > >
> > > >
> > > >>> File paths can contain fun characters like newlines or colons, which
> > > >>> could make parsing out filenames in this text file... fun. How would your
> > > >>> userspace parsing logic handle "/home/stephen/filename\nsize:\t4096"? The
> > > >>> readlink(2) API makes that easy already.
> > > >> I think since we have escaped the "\n" (seq_file_path(m, file, "\n")),
> > > > I really should have read through that function before commenting,
> > > > thanks for teaching me something new :)
> > > >
> > > > Stephen
> > > >
> > > >> then user space might parse this line like:
> > > >>
> > > >> if (strncmp(line, "path:\t", 6) == 0)
> > > >>          char* path = line + 6;
> > > >>
> > > >>
> > > >> Thanks,
> > > >> Kalesh
> > > >>
> > > >>> Is the goal avoiding races (e.g. file descriptor 3 is closed and reopened
> > > >>> to a different path between reading fdinfo and stating the fd)?
> > > >>>
> > > >>> Stephen
> > > >>>
> > > >>>> Signed-off-by: Kalesh Singh <kaleshsingh@google.com>
> > > >>>> ---
> > > >>>>
> > > >>>> Changes from rfc:
> > > >>>>    - Split adding 'size' and 'path' into a separate patches, per Christian
> > > >>>>    - Fix indentation (use tabs) in documentaion, per Randy
> > > >>>>
> > > >>>>   Documentation/filesystems/proc.rst | 14 ++++++++++++--
> > > >>>>   fs/proc/fd.c                       |  4 ++++
> > > >>>>   2 files changed, 16 insertions(+), 2 deletions(-)
> > > >>>>
> > > >>>> diff --git a/Documentation/filesystems/proc.rst b/Documentation/filesystems/proc.rst
> > > >>>> index 779c05528e87..591f12d30d97 100644
> > > >>>> --- a/Documentation/filesystems/proc.rst
> > > >>>> +++ b/Documentation/filesystems/proc.rst
> > > >>>> @@ -1886,14 +1886,16 @@ if precise results are needed.
> > > >>>>   3.8  /proc/<pid>/fdinfo/<fd> - Information about opened file
> > > >>>>   ---------------------------------------------------------------
> > > >>>>   This file provides information associated with an opened file. The regular
> > > >>>> -files have at least five fields -- 'pos', 'flags', 'mnt_id', 'ino', and 'size'.
> > > >>>> +files have at least six fields -- 'pos', 'flags', 'mnt_id', 'ino', 'size',
> > > >>>> +and 'path'.
> > > >>>>
> > > >>>>   The 'pos' represents the current offset of the opened file in decimal
> > > >>>>   form [see lseek(2) for details], 'flags' denotes the octal O_xxx mask the
> > > >>>>   file has been created with [see open(2) for details] and 'mnt_id' represents
> > > >>>>   mount ID of the file system containing the opened file [see 3.5
> > > >>>>   /proc/<pid>/mountinfo for details]. 'ino' represents the inode number of
> > > >>>> -the file, and 'size' represents the size of the file in bytes.
> > > >>>> +the file, 'size' represents the size of the file in bytes, and 'path'
> > > >>>> +represents the file path.
> > > >>>>
> > > >>>>   A typical output is::
> > > >>>>
> > > >>>> @@ -1902,6 +1904,7 @@ A typical output is::
> > > >>>>        mnt_id: 19
> > > >>>>        ino:    63107
> > > >>>>        size:   0
> > > >>>> +     path:   /dev/null
> > > >>>>
> > > >>>>   All locks associated with a file descriptor are shown in its fdinfo too::
> > > >>>>
> > > >>>> @@ -1920,6 +1923,7 @@ Eventfd files
> > > >>>>        mnt_id: 9
> > > >>>>        ino:    63107
> > > >>>>        size:   0
> > > >>>> +     path:   anon_inode:[eventfd]
> > > >>>>        eventfd-count:  5a
> > > >>>>
> > > >>>>   where 'eventfd-count' is hex value of a counter.
> > > >>>> @@ -1934,6 +1938,7 @@ Signalfd files
> > > >>>>        mnt_id: 9
> > > >>>>        ino:    63107
> > > >>>>        size:   0
> > > >>>> +     path:   anon_inode:[signalfd]
> > > >>>>        sigmask:        0000000000000200
> > > >>>>
> > > >>>>   where 'sigmask' is hex value of the signal mask associated
> > > >>>> @@ -1949,6 +1954,7 @@ Epoll files
> > > >>>>        mnt_id: 9
> > > >>>>        ino:    63107
> > > >>>>        size:   0
> > > >>>> +     path:   anon_inode:[eventpoll]
> > > >>>>        tfd:        5 events:       1d data: ffffffffffffffff pos:0 ino:61af sdev:7
> > > >>>>
> > > >>>>   where 'tfd' is a target file descriptor number in decimal form,
> > > >>>> @@ -1968,6 +1974,7 @@ For inotify files the format is the following::
> > > >>>>        mnt_id: 9
> > > >>>>        ino:    63107
> > > >>>>        size:   0
> > > >>>> +     path:   anon_inode:inotify
> > > >>>>        inotify wd:3 ino:9e7e sdev:800013 mask:800afce ignored_mask:0 fhandle-bytes:8 fhandle-type:1 f_handle:7e9e0000640d1b6d
> > > >>>>
> > > >>>>   where 'wd' is a watch descriptor in decimal form, i.e. a target file
> > > >>>> @@ -1992,6 +1999,7 @@ For fanotify files the format is::
> > > >>>>        mnt_id: 9
> > > >>>>        ino:    63107
> > > >>>>        size:   0
> > > >>>> +     path:   anon_inode:[fanotify]
> > > >>>>        fanotify flags:10 event-flags:0
> > > >>>>        fanotify mnt_id:12 mflags:40 mask:38 ignored_mask:40000003
> > > >>>>        fanotify ino:4f969 sdev:800013 mflags:0 mask:3b ignored_mask:40000000 fhandle-bytes:8 fhandle-type:1 f_handle:69f90400c275b5b4
> > > >>>> @@ -2018,6 +2026,7 @@ Timerfd files
> > > >>>>        mnt_id: 9
> > > >>>>        ino:    63107
> > > >>>>        size:   0
> > > >>>> +     path:   anon_inode:[timerfd]
> > > >>>>        clockid: 0
> > > >>>>        ticks: 0
> > > >>>>        settime flags: 01
> > > >>>> @@ -2042,6 +2051,7 @@ DMA Buffer files
> > > >>>>        mnt_id: 9
> > > >>>>        ino:    63107
> > > >>>>        size:   32768
> > > >>>> +     path:   /dmabuf:
> > > >>>>        count:  2
> > > >>>>        exp_name:  system-heap
> > > >>>>
> > > >>>> diff --git a/fs/proc/fd.c b/fs/proc/fd.c
> > > >>>> index 464bc3f55759..8889a8ba09d4 100644
> > > >>>> --- a/fs/proc/fd.c
> > > >>>> +++ b/fs/proc/fd.c
> > > >>>> @@ -60,6 +60,10 @@ static int seq_show(struct seq_file *m, void *v)
> > > >>>>        seq_printf(m, "ino:\t%lu\n", file_inode(file)->i_ino);
> > > >>>>        seq_printf(m, "size:\t%lli\n", (long long)file_inode(file)->i_size);
> > > >>>>
> > > >>>> +     seq_puts(m, "path:\t");
> > > >>>> +     seq_file_path(m, file, "\n");
> > > >>>> +     seq_putc(m, '\n');
> > > >>>> +
> > > >>>>        /* show_fd_locks() never deferences files so a stale value is safe */
> > > >>>>        show_fd_locks(m, file, files);
> > > >>>>        if (seq_has_overflowed(m))
> > > >>> --
> > > >>> To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com.
> > > >>>
> > > > _______________________________________________
> > > > Linaro-mm-sig mailing list -- linaro-mm-sig@lists.linaro.org
> > > > To unsubscribe send an email to linaro-mm-sig-leave@lists.linaro.org
> > >