[PATCH v3 1/2] procfs: Allow reading fdinfo with PTRACE_MODE_READ

[PATCH v3 1/2] procfs: Allow reading fdinfo with PTRACE_MODE_READ

[Kalesh Singh]

Android captures per-process system memory state when certain low memory
events (e.g a foreground app kill) occur, to identify potential memory
hoggers. In order to measure how much memory a process actually consumes,
it is necessary to include the DMA buffer sizes for that process in the
memory accounting. Since the handle to DMA buffers are raw FDs, it is
important to be able to identify which processes have FD references to
a DMA buffer.

Currently, DMA buffer FDs can be accounted using /proc/<pid>/fd/* and
/proc/<pid>/fdinfo -- both are only readable by the process owner,
as follows:
  1. Do a readlink on each FD.
  2. If the target path begins with "/dmabuf", then the FD is a dmabuf FD.
  3. stat the file to get the dmabuf inode number.
  4. Read/ proc/<pid>/fdinfo/<fd>, to get the DMA buffer size.

Accessing other processes’ fdinfo requires root privileges. This limits
the use of the interface to debugging environments and is not suitable
for production builds.  Granting root privileges even to a system process
increases the attack surface and is highly undesirable.

Since fdinfo doesn't permit reading process memory and manipulating
process state, allow accessing fdinfo under PTRACE_MODE_READ_FSCRED.

Suggested-by: Jann Horn <jannh@google.com>
Signed-off-by: Kalesh Singh <kaleshsingh@google.com>
---
Changes in v2:
  - Update patch description

 fs/proc/base.c |  4 ++--
 fs/proc/fd.c   | 15 ++++++++++++++-
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index b3422cda2a91..a37f9de7103f 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -3160,7 +3160,7 @@ static const struct pid_entry tgid_base_stuff[] = {
 	DIR("task",       S_IRUGO|S_IXUGO, proc_task_inode_operations, proc_task_operations),
 	DIR("fd",         S_IRUSR|S_IXUSR, proc_fd_inode_operations, proc_fd_operations),
 	DIR("map_files",  S_IRUSR|S_IXUSR, proc_map_files_inode_operations, proc_map_files_operations),
-	DIR("fdinfo",     S_IRUSR|S_IXUSR, proc_fdinfo_inode_operations, proc_fdinfo_operations),
+	DIR("fdinfo",     S_IRUGO|S_IXUGO, proc_fdinfo_inode_operations, proc_fdinfo_operations),
 	DIR("ns",	  S_IRUSR|S_IXUGO, proc_ns_dir_inode_operations, proc_ns_dir_operations),
 #ifdef CONFIG_NET
 	DIR("net",        S_IRUGO|S_IXUGO, proc_net_inode_operations, proc_net_operations),
@@ -3504,7 +3504,7 @@ static const struct inode_operations proc_tid_comm_inode_operations = {
  */
 static const struct pid_entry tid_base_stuff[] = {
 	DIR("fd",        S_IRUSR|S_IXUSR, proc_fd_inode_operations, proc_fd_operations),
-	DIR("fdinfo",    S_IRUSR|S_IXUSR, proc_fdinfo_inode_operations, proc_fdinfo_operations),
+	DIR("fdinfo",    S_IRUGO|S_IXUGO, proc_fdinfo_inode_operations, proc_fdinfo_operations),
 	DIR("ns",	 S_IRUSR|S_IXUGO, proc_ns_dir_inode_operations, proc_ns_dir_operations),
 #ifdef CONFIG_NET
 	DIR("net",        S_IRUGO|S_IXUGO, proc_net_inode_operations, proc_net_operations),
diff --git a/fs/proc/fd.c b/fs/proc/fd.c
index cb51763ed554..585e213301f9 100644
--- a/fs/proc/fd.c
+++ b/fs/proc/fd.c
@@ -6,6 +6,7 @@
 #include <linux/fdtable.h>
 #include <linux/namei.h>
 #include <linux/pid.h>
+#include <linux/ptrace.h>
 #include <linux/security.h>
 #include <linux/file.h>
 #include <linux/seq_file.h>
@@ -72,6 +73,18 @@ static int seq_show(struct seq_file *m, void *v)
 
 static int seq_fdinfo_open(struct inode *inode, struct file *file)
 {
+	bool allowed = false;
+	struct task_struct *task = get_proc_task(inode);
+
+	if (!task)
+		return -ESRCH;
+
+	allowed = ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS);
+	put_task_struct(task);
+
+	if (!allowed)
+		return -EACCES;
+
 	return single_open(file, seq_show, inode);
 }
 
