Processes can pin shared memory by keeping a handle to it through a
file descriptor; for instance dmabufs, memfd, and ashsmem (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.
[1] https://lore.kernel.org/lkml/20210308170651.919148-1-kaleshsingh@google.com/
Signed-off-by: Kalesh Singh <kaleshsingh(a)google.com>
---
Documentation/filesystems/proc.rst | 22 ++++++++++++++++++++--
drivers/dma-buf/dma-buf.c | 1 -
fs/proc/fd.c | 9 +++++++--
3 files changed, 27 insertions(+), 5 deletions(-)
diff --git a/Documentation/filesystems/proc.rst b/Documentation/filesystems/proc.rst
index 061744c436d9..ad66d78aca51 100644
--- a/Documentation/filesystems/proc.rst
+++ b/Documentation/filesystems/proc.rst
@@ -1922,13 +1922,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 four fields -- 'pos', 'flags', 'mnt_id' and 'ino'.
+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.
+the file, 'size' represents the size of the file in bytes, and 'path'
+represents the file path.
A typical output is::
@@ -1936,6 +1939,8 @@ A typical output is::
flags: 0100002
mnt_id: 19
ino: 63107
+ size: 0
+ path: /dev/null
All locks associated with a file descriptor are shown in its fdinfo too::
@@ -1953,6 +1958,8 @@ Eventfd files
flags: 04002
mnt_id: 9
ino: 63107
+ size: 0
+ path: anon_inode:[eventfd]
eventfd-count: 5a
where 'eventfd-count' is hex value of a counter.
@@ -1966,6 +1973,8 @@ Signalfd files
flags: 04002
mnt_id: 9
ino: 63107
+ size: 0
+ path: anon_inode:[signalfd]
sigmask: 0000000000000200
where 'sigmask' is hex value of the signal mask associated
@@ -1980,6 +1989,8 @@ Epoll files
flags: 02
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,
@@ -1998,6 +2009,8 @@ For inotify files the format is the following::
flags: 02000000
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
@@ -2021,6 +2034,8 @@ For fanotify files the format is::
flags: 02
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
@@ -2046,6 +2061,8 @@ Timerfd files
flags: 02
mnt_id: 9
ino: 63107
+ size: 0
+ path: anon_inode:[timerfd]
clockid: 0
ticks: 0
settime flags: 01
@@ -2070,6 +2087,7 @@ DMA Buffer files
mnt_id: 9
ino: 63107
size: 32768
+ path: /dmabuf:
count: 2
exp_name: system-heap
diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index b1e25ae98302..d61183ff3c30 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -377,7 +377,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..a8a968bc58f0 100644
--- a/fs/proc/fd.c
+++ b/fs/proc/fd.c
@@ -54,10 +54,15 @@ 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",
+ seq_printf(m, "pos:\t%lli\nflags:\t0%o\nmnt_id:\t%i\nino:\t%lu\nsize:\t%zu\n",
(long long)file->f_pos, f_flags,
real_mount(file->f_path.mnt)->mnt_id,
- file_inode(file)->i_ino);
+ file_inode(file)->i_ino,
+ 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);
base-commit: b015dcd62b86d298829990f8261d5d154b8d7af5
--
2.36.1.124.g0e6072fb45-goog
The dmabuf file uses get_next_ino()(through dma_buf_getfile() ->
alloc_anon_inode()) to get an inode number and uses the same as a
directory name under /sys/kernel/dmabuf/buffers/<ino>. This directory is
used to collect the dmabuf stats and it is created through
dma_buf_stats_setup(). At current, failure to create this directory
entry can make the dma_buf_export() to fail.
Now, as the get_next_ino() can definitely give a repetitive inode no
causing the directory entry creation to fail with -EEXIST. This is a
problem on the systems where dmabuf stats functionality is enabled on
the production builds can make the dma_buf_export(), though the dmabuf
memory is allocated successfully, to fail just because it couldn't
create stats entry.
This issue we are able to see on the snapdragon system within 13 days
where there already exists a directory with inode no "122602" so
dma_buf_stats_setup() failed with -EEXIST as it is trying to create
the same directory entry.
To make the dentry name as unique, use the dmabuf fs specific inode
which is based on the simple atomic variable increment. There is tmpfs
subsystem too which relies on its own inode generation rather than
relying on the get_next_ino() for the same reason of avoiding the
duplicate inodes[1].
[1] https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/patch/?id=…
Signed-off-by: Charan Teja Kalla <quic_charante(a)quicinc.com>
Cc: <stable(a)vger.kernel.org> # 5.15.x+
Reviewed-by: Greg Kroah-Hartman <gregkh(a)linuxfoundation.org>
---
Changes in V3-resend:
-- Collect all the tags and apply stable tag.
Changes in V3:
-- Used the atomic64 variable to have dmabuf files its own inodes.
-- Ensured no UAPI breakage as suggested by Christian.
Changes in V2:
-- Used the atomic64_t variable to generate a unique_id to be appended to inode
to have an unique directory with name <inode_number-unique_id> -- Suggested by christian
-- Updated the ABI documentation -- Identified by Greg.
-- Massaged the commit log.
-- https://lore.kernel.org/all/1652191562-18700-1-git-send-email-quic_charante…
Changes in V1:
-- Used the inode->i_ctime->tv_secs as an id appended to inode to create the
unique directory with name <inode_number-time_in_secs>.
-- https://lore.kernel.org/all/1652178212-22383-1-git-send-email-quic_charante…
drivers/dma-buf/dma-buf.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index a6fc96e..0ad5039 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -407,6 +407,7 @@ static inline int is_dma_buf_file(struct file *file)
static struct file *dma_buf_getfile(struct dma_buf *dmabuf, int flags)
{
+ static atomic64_t dmabuf_inode = ATOMIC64_INIT(0);
struct file *file;
struct inode *inode = alloc_anon_inode(dma_buf_mnt->mnt_sb);
@@ -416,6 +417,13 @@ static struct file *dma_buf_getfile(struct dma_buf *dmabuf, int flags)
inode->i_size = dmabuf->size;
inode_set_bytes(inode, dmabuf->size);
+ /*
+ * The ->i_ino acquired from get_next_ino() is not unique thus
+ * not suitable for using it as dentry name by dmabuf stats.
+ * Override ->i_ino with the unique and dmabuffs specific
+ * value.
+ */
+ inode->i_ino = atomic64_add_return(1, &dmabuf_inode);
file = alloc_file_pseudo(inode, dma_buf_mnt, "dmabuf",
flags, &dma_buf_fops);
if (IS_ERR(file))
--
2.7.4
The dmabuf file uses get_next_ino()(through dma_buf_getfile() ->
alloc_anon_inode()) to get an inode number and uses the same as a
directory name under /sys/kernel/dmabuf/buffers/<ino>. This directory is
used to collect the dmabuf stats and it is created through
dma_buf_stats_setup(). At current, failure to create this directory
entry can make the dma_buf_export() to fail.
Now, as the get_next_ino() can definitely give a repetitive inode no
causing the directory entry creation to fail with -EEXIST. This is a
problem on the systems where dmabuf stats functionality is enabled on
the production builds can make the dma_buf_export(), though the dmabuf
memory is allocated successfully, to fail just because it couldn't
create stats entry.
This issue we are able to see on the snapdragon system within 13 days
where there already exists a directory with inode no "122602" so
dma_buf_stats_setup() failed with -EEXIST as it is trying to create
the same directory entry.
To make the dentry name as unique, use the dmabuf fs specific inode
which is based on the simple atomic variable increment. There is tmpfs
subsystem too which relies on its own inode generation rather than
relying on the get_next_ino() for the same reason of avoiding the
duplicate inodes[1].
[1] https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/patch/?id=…
Reported-by: kernel test robot <lkp(a)intel.com>
Signed-off-by: Charan Teja Kalla <quic_charante(a)quicinc.com>
---
Changes in V3:
-- Used the atomic64 variable to have dmabuf files its own inodes.
-- Ensured no UAPI breakage as suggested by Christian.
Changes in V2:
-- Used the atomic64_t variable to generate a unique_id to be appended to inode
to have an unique directory with name <inode_number-unique_id> -- Suggested by christian
-- Updated the ABI documentation -- Identified by Greg.
-- Massaged the commit log.
-- https://lore.kernel.org/all/1652191562-18700-1-git-send-email-quic_charante…
Changes in V1:
-- Used the inode->i_ctime->tv_secs as an id appended to inode to create the
unique directory with name <inode_number-time_in_secs>.
-- https://lore.kernel.org/all/1652178212-22383-1-git-send-email-quic_charante…
drivers/dma-buf/dma-buf.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index a6fc96e..0ad5039 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -407,6 +407,7 @@ static inline int is_dma_buf_file(struct file *file)
static struct file *dma_buf_getfile(struct dma_buf *dmabuf, int flags)
{
+ static atomic64_t dmabuf_inode = ATOMIC64_INIT(0);
struct file *file;
struct inode *inode = alloc_anon_inode(dma_buf_mnt->mnt_sb);
@@ -416,6 +417,13 @@ static struct file *dma_buf_getfile(struct dma_buf *dmabuf, int flags)
inode->i_size = dmabuf->size;
inode_set_bytes(inode, dmabuf->size);
+ /*
+ * The ->i_ino acquired from get_next_ino() is not unique thus
+ * not suitable for using it as dentry name by dmabuf stats.
+ * Override ->i_ino with the unique and dmabuffs specific
+ * value.
+ */
+ inode->i_ino = atomic64_add_return(1, &dmabuf_inode);
file = alloc_file_pseudo(inode, dma_buf_mnt, "dmabuf",
flags, &dma_buf_fops);
if (IS_ERR(file))
--
2.7.4
The dmabuf file uses get_next_ino()(through dma_buf_getfile() ->
alloc_anon_inode()) to get an inode number and uses the same as a
directory name under /sys/kernel/dmabuf/buffers/<ino>. This directory is
used to collect the dmabuf stats and it is created through
dma_buf_stats_setup(). At current, failure to create this directory
entry can make the dma_buf_export() to fail.
Now, as the get_next_ino() can definitely give a repetitive inode no
causing the directory entry creation to fail with -EEXIST. This is a
problem on the systems where dmabuf stats functionality is enabled on
the production builds can make the dma_buf_export(), though the dmabuf
memory is allocated successfully, to fail just because it couldn't
create stats entry.
This issue we are able to see on the snapdragon system within 13 days
where there already exists a directory with inode no "122602" so
dma_buf_stats_setup() failed with -EEXIST as it is trying to create
the same directory entry.
To make the directory entry as unique, append the unique_id for every
inode. With this change the stats directory entries will be in the
format of: /sys/kernel/dmabuf/buffers/<inode_number-unique_id>.
Signed-off-by: Charan Teja Kalla <quic_charante(a)quicinc.com>
---
Changes in V2:
-- Used the atomic64_t variable to generate a unique_id to be appended to inode
to have an unique directory with name <inode_number-unique_id> -- Suggested by christian
-- Updated the ABI documentation -- Identified by Greg.
-- Massaged the commit log.
Changes in V1:
-- Used the inode->i_ctime->tv_secs as an id appended to inode to create the
unique directory with name <inode_number-time_in_secs>.
-- https://lore.kernel.org/all/1652178212-22383-1-git-send-email-quic_charante…
Documentation/ABI/testing/sysfs-kernel-dmabuf-buffers | 10 +++++-----
drivers/dma-buf/Kconfig | 6 +++---
drivers/dma-buf/dma-buf-sysfs-stats.c | 8 +++++---
3 files changed, 13 insertions(+), 11 deletions(-)
diff --git a/Documentation/ABI/testing/sysfs-kernel-dmabuf-buffers b/Documentation/ABI/testing/sysfs-kernel-dmabuf-buffers
index 5d3bc99..9fffbd3 100644
--- a/Documentation/ABI/testing/sysfs-kernel-dmabuf-buffers
+++ b/Documentation/ABI/testing/sysfs-kernel-dmabuf-buffers
@@ -4,19 +4,19 @@ KernelVersion: v5.13
Contact: Hridya Valsaraju <hridya(a)google.com>
Description: The /sys/kernel/dmabuf/buffers directory contains a
snapshot of the internal state of every DMA-BUF.
- /sys/kernel/dmabuf/buffers/<inode_number> will contain the
- statistics for the DMA-BUF with the unique inode number
- <inode_number>
+ /sys/kernel/dmabuf/buffers/<inode_number-unique_id> will
+ contain the statistics for the DMA-BUF with the unique
+ pair <inode_number-unique_id>
Users: kernel memory tuning/debugging tools
-What: /sys/kernel/dmabuf/buffers/<inode_number>/exporter_name
+What: /sys/kernel/dmabuf/buffers/<inode_number-unique_id>/exporter_name
Date: May 2021
KernelVersion: v5.13
Contact: Hridya Valsaraju <hridya(a)google.com>
Description: This file is read-only and contains the name of the exporter of
the DMA-BUF.
-What: /sys/kernel/dmabuf/buffers/<inode_number>/size
+What: /sys/kernel/dmabuf/buffers/<inode_number-unique_id>/size
Date: May 2021
KernelVersion: v5.13
Contact: Hridya Valsaraju <hridya(a)google.com>
diff --git a/drivers/dma-buf/Kconfig b/drivers/dma-buf/Kconfig
index 541efe0..5bcbdb1 100644
--- a/drivers/dma-buf/Kconfig
+++ b/drivers/dma-buf/Kconfig
@@ -81,9 +81,9 @@ menuconfig DMABUF_SYSFS_STATS
Choose this option to enable DMA-BUF sysfs statistics
in location /sys/kernel/dmabuf/buffers.
- /sys/kernel/dmabuf/buffers/<inode_number> will contain
- statistics for the DMA-BUF with the unique inode number
- <inode_number>.
+ /sys/kernel/dmabuf/buffers/<inode_number-unique_id> will contain
+ statistics for the DMA-BUF with the unique pair
+ <inode_number-unique_id>.
source "drivers/dma-buf/heaps/Kconfig"
diff --git a/drivers/dma-buf/dma-buf-sysfs-stats.c b/drivers/dma-buf/dma-buf-sysfs-stats.c
index 2bba0ba..29e9e23 100644
--- a/drivers/dma-buf/dma-buf-sysfs-stats.c
+++ b/drivers/dma-buf/dma-buf-sysfs-stats.c
@@ -38,8 +38,8 @@
*
* The following stats are exposed by the interface:
*
- * * ``/sys/kernel/dmabuf/buffers/<inode_number>/exporter_name``
- * * ``/sys/kernel/dmabuf/buffers/<inode_number>/size``
+ * * ``/sys/kernel/dmabuf/buffers/<inode_number-unique_id>/exporter_name``
+ * * ``/sys/kernel/dmabuf/buffers/<inode_number-unique_id>/size``
*
* The information in the interface can also be used to derive per-exporter
* statistics. The data from the interface can be gathered on error conditions
@@ -172,6 +172,7 @@ int dma_buf_stats_setup(struct dma_buf *dmabuf)
{
struct dma_buf_sysfs_entry *sysfs_entry;
int ret;
+ static atomic64_t unique_id = ATOMIC_INIT(0);
if (!dmabuf || !dmabuf->file)
return -EINVAL;
@@ -192,7 +193,8 @@ int dma_buf_stats_setup(struct dma_buf *dmabuf)
/* create the directory for buffer stats */
ret = kobject_init_and_add(&sysfs_entry->kobj, &dma_buf_ktype, NULL,
- "%lu", file_inode(dmabuf->file)->i_ino);
+ "%lu-%lu", file_inode(dmabuf->file)->i_ino,
+ atomic64_add_return(1, &unique_id));
if (ret)
goto err_sysfs_dmabuf;
--
2.7.4
The dmabuf file uses get_next_ino()(through dma_buf_getfile() ->
alloc_anon_inode()) to get an inode number and uses the same as a
directory name under /sys/kernel/dmabuf/buffers/<ino>. This directory is
used to collect the dmabuf stats and it is created through
dma_buf_stats_setup(). At current, failure to create this directory
entry can make the dma_buf_export() to fail.
Now, as the get_next_ino() can definitely give a repetitive inode no
causing the directory entry creation to fail with -EEXIST. This is a
problem on the systems where dmabuf stats functionality is enabled on
the production builds can make the dma_buf_export(), though the dmabuf
memory is allocated successfully, to fail just because it couldn't
create stats entry.
This issue we are able to see on the snapdragon system within 13 days
where there already exists a directory with inode no "122602" so
dma_buf_stats_setup() failed with -EEXIST as it is trying to create
the same directory entry.
To make the directory entry as unique, append the inode creation time to
the inode. With this change the stats directory entries will be in the
format of: /sys/kernel/dmabuf/buffers/<inode no>-<inode creation time in
secs>.
Signed-off-by: Charan Teja Kalla <quic_charante(a)quicinc.com>
---
drivers/dma-buf/dma-buf-sysfs-stats.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/dma-buf/dma-buf-sysfs-stats.c b/drivers/dma-buf/dma-buf-sysfs-stats.c
index 2bba0ba..292cb31 100644
--- a/drivers/dma-buf/dma-buf-sysfs-stats.c
+++ b/drivers/dma-buf/dma-buf-sysfs-stats.c
@@ -192,7 +192,8 @@ int dma_buf_stats_setup(struct dma_buf *dmabuf)
/* create the directory for buffer stats */
ret = kobject_init_and_add(&sysfs_entry->kobj, &dma_buf_ktype, NULL,
- "%lu", file_inode(dmabuf->file)->i_ino);
+ "%lu-%lu", file_inode(dmabuf->file)->i_ino,
+ file_inode(dmabuf->file)->i_ctime.tv_sec);
if (ret)
goto err_sysfs_dmabuf;
--
2.7.4