From: Hridya Valsaraju <[email protected]>
This patch introduces flags BINDER_FD_FLAG_XFER_CHARGE, and
BINDER_FD_FLAG_XFER_CHARGE that a process sending an individual fd or
fd array to another process over binder IPC can set to relinquish
ownership of the fd(s) being sent for memory accounting purposes. If the
flag is found to be set during the fd or fd array translation and the
fd is for a DMA-BUF, the buffer is uncharged from the sender's cgroup
and charged to the receiving process's cgroup instead.
It is up to the sending process to ensure that it closes the fds
regardless of whether the transfer failed or succeeded.
Most graphics shared memory allocations in Android are done by the
graphics allocator HAL process. On requests from clients, the HAL
process allocates memory and sends the fds to the clients over binder
IPC. The graphics allocator HAL will not retain any references to the
buffers. When the HAL sets *_FLAG_XFER_CHARGE for fd arrays holding
DMA-BUF fds, or individual fd objects, binder will transfer the charge
for the buffer from the allocator process cgroup to the client process
cgroup.
The pad [1] and pad_flags [2] fields of binder_fd_object and
binder_fda_array_object come from alignment with flat_binder_object and
have never been exposed for use from userspace. This new flags use
follows the pattern set by binder_buffer_object.
[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/include/uapi/linux/android/binder.h?id=feba3900cabb8e7c87368faa28e7a6936809ba22
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/include/uapi/linux/android/binder.h?id=5cdcf4c6a638591ec0e98c57404a19e7f9997567
Signed-off-by: Hridya Valsaraju <[email protected]>
Signed-off-by: T.J. Mercier <[email protected]>
---
Documentation/admin-guide/cgroup-v2.rst | 3 ++-
drivers/android/binder.c | 31 +++++++++++++++++++++----
drivers/dma-buf/dma-buf.c | 4 +---
include/linux/dma-buf.h | 1 +
include/uapi/linux/android/binder.h | 23 ++++++++++++++----
5 files changed, 50 insertions(+), 12 deletions(-)
diff --git a/Documentation/admin-guide/cgroup-v2.rst b/Documentation/admin-guide/cgroup-v2.rst
index 538ae22bc514..d225295932c0 100644
--- a/Documentation/admin-guide/cgroup-v2.rst
+++ b/Documentation/admin-guide/cgroup-v2.rst
@@ -1457,7 +1457,8 @@ PAGE_SIZE multiple when read back.
dmabuf (npn)
Amount of memory used for exported DMA buffers allocated by the cgroup.
- Stays with the allocating cgroup regardless of how the buffer is shared.
+ Stays with the allocating cgroup regardless of how the buffer is shared
+ unless explicitly transferred.
workingset_refault_anon
Number of refaults of previously evicted anonymous pages.
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 880224ec6abb..9830848c8d25 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -42,6 +42,7 @@
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+#include <linux/dma-buf.h>
#include <linux/fdtable.h>
#include <linux/file.h>
#include <linux/freezer.h>
@@ -2237,7 +2238,7 @@ static int binder_translate_handle(struct flat_binder_object *fp,
return ret;
}
-static int binder_translate_fd(u32 fd, binder_size_t fd_offset,
+static int binder_translate_fd(u32 fd, binder_size_t fd_offset, __u32 flags,
struct binder_transaction *t,
struct binder_thread *thread,
struct binder_transaction *in_reply_to)
@@ -2275,6 +2276,26 @@ static int binder_translate_fd(u32 fd, binder_size_t fd_offset,
goto err_security;
}
+ if (IS_ENABLED(CONFIG_MEMCG) && (flags & BINDER_FD_FLAG_XFER_CHARGE)) {
+ struct dma_buf *dmabuf;
+
+ if (unlikely(!is_dma_buf_file(file))) {
+ binder_user_error(
+ "%d:%d got transaction with XFER_CHARGE for non-dmabuf fd, %d\n",
+ proc->pid, thread->pid, fd);
+ ret = -EINVAL;
+ goto err_dmabuf;
+ }
+
+ dmabuf = file->private_data;
+ ret = dma_buf_transfer_charge(dmabuf, target_proc->tsk);
+ if (ret) {
+ pr_warn("%d:%d Unable to transfer DMA-BUF fd charge to %d\n",
+ proc->pid, thread->pid, target_proc->pid);
+ goto err_xfer;
+ }
+ }
+
/*
* Add fixup record for this transaction. The allocation
* of the fd in the target needs to be done from a
@@ -2294,6 +2315,8 @@ static int binder_translate_fd(u32 fd, binder_size_t fd_offset,
return ret;
err_alloc:
+err_xfer:
+err_dmabuf:
err_security:
fput(file);
err_fget:
@@ -2604,7 +2627,7 @@ static int binder_translate_fd_array(struct list_head *pf_head,
ret = copy_from_user(&fd, sender_ufda_base + sender_uoffset, sizeof(fd));
if (!ret)
- ret = binder_translate_fd(fd, offset, t, thread,
+ ret = binder_translate_fd(fd, offset, fda->flags, t, thread,
in_reply_to);
if (ret)
return ret > 0 ? -EINVAL : ret;
@@ -3383,8 +3406,8 @@ static void binder_transaction(struct binder_proc *proc,
struct binder_fd_object *fp = to_binder_fd_object(hdr);
binder_size_t fd_offset = object_offset +
(uintptr_t)&fp->fd - (uintptr_t)fp;
- int ret = binder_translate_fd(fp->fd, fd_offset, t,
- thread, in_reply_to);
+ int ret = binder_translate_fd(fp->fd, fd_offset, fp->flags,
+ t, thread, in_reply_to);
fp->pad_binder = 0;
if (ret < 0 ||
diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index fd6c5002032b..a65b42433099 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -34,8 +34,6 @@
#include "dma-buf-sysfs-stats.h"
-static inline int is_dma_buf_file(struct file *);
-
struct dma_buf_list {
struct list_head head;
struct mutex lock;
@@ -527,7 +525,7 @@ static const struct file_operations dma_buf_fops = {
/*
* is_dma_buf_file - Check if struct file* is associated with dma_buf
*/
-static inline int is_dma_buf_file(struct file *file)
+int is_dma_buf_file(struct file *file)
{
return file->f_op == &dma_buf_fops;
}
diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
index 6aa128d76aa7..092d572ce528 100644
--- a/include/linux/dma-buf.h
+++ b/include/linux/dma-buf.h
@@ -595,6 +595,7 @@ dma_buf_attachment_is_dynamic(struct dma_buf_attachment *attach)
return !!attach->importer_ops;
}
+int is_dma_buf_file(struct file *file);
struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf,
struct device *dev);
struct dma_buf_attachment *
diff --git a/include/uapi/linux/android/binder.h b/include/uapi/linux/android/binder.h
index e72e4de8f452..696c2bdb8a7e 100644
--- a/include/uapi/linux/android/binder.h
+++ b/include/uapi/linux/android/binder.h
@@ -91,14 +91,14 @@ struct flat_binder_object {
/**
* struct binder_fd_object - describes a filedescriptor to be fixed up.
* @hdr: common header structure
- * @pad_flags: padding to remain compatible with old userspace code
+ * @flags: One or more BINDER_FD_FLAG_* flags
* @pad_binder: padding to remain compatible with old userspace code
* @fd: file descriptor
* @cookie: opaque data, used by user-space
*/
struct binder_fd_object {
struct binder_object_header hdr;
- __u32 pad_flags;
+ __u32 flags;
union {
binder_uintptr_t pad_binder;
__u32 fd;
@@ -107,6 +107,17 @@ struct binder_fd_object {
binder_uintptr_t cookie;
};
+enum {
+ /**
+ * @BINDER_FD_FLAG_XFER_CHARGE
+ *
+ * When set, the sender of a binder_fd_object wishes to relinquish ownership of the fd for
+ * memory accounting purposes. If the fd is for a DMA-BUF, the buffer is uncharged from the
+ * sender's cgroup and charged to the receiving process's cgroup instead.
+ */
+ BINDER_FD_FLAG_XFER_CHARGE = 0x01,
+};
+
/* struct binder_buffer_object - object describing a userspace buffer
* @hdr: common header structure
* @flags: one or more BINDER_BUFFER_* flags
@@ -141,7 +152,7 @@ enum {
/* struct binder_fd_array_object - object describing an array of fds in a buffer
* @hdr: common header structure
- * @pad: padding to ensure correct alignment
+ * @flags: One or more BINDER_FDA_FLAG_* flags
* @num_fds: number of file descriptors in the buffer
* @parent: index in offset array to buffer holding the fd array
* @parent_offset: start offset of fd array in the buffer
@@ -162,12 +173,16 @@ enum {
*/
struct binder_fd_array_object {
struct binder_object_header hdr;
- __u32 pad;
+ __u32 flags;
binder_size_t num_fds;
binder_size_t parent;
binder_size_t parent_offset;
};
+enum {
+ BINDER_FDA_FLAG_XFER_CHARGE = BINDER_FD_FLAG_XFER_CHARGE,
+};
+
/*
* On 64-bit platforms where user code may run in 32-bits the driver must
* translate the buffer (and local binder) addresses appropriately.
--
2.39.0.314.g84b9a713c41-goog
On Mon, Jan 9, 2023 at 6:07 PM Hillf Danton <[email protected]> wrote:
>
> On 9 Jan 2023 21:38:06 +0000 T.J. Mercier <[email protected]>
> >
> > @@ -2275,6 +2276,26 @@ static int binder_translate_fd(u32 fd, binder_size_t fd_offset,
> > goto err_security;
> > }
> >
> > + if (IS_ENABLED(CONFIG_MEMCG) && (flags & BINDER_FD_FLAG_XFER_CHARGE)) {
> > + struct dma_buf *dmabuf;
> > +
> > + if (unlikely(!is_dma_buf_file(file))) {
> > + binder_user_error(
> > + "%d:%d got transaction with XFER_CHARGE for non-dmabuf fd, %d\n",
> > + proc->pid, thread->pid, fd);
> > + ret = -EINVAL;
> > + goto err_dmabuf;
> > + }
>
> It barely makes sense to expose is_dma_buf_file() only for this.
> > +
> > + dmabuf = file->private_data;
> > + ret = dma_buf_transfer_charge(dmabuf, target_proc->tsk);
> > + if (ret) {
> > + pr_warn("%d:%d Unable to transfer DMA-BUF fd charge to %d\n",
> > + proc->pid, thread->pid, target_proc->pid);
> > + goto err_xfer;
> > + }
> > + }
> > +
>
> This whole hunk should go to dma-buf instead by adding change to
> dma_buf_transfer_charge() for instance.
Fair enough, will change this for v2. I think we'd still want to
distinguish between the two failure modes for logging purposes, so
I'll use the return value of dma_buf_transfer_charge to do that.
On Mon, Jan 09, 2023 at 09:38:06PM +0000, T.J. Mercier wrote:
> From: Hridya Valsaraju <[email protected]>
>
> This patch introduces flags BINDER_FD_FLAG_XFER_CHARGE, and
> BINDER_FD_FLAG_XFER_CHARGE that a process sending an individual fd or
I believe the second one was meant to be BINDER_FDA_FLAG_XFER_CHARGE.
However, I don't think a separation of flags is needed. We process each
fd in the array individually anyway. So, it's OK to reuse the FD flags
for FDAs too.
> fd array to another process over binder IPC can set to relinquish
> ownership of the fd(s) being sent for memory accounting purposes. If the
> flag is found to be set during the fd or fd array translation and the
> fd is for a DMA-BUF, the buffer is uncharged from the sender's cgroup
> and charged to the receiving process's cgroup instead.
>
> It is up to the sending process to ensure that it closes the fds
> regardless of whether the transfer failed or succeeded.
>
> Most graphics shared memory allocations in Android are done by the
> graphics allocator HAL process. On requests from clients, the HAL
> process allocates memory and sends the fds to the clients over binder
> IPC. The graphics allocator HAL will not retain any references to the
> buffers. When the HAL sets *_FLAG_XFER_CHARGE for fd arrays holding
> DMA-BUF fds, or individual fd objects, binder will transfer the charge
> for the buffer from the allocator process cgroup to the client process
> cgroup.
>
> The pad [1] and pad_flags [2] fields of binder_fd_object and
> binder_fda_array_object come from alignment with flat_binder_object and
> have never been exposed for use from userspace. This new flags use
> follows the pattern set by binder_buffer_object.
>
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/include/uapi/linux/android/binder.h?id=feba3900cabb8e7c87368faa28e7a6936809ba22
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/include/uapi/linux/android/binder.h?id=5cdcf4c6a638591ec0e98c57404a19e7f9997567
>
> Signed-off-by: Hridya Valsaraju <[email protected]>
> Signed-off-by: T.J. Mercier <[email protected]>
> ---
> Documentation/admin-guide/cgroup-v2.rst | 3 ++-
> drivers/android/binder.c | 31 +++++++++++++++++++++----
> drivers/dma-buf/dma-buf.c | 4 +---
> include/linux/dma-buf.h | 1 +
> include/uapi/linux/android/binder.h | 23 ++++++++++++++----
> 5 files changed, 50 insertions(+), 12 deletions(-)
>
> diff --git a/Documentation/admin-guide/cgroup-v2.rst b/Documentation/admin-guide/cgroup-v2.rst
> index 538ae22bc514..d225295932c0 100644
> --- a/Documentation/admin-guide/cgroup-v2.rst
> +++ b/Documentation/admin-guide/cgroup-v2.rst
> @@ -1457,7 +1457,8 @@ PAGE_SIZE multiple when read back.
>
> dmabuf (npn)
> Amount of memory used for exported DMA buffers allocated by the cgroup.
> - Stays with the allocating cgroup regardless of how the buffer is shared.
> + Stays with the allocating cgroup regardless of how the buffer is shared
> + unless explicitly transferred.
>
> workingset_refault_anon
> Number of refaults of previously evicted anonymous pages.
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index 880224ec6abb..9830848c8d25 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -42,6 +42,7 @@
>
> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>
> +#include <linux/dma-buf.h>
> #include <linux/fdtable.h>
> #include <linux/file.h>
> #include <linux/freezer.h>
> @@ -2237,7 +2238,7 @@ static int binder_translate_handle(struct flat_binder_object *fp,
> return ret;
> }
>
> -static int binder_translate_fd(u32 fd, binder_size_t fd_offset,
> +static int binder_translate_fd(u32 fd, binder_size_t fd_offset, __u32 flags,
> struct binder_transaction *t,
> struct binder_thread *thread,
> struct binder_transaction *in_reply_to)
> @@ -2275,6 +2276,26 @@ static int binder_translate_fd(u32 fd, binder_size_t fd_offset,
> goto err_security;
> }
>
> + if (IS_ENABLED(CONFIG_MEMCG) && (flags & BINDER_FD_FLAG_XFER_CHARGE)) {
> + struct dma_buf *dmabuf;
> +
> + if (unlikely(!is_dma_buf_file(file))) {
> + binder_user_error(
> + "%d:%d got transaction with XFER_CHARGE for non-dmabuf fd, %d\n",
> + proc->pid, thread->pid, fd);
> + ret = -EINVAL;
> + goto err_dmabuf;
> + }
> +
> + dmabuf = file->private_data;
> + ret = dma_buf_transfer_charge(dmabuf, target_proc->tsk);
> + if (ret) {
> + pr_warn("%d:%d Unable to transfer DMA-BUF fd charge to %d\n",
> + proc->pid, thread->pid, target_proc->pid);
> + goto err_xfer;
> + }
> + }
> +
> /*
> * Add fixup record for this transaction. The allocation
> * of the fd in the target needs to be done from a
> @@ -2294,6 +2315,8 @@ static int binder_translate_fd(u32 fd, binder_size_t fd_offset,
> return ret;
>
> err_alloc:
> +err_xfer:
> +err_dmabuf:
> err_security:
> fput(file);
> err_fget:
> @@ -2604,7 +2627,7 @@ static int binder_translate_fd_array(struct list_head *pf_head,
>
> ret = copy_from_user(&fd, sender_ufda_base + sender_uoffset, sizeof(fd));
> if (!ret)
> - ret = binder_translate_fd(fd, offset, t, thread,
> + ret = binder_translate_fd(fd, offset, fda->flags, t, thread,
> in_reply_to);
> if (ret)
> return ret > 0 ? -EINVAL : ret;
> @@ -3383,8 +3406,8 @@ static void binder_transaction(struct binder_proc *proc,
> struct binder_fd_object *fp = to_binder_fd_object(hdr);
> binder_size_t fd_offset = object_offset +
> (uintptr_t)&fp->fd - (uintptr_t)fp;
> - int ret = binder_translate_fd(fp->fd, fd_offset, t,
> - thread, in_reply_to);
> + int ret = binder_translate_fd(fp->fd, fd_offset, fp->flags,
> + t, thread, in_reply_to);
>
> fp->pad_binder = 0;
> if (ret < 0 ||
IMO the changes to the dma-buf api should some in a separate patch. So
those can be approved and managed separately.
> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> index fd6c5002032b..a65b42433099 100644
> --- a/drivers/dma-buf/dma-buf.c
> +++ b/drivers/dma-buf/dma-buf.c
> @@ -34,8 +34,6 @@
>
> #include "dma-buf-sysfs-stats.h"
>
> -static inline int is_dma_buf_file(struct file *);
> -
> struct dma_buf_list {
> struct list_head head;
> struct mutex lock;
> @@ -527,7 +525,7 @@ static const struct file_operations dma_buf_fops = {
> /*
> * is_dma_buf_file - Check if struct file* is associated with dma_buf
> */
> -static inline int is_dma_buf_file(struct file *file)
> +int is_dma_buf_file(struct file *file)
> {
> return file->f_op == &dma_buf_fops;
> }
> diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
> index 6aa128d76aa7..092d572ce528 100644
> --- a/include/linux/dma-buf.h
> +++ b/include/linux/dma-buf.h
> @@ -595,6 +595,7 @@ dma_buf_attachment_is_dynamic(struct dma_buf_attachment *attach)
> return !!attach->importer_ops;
> }
>
> +int is_dma_buf_file(struct file *file);
> struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf,
> struct device *dev);
> struct dma_buf_attachment *
> diff --git a/include/uapi/linux/android/binder.h b/include/uapi/linux/android/binder.h
> index e72e4de8f452..696c2bdb8a7e 100644
> --- a/include/uapi/linux/android/binder.h
> +++ b/include/uapi/linux/android/binder.h
> @@ -91,14 +91,14 @@ struct flat_binder_object {
> /**
> * struct binder_fd_object - describes a filedescriptor to be fixed up.
> * @hdr: common header structure
> - * @pad_flags: padding to remain compatible with old userspace code
> + * @flags: One or more BINDER_FD_FLAG_* flags
> * @pad_binder: padding to remain compatible with old userspace code
> * @fd: file descriptor
> * @cookie: opaque data, used by user-space
> */
> struct binder_fd_object {
> struct binder_object_header hdr;
> - __u32 pad_flags;
> + __u32 flags;
> union {
> binder_uintptr_t pad_binder;
> __u32 fd;
> @@ -107,6 +107,17 @@ struct binder_fd_object {
> binder_uintptr_t cookie;
> };
>
> +enum {
> + /**
> + * @BINDER_FD_FLAG_XFER_CHARGE
> + *
> + * When set, the sender of a binder_fd_object wishes to relinquish ownership of the fd for
> + * memory accounting purposes. If the fd is for a DMA-BUF, the buffer is uncharged from the
> + * sender's cgroup and charged to the receiving process's cgroup instead.
> + */
> + BINDER_FD_FLAG_XFER_CHARGE = 0x01,
> +};
> +
> /* struct binder_buffer_object - object describing a userspace buffer
> * @hdr: common header structure
> * @flags: one or more BINDER_BUFFER_* flags
> @@ -141,7 +152,7 @@ enum {
>
> /* struct binder_fd_array_object - object describing an array of fds in a buffer
> * @hdr: common header structure
> - * @pad: padding to ensure correct alignment
> + * @flags: One or more BINDER_FDA_FLAG_* flags
> * @num_fds: number of file descriptors in the buffer
> * @parent: index in offset array to buffer holding the fd array
> * @parent_offset: start offset of fd array in the buffer
> @@ -162,12 +173,16 @@ enum {
> */
> struct binder_fd_array_object {
> struct binder_object_header hdr;
> - __u32 pad;
> + __u32 flags;
> binder_size_t num_fds;
> binder_size_t parent;
> binder_size_t parent_offset;
> };
>
> +enum {
> + BINDER_FDA_FLAG_XFER_CHARGE = BINDER_FD_FLAG_XFER_CHARGE,
> +};
> +
I would prefer to drop this. It should avoid silly mistakes in
userspace similar to the typo in the commit message above.
> /*
> * On 64-bit platforms where user code may run in 32-bits the driver must
> * translate the buffer (and local binder) addresses appropriately.
> --
> 2.39.0.314.g84b9a713c41-goog
>
Thanks,
--
Carlos Llamas
On Fri, Jan 20, 2023 at 1:25 PM Carlos Llamas <[email protected]> wrote:
>
> On Mon, Jan 09, 2023 at 09:38:06PM +0000, T.J. Mercier wrote:
> > From: Hridya Valsaraju <[email protected]>
> >
> > This patch introduces flags BINDER_FD_FLAG_XFER_CHARGE, and
> > BINDER_FD_FLAG_XFER_CHARGE that a process sending an individual fd or
>
> I believe the second one was meant to be BINDER_FDA_FLAG_XFER_CHARGE.
> However, I don't think a separation of flags is needed. We process each
> fd in the array individually anyway. So, it's OK to reuse the FD flags
> for FDAs too.
Yes, thanks.
>
>
> > fd array to another process over binder IPC can set to relinquish
> > ownership of the fd(s) being sent for memory accounting purposes. If the
> > flag is found to be set during the fd or fd array translation and the
> > fd is for a DMA-BUF, the buffer is uncharged from the sender's cgroup
> > and charged to the receiving process's cgroup instead.
> >
> > It is up to the sending process to ensure that it closes the fds
> > regardless of whether the transfer failed or succeeded.
> >
> > Most graphics shared memory allocations in Android are done by the
> > graphics allocator HAL process. On requests from clients, the HAL
> > process allocates memory and sends the fds to the clients over binder
> > IPC. The graphics allocator HAL will not retain any references to the
> > buffers. When the HAL sets *_FLAG_XFER_CHARGE for fd arrays holding
> > DMA-BUF fds, or individual fd objects, binder will transfer the charge
> > for the buffer from the allocator process cgroup to the client process
> > cgroup.
> >
> > The pad [1] and pad_flags [2] fields of binder_fd_object and
> > binder_fda_array_object come from alignment with flat_binder_object and
> > have never been exposed for use from userspace. This new flags use
> > follows the pattern set by binder_buffer_object.
> >
> > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/include/uapi/linux/android/binder.h?id=feba3900cabb8e7c87368faa28e7a6936809ba22
> > [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/include/uapi/linux/android/binder.h?id=5cdcf4c6a638591ec0e98c57404a19e7f9997567
> >
> > Signed-off-by: Hridya Valsaraju <[email protected]>
> > Signed-off-by: T.J. Mercier <[email protected]>
> > ---
> > Documentation/admin-guide/cgroup-v2.rst | 3 ++-
> > drivers/android/binder.c | 31 +++++++++++++++++++++----
> > drivers/dma-buf/dma-buf.c | 4 +---
> > include/linux/dma-buf.h | 1 +
> > include/uapi/linux/android/binder.h | 23 ++++++++++++++----
> > 5 files changed, 50 insertions(+), 12 deletions(-)
> >
> > diff --git a/Documentation/admin-guide/cgroup-v2.rst b/Documentation/admin-guide/cgroup-v2.rst
> > index 538ae22bc514..d225295932c0 100644
> > --- a/Documentation/admin-guide/cgroup-v2.rst
> > +++ b/Documentation/admin-guide/cgroup-v2.rst
> > @@ -1457,7 +1457,8 @@ PAGE_SIZE multiple when read back.
> >
> > dmabuf (npn)
> > Amount of memory used for exported DMA buffers allocated by the cgroup.
> > - Stays with the allocating cgroup regardless of how the buffer is shared.
> > + Stays with the allocating cgroup regardless of how the buffer is shared
> > + unless explicitly transferred.
> >
> > workingset_refault_anon
> > Number of refaults of previously evicted anonymous pages.
> > diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> > index 880224ec6abb..9830848c8d25 100644
> > --- a/drivers/android/binder.c
> > +++ b/drivers/android/binder.c
> > @@ -42,6 +42,7 @@
> >
> > #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> >
> > +#include <linux/dma-buf.h>
> > #include <linux/fdtable.h>
> > #include <linux/file.h>
> > #include <linux/freezer.h>
> > @@ -2237,7 +2238,7 @@ static int binder_translate_handle(struct flat_binder_object *fp,
> > return ret;
> > }
> >
> > -static int binder_translate_fd(u32 fd, binder_size_t fd_offset,
> > +static int binder_translate_fd(u32 fd, binder_size_t fd_offset, __u32 flags,
> > struct binder_transaction *t,
> > struct binder_thread *thread,
> > struct binder_transaction *in_reply_to)
> > @@ -2275,6 +2276,26 @@ static int binder_translate_fd(u32 fd, binder_size_t fd_offset,
> > goto err_security;
> > }
> >
> > + if (IS_ENABLED(CONFIG_MEMCG) && (flags & BINDER_FD_FLAG_XFER_CHARGE)) {
> > + struct dma_buf *dmabuf;
> > +
> > + if (unlikely(!is_dma_buf_file(file))) {
> > + binder_user_error(
> > + "%d:%d got transaction with XFER_CHARGE for non-dmabuf fd, %d\n",
> > + proc->pid, thread->pid, fd);
> > + ret = -EINVAL;
> > + goto err_dmabuf;
> > + }
> > +
> > + dmabuf = file->private_data;
> > + ret = dma_buf_transfer_charge(dmabuf, target_proc->tsk);
> > + if (ret) {
> > + pr_warn("%d:%d Unable to transfer DMA-BUF fd charge to %d\n",
> > + proc->pid, thread->pid, target_proc->pid);
> > + goto err_xfer;
> > + }
> > + }
> > +
> > /*
> > * Add fixup record for this transaction. The allocation
> > * of the fd in the target needs to be done from a
> > @@ -2294,6 +2315,8 @@ static int binder_translate_fd(u32 fd, binder_size_t fd_offset,
> > return ret;
> >
> > err_alloc:
> > +err_xfer:
> > +err_dmabuf:
> > err_security:
> > fput(file);
> > err_fget:
> > @@ -2604,7 +2627,7 @@ static int binder_translate_fd_array(struct list_head *pf_head,
> >
> > ret = copy_from_user(&fd, sender_ufda_base + sender_uoffset, sizeof(fd));
> > if (!ret)
> > - ret = binder_translate_fd(fd, offset, t, thread,
> > + ret = binder_translate_fd(fd, offset, fda->flags, t, thread,
> > in_reply_to);
> > if (ret)
> > return ret > 0 ? -EINVAL : ret;
> > @@ -3383,8 +3406,8 @@ static void binder_transaction(struct binder_proc *proc,
> > struct binder_fd_object *fp = to_binder_fd_object(hdr);
> > binder_size_t fd_offset = object_offset +
> > (uintptr_t)&fp->fd - (uintptr_t)fp;
> > - int ret = binder_translate_fd(fp->fd, fd_offset, t,
> > - thread, in_reply_to);
> > + int ret = binder_translate_fd(fp->fd, fd_offset, fp->flags,
> > + t, thread, in_reply_to);
> >
> > fp->pad_binder = 0;
> > if (ret < 0 ||
>
> IMO the changes to the dma-buf api should some in a separate patch. So
> those can be approved and managed separately.
>
I've actually already dropped these based on feedback from Hillf, so
there are no longer any dma-buf.c changes on this patch for the v2 I
have queued up.
> > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> > index fd6c5002032b..a65b42433099 100644
> > --- a/drivers/dma-buf/dma-buf.c
> > +++ b/drivers/dma-buf/dma-buf.c
> > @@ -34,8 +34,6 @@
> >
> > #include "dma-buf-sysfs-stats.h"
> >
> > -static inline int is_dma_buf_file(struct file *);
> > -
> > struct dma_buf_list {
> > struct list_head head;
> > struct mutex lock;
> > @@ -527,7 +525,7 @@ static const struct file_operations dma_buf_fops = {
> > /*
> > * is_dma_buf_file - Check if struct file* is associated with dma_buf
> > */
> > -static inline int is_dma_buf_file(struct file *file)
> > +int is_dma_buf_file(struct file *file)
> > {
> > return file->f_op == &dma_buf_fops;
> > }
> > diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
> > index 6aa128d76aa7..092d572ce528 100644
> > --- a/include/linux/dma-buf.h
> > +++ b/include/linux/dma-buf.h
> > @@ -595,6 +595,7 @@ dma_buf_attachment_is_dynamic(struct dma_buf_attachment *attach)
> > return !!attach->importer_ops;
> > }
> >
> > +int is_dma_buf_file(struct file *file);
> > struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf,
> > struct device *dev);
> > struct dma_buf_attachment *
> > diff --git a/include/uapi/linux/android/binder.h b/include/uapi/linux/android/binder.h
> > index e72e4de8f452..696c2bdb8a7e 100644
> > --- a/include/uapi/linux/android/binder.h
> > +++ b/include/uapi/linux/android/binder.h
> > @@ -91,14 +91,14 @@ struct flat_binder_object {
> > /**
> > * struct binder_fd_object - describes a filedescriptor to be fixed up.
> > * @hdr: common header structure
> > - * @pad_flags: padding to remain compatible with old userspace code
> > + * @flags: One or more BINDER_FD_FLAG_* flags
> > * @pad_binder: padding to remain compatible with old userspace code
> > * @fd: file descriptor
> > * @cookie: opaque data, used by user-space
> > */
> > struct binder_fd_object {
> > struct binder_object_header hdr;
> > - __u32 pad_flags;
> > + __u32 flags;
> > union {
> > binder_uintptr_t pad_binder;
> > __u32 fd;
> > @@ -107,6 +107,17 @@ struct binder_fd_object {
> > binder_uintptr_t cookie;
> > };
> >
> > +enum {
> > + /**
> > + * @BINDER_FD_FLAG_XFER_CHARGE
> > + *
> > + * When set, the sender of a binder_fd_object wishes to relinquish ownership of the fd for
> > + * memory accounting purposes. If the fd is for a DMA-BUF, the buffer is uncharged from the
> > + * sender's cgroup and charged to the receiving process's cgroup instead.
> > + */
> > + BINDER_FD_FLAG_XFER_CHARGE = 0x01,
> > +};
> > +
> > /* struct binder_buffer_object - object describing a userspace buffer
> > * @hdr: common header structure
> > * @flags: one or more BINDER_BUFFER_* flags
> > @@ -141,7 +152,7 @@ enum {
> >
> > /* struct binder_fd_array_object - object describing an array of fds in a buffer
> > * @hdr: common header structure
> > - * @pad: padding to ensure correct alignment
> > + * @flags: One or more BINDER_FDA_FLAG_* flags
> > * @num_fds: number of file descriptors in the buffer
> > * @parent: index in offset array to buffer holding the fd array
> > * @parent_offset: start offset of fd array in the buffer
> > @@ -162,12 +173,16 @@ enum {
> > */
> > struct binder_fd_array_object {
> > struct binder_object_header hdr;
> > - __u32 pad;
> > + __u32 flags;
> > binder_size_t num_fds;
> > binder_size_t parent;
> > binder_size_t parent_offset;
> > };
> >
> > +enum {
> > + BINDER_FDA_FLAG_XFER_CHARGE = BINDER_FD_FLAG_XFER_CHARGE,
> > +};
> > +
>
> I would prefer to drop this. It should avoid silly mistakes in
> userspace similar to the typo in the commit message above.
>
Ok I'll make that work.
>
> > /*
> > * On 64-bit platforms where user code may run in 32-bits the driver must
> > * translate the buffer (and local binder) addresses appropriately.
> > --
> > 2.39.0.314.g84b9a713c41-goog
> >
>
> Thanks,
> --
> Carlos Llamas