Re: [PATCH] procfs/dmabuf: Add /proc/<pid>/task/<tid>/dmabuf_fds

+jeffv from Android

On Tue, Jan 26, 2021 at 11:51 PM Kalesh Singh [off-list ref] wrote:

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.

Or you could try to let the DMA buffer take a reference on the
mm_struct and account its size into the mm_struct? That would probably
be nicer to work with than having to poke around in procfs separately
for DMA buffers.

Currently, DMA buffer FDs can be accounted using /proc/<pid>/fd/* and
/proc/<pid>/fdinfo -- both of which are only root readable, as follows:

That's not quite right. They can both also be accessed by the user
owning the process. Also, fdinfo is a standard interface for
inspecting process state that doesn't permit reading process memory or
manipulating process state - so I think it would be fine to permit
access to fdinfo under a PTRACE_MODE_READ_FSCRED check, just like the
interface you're suggesting.

  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.

Android captures per-process system memory state when certain low memory
events (e.g a foreground app kill) occur, to identify potential memory
hoggers. To include a process’s dmabuf usage as part of its memory state,
the data collection needs to be fast enough to reflect the memory state at
the time of such events.

Since reading /proc/<pid>/fd/ and /proc/<pid>/fdinfo/ requires root
privileges, this approach is not suitable for production builds.

It should be easy to add enough information to /proc/<pid>/fdinfo/ so
that you don't need to look at /proc/<pid>/fd/ anymore.

Granting
root privileges even to a system process increases the attack surface and
is highly undesirable. Additionally this is slow as it requires many
context switches for searching and getting the dma-buf info.

What do you mean by "context switches"? Task switches or kernel/user
transitions (e.g. via syscall)?

With the addition of per-buffer dmabuf stats in sysfs [1], the DMA buffer
details can be queried using their unique inode numbers.

This patch proposes adding a /proc/<pid>/task/<tid>/dmabuf_fds interface.

/proc/<pid>/task/<tid>/dmabuf_fds contains a list of inode numbers for
every DMA buffer FD that the task has. Entries with the same inode
number can appear more than once, indicating the total FD references
for the associated DMA buffer.

If a thread shares the same files as the group leader then its
dmabuf_fds file will be empty, as these dmabufs are reported by the
group leader.

The interface requires PTRACE_MODE_READ_FSCRED (same as /proc/<pid>/maps)
and allows the efficient accounting of per-process DMA buffer usage without
requiring root privileges. (See data below)

I'm not convinced that introducing a new procfs file for this is the
right way to go. And the idea of having to poke into multiple
different files in procfs and in sysfs just to be able to compute a
proper memory usage score for a process seems weird to me. "How much
memory is this process using" seems like the kind of question the
kernel ought to be able to answer (and the kernel needs to be able to
answer somewhat accurately so that its own OOM killer can do its job
properly)?

On Wed 27-01-21 11:47:29, Jann Horn wrote:

+jeffv from Android

On Tue, Jan 26, 2021 at 11:51 PM Kalesh Singh [off-list ref] wrote:

quoted

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.

Or you could try to let the DMA buffer take a reference on the
mm_struct and account its size into the mm_struct? That would probably
be nicer to work with than having to poke around in procfs separately
for DMA buffers.

Yes that would make some sense to me as well but how do you know that
the process actually uses a buffer? If it mmaps it then you already have
that information via /proc/<pid>/maps. My understanding of dma-buf is
really coarse but my impression is that you can consume the memory via
standard read syscall as well. How would you account for that.

[...]
Skipping over a large part of your response but I do agree that the
interface is really elaborate to drill down to the information.

I'm not convinced that introducing a new procfs file for this is the
right way to go. And the idea of having to poke into multiple
different files in procfs and in sysfs just to be able to compute a
proper memory usage score for a process seems weird to me. "How much
memory is this process using" seems like the kind of question the
kernel ought to be able to answer (and the kernel needs to be able to
answer somewhat accurately so that its own OOM killer can do its job
properly)?

Well, shared buffers are tricky but it is true that we already consider
shmem in badness so this wouldn't go out of line. Kernel oom killer
could be more clever with these special fds though and query for buffer
size directly.
-- 
Michal Hocko
SUSE Labs

Am 27.01.21 um 11:57 schrieb Michal Hocko:

On Wed 27-01-21 11:47:29, Jann Horn wrote:

quoted

+jeffv from Android

On Tue, Jan 26, 2021 at 11:51 PM Kalesh Singh [off-list ref] wrote:

quoted

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.

Or you could try to let the DMA buffer take a reference on the
mm_struct and account its size into the mm_struct? That would probably
be nicer to work with than having to poke around in procfs separately
for DMA buffers.

Yes that would make some sense to me as well but how do you know that
the process actually uses a buffer? If it mmaps it then you already have
that information via /proc/<pid>/maps. My understanding of dma-buf is
really coarse but my impression is that you can consume the memory via
standard read syscall as well. How would you account for that.

Somewhat correct. This interface here really doesn't make sense since 
the file descriptor representation of DMA-buf is only meant to be used 
for short term usage.

E.g. the idea is that you can export a DMA-buf fd from your device 
driver, transfer that to another process and then import it again into a 
device driver.

Keeping a long term reference to a DMA-buf fd sounds like a design bug 
in userspace to me.

[...]
Skipping over a large part of your response but I do agree that the
interface is really elaborate to drill down to the information.

quoted

I'm not convinced that introducing a new procfs file for this is the
right way to go. And the idea of having to poke into multiple
different files in procfs and in sysfs just to be able to compute a
proper memory usage score for a process seems weird to me. "How much
memory is this process using" seems like the kind of question the
kernel ought to be able to answer (and the kernel needs to be able to
answer somewhat accurately so that its own OOM killer can do its job
properly)?

Well, shared buffers are tricky but it is true that we already consider
shmem in badness so this wouldn't go out of line. Kernel oom killer
could be more clever with these special fds though and query for buffer
size directly.

Some years ago I've proposed an change to improve the OOM killer to take 
into account how much memory is reference through special file 
descriptors like device drivers or DMA-buf.

But that never want anywhere because of concerns that this would add to 
much overhead to the OOM killer.

Regards,
Christian.

On Wed 27-01-21 12:01:55, Christian König wrote:
[...]

Some years ago I've proposed an change to improve the OOM killer to take
into account how much memory is reference through special file descriptors
like device drivers or DMA-buf.

But that never want anywhere because of concerns that this would add to much
overhead to the OOM killer.

I have to admit I do not remember such a discussion. There were many on
the related topic, concerns were mostly about a proper accounting of
shared resources or locking required to gain the information. I do not
remember overhead as being the main concern as oom is a real cold path.

Anyway, if you want to revamp those patches then feel free to CC me and
we can discuss further. I do not want to hijack this thread by an
unrelated topic.
-- 
Michal Hocko
SUSE Labs

On Wed, Jan 27, 2021 at 5:47 AM Jann Horn [off-list ref] wrote:

+jeffv from Android

On Tue, Jan 26, 2021 at 11:51 PM Kalesh Singh [off-list ref] wrote:

quoted

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.

Or you could try to let the DMA buffer take a reference on the
mm_struct and account its size into the mm_struct? That would probably
be nicer to work with than having to poke around in procfs separately
for DMA buffers.

quoted

Currently, DMA buffer FDs can be accounted using /proc/<pid>/fd/* and
/proc/<pid>/fdinfo -- both of which are only root readable, as follows:

That's not quite right. They can both also be accessed by the user
owning the process. Also, fdinfo is a standard interface for
inspecting process state that doesn't permit reading process memory or
manipulating process state - so I think it would be fine to permit
access to fdinfo under a PTRACE_MODE_READ_FSCRED check, just like the
interface you're suggesting.

Hi everyone. Thank you for the feedback.

I understand there is a deeper problem of accounting shared memory in
the kernel, that’s not only specific to the DMA buffers. In this case
DMA buffers, I think Jann’s proposal is the cleanest way to attribute
the shared buffers to processes. I can respin a patch modifying fdinfo
as suggested, if this is not an issue from a security perspective.

Thanks,
Kalesh

quoted

  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.

Android captures per-process system memory state when certain low memory
events (e.g a foreground app kill) occur, to identify potential memory
hoggers. To include a process’s dmabuf usage as part of its memory state,
the data collection needs to be fast enough to reflect the memory state at
the time of such events.

Since reading /proc/<pid>/fd/ and /proc/<pid>/fdinfo/ requires root
privileges, this approach is not suitable for production builds.

It should be easy to add enough information to /proc/<pid>/fdinfo/ so
that you don't need to look at /proc/<pid>/fd/ anymore.

quoted

Granting
root privileges even to a system process increases the attack surface and
is highly undesirable. Additionally this is slow as it requires many
context switches for searching and getting the dma-buf info.

What do you mean by "context switches"? Task switches or kernel/user
transitions (e.g. via syscall)?

quoted

With the addition of per-buffer dmabuf stats in sysfs [1], the DMA buffer
details can be queried using their unique inode numbers.

This patch proposes adding a /proc/<pid>/task/<tid>/dmabuf_fds interface.

/proc/<pid>/task/<tid>/dmabuf_fds contains a list of inode numbers for
every DMA buffer FD that the task has. Entries with the same inode
number can appear more than once, indicating the total FD references
for the associated DMA buffer.

If a thread shares the same files as the group leader then its
dmabuf_fds file will be empty, as these dmabufs are reported by the
group leader.

The interface requires PTRACE_MODE_READ_FSCRED (same as /proc/<pid>/maps)
and allows the efficient accounting of per-process DMA buffer usage without
requiring root privileges. (See data below)

I'm not convinced that introducing a new procfs file for this is the
right way to go. And the idea of having to poke into multiple
different files in procfs and in sysfs just to be able to compute a
proper memory usage score for a process seems weird to me. "How much
memory is this process using" seems like the kind of question the
kernel ought to be able to answer (and the kernel needs to be able to
answer somewhat accurately so that its own OOM killer can do its job
properly)?

On Wed, 27 Jan 2021 12:01:55 +0100
Christian König [off-list ref] wrote:

Somewhat correct. This interface here really doesn't make sense since 
the file descriptor representation of DMA-buf is only meant to be used 
for short term usage.

E.g. the idea is that you can export a DMA-buf fd from your device 
driver, transfer that to another process and then import it again into a 
device driver.

Keeping a long term reference to a DMA-buf fd sounds like a design bug 
in userspace to me.

Except keeping the fd is exactly what userspace must do if it wishes to
re-use the buffer without passing a new fd over IPC again. Particularly
Wayland compositors need to keep the client buffer dmabuf fd open after
receiving it, so that they can re-import it to EGL to ensure updated
contents are correctly flushed as EGL has no other API for it.

That is my vague understanding, and what Weston implements. You can say
it's a bad userspace API design in EGL, but what else can we do?

However, in the particular case of Wayland, the shared dmabufs should
be accounted to the Wayland client process. OOM-killing the client
process will eventually free the dmabuf, also the Wayland server
references to it. Killing the Wayland server (compositor, display
server) OTOH is something that should not be done as long as there are
e.g. Wayland clients to be killed.

Unfortunately(?), Wayland clients do not have a reason to keep the
dmabuf fd open themselves, so they probably close it as soon as it has
been sent to a display server. So the process that should be OOM-killed
does not have an open fd for the dmabuf (but probably has something
else, but not an mmap for CPU). Buffer re-use in Wayland does not
require re-sending the dmabuf fd over IPC.

(In general, dmabufs are never mmapped for CPU. They are accessed by
devices.)


Thanks,
pq