@@ -307,7 +320,7 @@ static struct dentry *proc_fdinfo_instantiate(struct dentry *dentry,
 	struct proc_inode *ei;
 	struct inode *inode;
 
-	inode = proc_pid_make_inode(dentry->d_sb, task, S_IFREG | S_IRUSR);
+	inode = proc_pid_make_inode(dentry->d_sb, task, S_IFREG | S_IRUGO);
 	if (!inode)
 		return ERR_PTR(-ENOENT);
 
-- 
2.30.0.478.g8a0d178c01-goog

[PATCH v3 2/2] dmabuf: Add dmabuf inode number to /proc/*/fdinfo

[Kalesh Singh]

If a FD refers to a DMA buffer add the DMA buffer inode number to
/proc/<pid>/fdinfo/<FD> and /proc/<pid>/task/<tid>/fdindo/<FD>.

The dmabuf inode number allows userspace to uniquely identify the buffer
and avoids a dependency on /proc/<pid>/fd/* when accounting per-process
DMA buffer sizes.

Signed-off-by: Kalesh Singh <kaleshsingh@google.com>
---
Changes in v3:
  - Add documentation in proc.rst
Changes in v2:
  - Update patch description

 Documentation/filesystems/proc.rst | 17 +++++++++++++++++
 drivers/dma-buf/dma-buf.c          |  1 +
 2 files changed, 18 insertions(+)

diff --git a/Documentation/filesystems/proc.rst b/Documentation/filesystems/proc.rst
index 2fa69f710e2a..fdd38676f57f 100644
--- a/Documentation/filesystems/proc.rst
+++ b/Documentation/filesystems/proc.rst
@@ -2031,6 +2031,23 @@ details]. 'it_value' is remaining time until the timer expiration.
 with TIMER_ABSTIME option which will be shown in 'settime flags', but 'it_value'
 still exhibits timer's remaining time.
 
+DMA Buffer files
+~~~~~~~~~~~~~~~~
+
+::
+
+	pos:	0
+	flags:	04002
+	mnt_id:	9
+	dmabuf_inode_no: 63107
+	size:   32768
+	count:  2
+	exp_name:  system-heap
+
+where 'dmabuf_inode_no' is the unique inode number of the DMA buffer file.
+'size' is the size of the DMA buffer in bytes. 'count' is the file count of
+the DMA buffer file. 'exp_name' is the name of the DMA buffer exporter.
+
 3.9	/proc/<pid>/map_files - Information about memory mapped files
 ---------------------------------------------------------------------
 This directory contains symbolic links which represent memory mapped files
diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index 9ad6397aaa97..d869099ede83 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -414,6 +414,7 @@ static void dma_buf_show_fdinfo(struct seq_file *m, struct file *file)
 {
 	struct dma_buf *dmabuf = file->private_data;
 
+	seq_printf(m, "dmabuf_inode_no:\t%lu\n", file_inode(file)->i_ino);
 	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);
-- 
2.30.0.478.g8a0d178c01-goog

Re: [PATCH v3 2/2] dmabuf: Add dmabuf inode number to /proc/*/fdinfo

[Randy Dunlap]

On 2/4/21 6:23 PM, Kalesh Singh wrote:
> If a FD refers to a DMA buffer add the DMA buffer inode number to
> /proc/<pid>/fdinfo/<FD> and /proc/<pid>/task/<tid>/fdindo/<FD>.
> 
> The dmabuf inode number allows userspace to uniquely identify the buffer
> and avoids a dependency on /proc/<pid>/fd/* when accounting per-process
> DMA buffer sizes.
> 
> Signed-off-by: Kalesh Singh <kaleshsingh@google.com>
> ---
> Changes in v3:
>   - Add documentation in proc.rst

Hi,
Thanks for the doc update.

> Changes in v2:
>   - Update patch description
> 
>  Documentation/filesystems/proc.rst | 17 +++++++++++++++++
>  drivers/dma-buf/dma-buf.c          |  1 +
>  2 files changed, 18 insertions(+)
> 

Acked-by: Randy Dunlap <rdunlap@infradead.org> # for Documentation/filesystems/proc.rst


-- 
~Randy

Re: [PATCH v3 2/2] dmabuf: Add dmabuf inode number to /proc/*/fdinfo

[Christian König]

Am 05.02.21 um 03:23 schrieb Kalesh Singh:
> If a FD refers to a DMA buffer add the DMA buffer inode number to
> /proc/<pid>/fdinfo/<FD> and /proc/<pid>/task/<tid>/fdindo/<FD>.
>
> The dmabuf inode number allows userspace to uniquely identify the buffer
> and avoids a dependency on /proc/<pid>/fd/* when accounting per-process
> DMA buffer sizes.

BTW: Why do we make this DMA-buf specific? Couldn't we always print the 
inode number for all fds?

Regards,
Christian.

>
> Signed-off-by: Kalesh Singh <kaleshsingh@google.com>
> ---
> Changes in v3:
>    - Add documentation in proc.rst
> Changes in v2:
>    - Update patch description
>
>   Documentation/filesystems/proc.rst | 17 +++++++++++++++++
>   drivers/dma-buf/dma-buf.c          |  1 +
>   2 files changed, 18 insertions(+)
>
> diff --git a/Documentation/filesystems/proc.rst b/Documentation/filesystems/proc.rst
> index 2fa69f710e2a..fdd38676f57f 100644
> --- a/Documentation/filesystems/proc.rst
> +++ b/Documentation/filesystems/proc.rst
> @@ -2031,6 +2031,23 @@ details]. 'it_value' is remaining time until the timer expiration.
>   with TIMER_ABSTIME option which will be shown in 'settime flags', but 'it_value'
>   still exhibits timer's remaining time.
>   
> +DMA Buffer files
> +~~~~~~~~~~~~~~~~
> +
> +::
> +
> +	pos:	0
> +	flags:	04002
> +	mnt_id:	9
> +	dmabuf_inode_no: 63107
> +	size:   32768
> +	count:  2
> +	exp_name:  system-heap
> +
> +where 'dmabuf_inode_no' is the unique inode number of the DMA buffer file.
> +'size' is the size of the DMA buffer in bytes. 'count' is the file count of
> +the DMA buffer file. 'exp_name' is the name of the DMA buffer exporter.
> +
>   3.9	/proc/<pid>/map_files - Information about memory mapped files
>   ---------------------------------------------------------------------
>   This directory contains symbolic links which represent memory mapped files
> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> index 9ad6397aaa97..d869099ede83 100644
> --- a/drivers/dma-buf/dma-buf.c
> +++ b/drivers/dma-buf/dma-buf.c
> @@ -414,6 +414,7 @@ static void dma_buf_show_fdinfo(struct seq_file *m, struct file *file)
>   {
>   	struct dma_buf *dmabuf = file->private_data;
>   
> +	seq_printf(m, "dmabuf_inode_no:\t%lu\n", file_inode(file)->i_ino);
>   	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);

Re: [PATCH v3 2/2] dmabuf: Add dmabuf inode number to /proc/*/fdinfo

[Kalesh Singh]

On Fri, Feb 5, 2021 at 2:56 AM Christian König <christian.koenig@amd.com> wrote:
>
> Am 05.02.21 um 03:23 schrieb Kalesh Singh:
> > If a FD refers to a DMA buffer add the DMA buffer inode number to
> > /proc/<pid>/fdinfo/<FD> and /proc/<pid>/task/<tid>/fdindo/<FD>.
> >
> > The dmabuf inode number allows userspace to uniquely identify the buffer
> > and avoids a dependency on /proc/<pid>/fd/* when accounting per-process
> > DMA buffer sizes.
>
> BTW: Why do we make this DMA-buf specific? Couldn't we always print the
> inode number for all fds?

Good point. We can make this a common field instead of DMA buf
specific. I will update in the next version.

Thanks,
Kalesh
>
> Regards,
> Christian.
>
> >
> > Signed-off-by: Kalesh Singh <kaleshsingh@google.com>
> > ---
> > Changes in v3:
> >    - Add documentation in proc.rst
> > Changes in v2:
> >    - Update patch description
> >
> >   Documentation/filesystems/proc.rst | 17 +++++++++++++++++
> >   drivers/dma-buf/dma-buf.c          |  1 +
> >   2 files changed, 18 insertions(+)
> >
> > diff --git a/Documentation/filesystems/proc.rst b/Documentation/filesystems/proc.rst
> > index 2fa69f710e2a..fdd38676f57f 100644
> > --- a/Documentation/filesystems/proc.rst
> > +++ b/Documentation/filesystems/proc.rst
> > @@ -2031,6 +2031,23 @@ details]. 'it_value' is remaining time until the timer expiration.
> >   with TIMER_ABSTIME option which will be shown in 'settime flags', but 'it_value'
> >   still exhibits timer's remaining time.
> >
> > +DMA Buffer files
> > +~~~~~~~~~~~~~~~~
> > +
> > +::
> > +
> > +     pos:    0
> > +     flags:  04002
> > +     mnt_id: 9
> > +     dmabuf_inode_no: 63107
> > +     size:   32768
> > +     count:  2
> > +     exp_name:  system-heap
> > +
> > +where 'dmabuf_inode_no' is the unique inode number of the DMA buffer file.
> > +'size' is the size of the DMA buffer in bytes. 'count' is the file count of
> > +the DMA buffer file. 'exp_name' is the name of the DMA buffer exporter.
> > +
> >   3.9 /proc/<pid>/map_files - Information about memory mapped files
> >   ---------------------------------------------------------------------
> >   This directory contains symbolic links which represent memory mapped files
> > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> > index 9ad6397aaa97..d869099ede83 100644
> > --- a/drivers/dma-buf/dma-buf.c
> > +++ b/drivers/dma-buf/dma-buf.c
> > @@ -414,6 +414,7 @@ static void dma_buf_show_fdinfo(struct seq_file *m, struct file *file)
> >   {
> >       struct dma_buf *dmabuf = file->private_data;
> >
> > +     seq_printf(m, "dmabuf_inode_no:\t%lu\n", file_inode(file)->i_ino);
> >       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);
>