* [PATCH v3 1/7] binder: create userspace-to-binder-buffer copy function
2019-02-08 18:35 [PATCH v3 0/7] binder: eliminate use of vmalloc space for binder buffers Todd Kjos
@ 2019-02-08 18:35 ` Todd Kjos
2019-02-14 19:45 ` Joel Fernandes
2019-02-08 18:35 ` [PATCH v3 2/7] binder: add functions to copy to/from binder buffers Todd Kjos
` (6 subsequent siblings)
7 siblings, 1 reply; 16+ messages in thread
From: Todd Kjos @ 2019-02-08 18:35 UTC (permalink / raw)
To: tkjos, gregkh, arve, devel, linux-kernel, maco; +Cc: joel, kernel-team
The binder driver uses a vm_area to map the per-process
binder buffer space. For 32-bit android devices, this is
now taking too much vmalloc space. This patch removes
the use of vm_area when copying the transaction data
from the sender to the buffer space. Instead of using
copy_from_user() for multi-page copies, it now uses
binder_alloc_copy_user_to_buffer() which uses kmap()
and kunmap() to map each page, and uses copy_from_user()
for copying to that page.
Signed-off-by: Todd Kjos <tkjos@google.com>
---
v2: remove casts as suggested by Dan Carpenter
drivers/android/binder.c | 29 +++++++--
drivers/android/binder_alloc.c | 113 +++++++++++++++++++++++++++++++++
drivers/android/binder_alloc.h | 8 +++
3 files changed, 143 insertions(+), 7 deletions(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 5f6ef5e63b91e..ab0b3eec363bc 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -3078,8 +3078,12 @@ static void binder_transaction(struct binder_proc *proc,
ALIGN(tr->data_size, sizeof(void *)));
offp = off_start;
- if (copy_from_user(t->buffer->data, (const void __user *)(uintptr_t)
- tr->data.ptr.buffer, tr->data_size)) {
+ if (binder_alloc_copy_user_to_buffer(
+ &target_proc->alloc,
+ t->buffer, 0,
+ (const void __user *)
+ (uintptr_t)tr->data.ptr.buffer,
+ tr->data_size)) {
binder_user_error("%d:%d got transaction with invalid data ptr\n",
proc->pid, thread->pid);
return_error = BR_FAILED_REPLY;
@@ -3087,8 +3091,13 @@ static void binder_transaction(struct binder_proc *proc,
return_error_line = __LINE__;
goto err_copy_data_failed;
}
- if (copy_from_user(offp, (const void __user *)(uintptr_t)
- tr->data.ptr.offsets, tr->offsets_size)) {
+ if (binder_alloc_copy_user_to_buffer(
+ &target_proc->alloc,
+ t->buffer,
+ ALIGN(tr->data_size, sizeof(void *)),
+ (const void __user *)
+ (uintptr_t)tr->data.ptr.offsets,
+ tr->offsets_size)) {
binder_user_error("%d:%d got transaction with invalid offsets ptr\n",
proc->pid, thread->pid);
return_error = BR_FAILED_REPLY;
@@ -3217,6 +3226,8 @@ static void binder_transaction(struct binder_proc *proc,
struct binder_buffer_object *bp =
to_binder_buffer_object(hdr);
size_t buf_left = sg_buf_end - sg_bufp;
+ binder_size_t sg_buf_offset = (uintptr_t)sg_bufp -
+ (uintptr_t)t->buffer->data;
if (bp->length > buf_left) {
binder_user_error("%d:%d got transaction with too large buffer\n",
@@ -3226,9 +3237,13 @@ static void binder_transaction(struct binder_proc *proc,
return_error_line = __LINE__;
goto err_bad_offset;
}
- if (copy_from_user(sg_bufp,
- (const void __user *)(uintptr_t)
- bp->buffer, bp->length)) {
+ if (binder_alloc_copy_user_to_buffer(
+ &target_proc->alloc,
+ t->buffer,
+ sg_buf_offset,
+ (const void __user *)
+ (uintptr_t)bp->buffer,
+ bp->length)) {
binder_user_error("%d:%d got transaction with invalid offsets ptr\n",
proc->pid, thread->pid);
return_error_param = -EFAULT;
diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
index 022cd80e80cc3..94c0d85c4e75b 100644
--- a/drivers/android/binder_alloc.c
+++ b/drivers/android/binder_alloc.c
@@ -29,6 +29,8 @@
#include <linux/list_lru.h>
#include <linux/ratelimit.h>
#include <asm/cacheflush.h>
+#include <linux/uaccess.h>
+#include <linux/highmem.h>
#include "binder_alloc.h"
#include "binder_trace.h"
@@ -1053,3 +1055,114 @@ int binder_alloc_shrinker_init(void)
}
return ret;
}
+
+/**
+ * check_buffer() - verify that buffer/offset is safe to access
+ * @alloc: binder_alloc for this proc
+ * @buffer: binder buffer to be accessed
+ * @offset: offset into @buffer data
+ * @bytes: bytes to access from offset
+ *
+ * Check that the @offset/@bytes are within the size of the given
+ * @buffer and that the buffer is currently active and not freeable.
+ * Offsets must also be multiples of sizeof(u32). The kernel is
+ * allowed to touch the buffer in two cases:
+ *
+ * 1) when the buffer is being created:
+ * (buffer->free == 0 && buffer->allow_user_free == 0)
+ * 2) when the buffer is being torn down:
+ * (buffer->free == 0 && buffer->transaction == NULL).
+ *
+ * Return: true if the buffer is safe to access
+ */
+static inline bool check_buffer(struct binder_alloc *alloc,
+ struct binder_buffer *buffer,
+ binder_size_t offset, size_t bytes)
+{
+ size_t buffer_size = binder_alloc_buffer_size(alloc, buffer);
+
+ return buffer_size >= bytes &&
+ offset <= buffer_size - bytes &&
+ IS_ALIGNED(offset, sizeof(u32)) &&
+ !buffer->free &&
+ (!buffer->allow_user_free || !buffer->transaction);
+}
+
+/**
+ * binder_alloc_get_page() - get kernel pointer for given buffer offset
+ * @alloc: binder_alloc for this proc
+ * @buffer: binder buffer to be accessed
+ * @buffer_offset: offset into @buffer data
+ * @pgoffp: address to copy final page offset to
+ *
+ * Lookup the struct page corresponding to the address
+ * at @buffer_offset into @buffer->data. If @pgoffp is not
+ * NULL, the byte-offset into the page is written there.
+ *
+ * The caller is responsible to ensure that the offset points
+ * to a valid address within the @buffer and that @buffer is
+ * not freeable by the user. Since it can't be freed, we are
+ * guaranteed that the corresponding elements of @alloc->pages[]
+ * cannot change.
+ *
+ * Return: struct page
+ */
+static struct page *binder_alloc_get_page(struct binder_alloc *alloc,
+ struct binder_buffer *buffer,
+ binder_size_t buffer_offset,
+ pgoff_t *pgoffp)
+{
+ binder_size_t buffer_space_offset = buffer_offset +
+ (buffer->data - alloc->buffer);
+ pgoff_t pgoff = buffer_space_offset & ~PAGE_MASK;
+ size_t index = buffer_space_offset >> PAGE_SHIFT;
+ struct binder_lru_page *lru_page;
+
+ lru_page = &alloc->pages[index];
+ *pgoffp = pgoff;
+ return lru_page->page_ptr;
+}
+
+/**
+ * binder_alloc_copy_user_to_buffer() - copy src user to tgt user
+ * @alloc: binder_alloc for this proc
+ * @buffer: binder buffer to be accessed
+ * @buffer_offset: offset into @buffer data
+ * @from: userspace pointer to source buffer
+ * @bytes: bytes to copy
+ *
+ * Copy bytes from source userspace to target buffer.
+ *
+ * Return: bytes remaining to be copied
+ */
+unsigned long
+binder_alloc_copy_user_to_buffer(struct binder_alloc *alloc,
+ struct binder_buffer *buffer,
+ binder_size_t buffer_offset,
+ const void __user *from,
+ size_t bytes)
+{
+ if (!check_buffer(alloc, buffer, buffer_offset, bytes))
+ return bytes;
+
+ while (bytes) {
+ unsigned long size;
+ unsigned long ret;
+ struct page *page;
+ pgoff_t pgoff;
+ void *kptr;
+
+ page = binder_alloc_get_page(alloc, buffer,
+ buffer_offset, &pgoff);
+ size = min_t(size_t, bytes, PAGE_SIZE - pgoff);
+ kptr = kmap(page) + pgoff;
+ ret = copy_from_user(kptr, from, size);
+ kunmap(page);
+ if (ret)
+ return bytes - size + ret;
+ bytes -= size;
+ from += size;
+ buffer_offset += size;
+ }
+ return 0;
+}
diff --git a/drivers/android/binder_alloc.h b/drivers/android/binder_alloc.h
index c0aadbbf7f193..995155f31dbd4 100644
--- a/drivers/android/binder_alloc.h
+++ b/drivers/android/binder_alloc.h
@@ -22,6 +22,7 @@
#include <linux/vmalloc.h>
#include <linux/slab.h>
#include <linux/list_lru.h>
+#include <uapi/linux/android/binder.h>
extern struct list_lru binder_alloc_lru;
struct binder_transaction;
@@ -183,5 +184,12 @@ binder_alloc_get_user_buffer_offset(struct binder_alloc *alloc)
return alloc->user_buffer_offset;
}
+unsigned long
+binder_alloc_copy_user_to_buffer(struct binder_alloc *alloc,
+ struct binder_buffer *buffer,
+ binder_size_t buffer_offset,
+ const void __user *from,
+ size_t bytes);
+
#endif /* _LINUX_BINDER_ALLOC_H */
--
2.20.1.791.gb4d0f1c61a-goog
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v3 1/7] binder: create userspace-to-binder-buffer copy function
2019-02-08 18:35 ` [PATCH v3 1/7] binder: create userspace-to-binder-buffer copy function Todd Kjos
@ 2019-02-14 19:45 ` Joel Fernandes
2019-02-14 20:42 ` Todd Kjos
0 siblings, 1 reply; 16+ messages in thread
From: Joel Fernandes @ 2019-02-14 19:45 UTC (permalink / raw)
To: Todd Kjos
Cc: tkjos, gregkh, arve, devel, linux-kernel, maco, joel, kernel-team
Hi Todd,
One quick question:
On Fri, Feb 08, 2019 at 10:35:14AM -0800, Todd Kjos wrote:
> The binder driver uses a vm_area to map the per-process
> binder buffer space. For 32-bit android devices, this is
> now taking too much vmalloc space. This patch removes
> the use of vm_area when copying the transaction data
> from the sender to the buffer space. Instead of using
> copy_from_user() for multi-page copies, it now uses
> binder_alloc_copy_user_to_buffer() which uses kmap()
> and kunmap() to map each page, and uses copy_from_user()
> for copying to that page.
>
> Signed-off-by: Todd Kjos <tkjos@google.com>
> ---
> v2: remove casts as suggested by Dan Carpenter
>
> drivers/android/binder.c | 29 +++++++--
> drivers/android/binder_alloc.c | 113 +++++++++++++++++++++++++++++++++
> drivers/android/binder_alloc.h | 8 +++
> 3 files changed, 143 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index 5f6ef5e63b91e..ab0b3eec363bc 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -3078,8 +3078,12 @@ static void binder_transaction(struct binder_proc *proc,
> ALIGN(tr->data_size, sizeof(void *)));
> offp = off_start;
>
> - if (copy_from_user(t->buffer->data, (const void __user *)(uintptr_t)
> - tr->data.ptr.buffer, tr->data_size)) {
> + if (binder_alloc_copy_user_to_buffer(
> + &target_proc->alloc,
> + t->buffer, 0,
> + (const void __user *)
> + (uintptr_t)tr->data.ptr.buffer,
> + tr->data_size)) {
> binder_user_error("%d:%d got transaction with invalid data ptr\n",
> proc->pid, thread->pid);
> return_error = BR_FAILED_REPLY;
> @@ -3087,8 +3091,13 @@ static void binder_transaction(struct binder_proc *proc,
> return_error_line = __LINE__;
> goto err_copy_data_failed;
> }
> - if (copy_from_user(offp, (const void __user *)(uintptr_t)
> - tr->data.ptr.offsets, tr->offsets_size)) {
> + if (binder_alloc_copy_user_to_buffer(
> + &target_proc->alloc,
> + t->buffer,
> + ALIGN(tr->data_size, sizeof(void *)),
> + (const void __user *)
> + (uintptr_t)tr->data.ptr.offsets,
> + tr->offsets_size)) {
> binder_user_error("%d:%d got transaction with invalid offsets ptr\n",
> proc->pid, thread->pid);
> return_error = BR_FAILED_REPLY;
> @@ -3217,6 +3226,8 @@ static void binder_transaction(struct binder_proc *proc,
> struct binder_buffer_object *bp =
> to_binder_buffer_object(hdr);
> size_t buf_left = sg_buf_end - sg_bufp;
> + binder_size_t sg_buf_offset = (uintptr_t)sg_bufp -
> + (uintptr_t)t->buffer->data;
>
> if (bp->length > buf_left) {
> binder_user_error("%d:%d got transaction with too large buffer\n",
> @@ -3226,9 +3237,13 @@ static void binder_transaction(struct binder_proc *proc,
> return_error_line = __LINE__;
> goto err_bad_offset;
> }
> - if (copy_from_user(sg_bufp,
> - (const void __user *)(uintptr_t)
> - bp->buffer, bp->length)) {
> + if (binder_alloc_copy_user_to_buffer(
> + &target_proc->alloc,
> + t->buffer,
> + sg_buf_offset,
> + (const void __user *)
> + (uintptr_t)bp->buffer,
> + bp->length)) {
> binder_user_error("%d:%d got transaction with invalid offsets ptr\n",
> proc->pid, thread->pid);
> return_error_param = -EFAULT;
> diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
> index 022cd80e80cc3..94c0d85c4e75b 100644
> --- a/drivers/android/binder_alloc.c
> +++ b/drivers/android/binder_alloc.c
> @@ -29,6 +29,8 @@
> #include <linux/list_lru.h>
> #include <linux/ratelimit.h>
> #include <asm/cacheflush.h>
> +#include <linux/uaccess.h>
> +#include <linux/highmem.h>
> #include "binder_alloc.h"
> #include "binder_trace.h"
>
> @@ -1053,3 +1055,114 @@ int binder_alloc_shrinker_init(void)
> }
> return ret;
> }
> +
> +/**
> + * check_buffer() - verify that buffer/offset is safe to access
> + * @alloc: binder_alloc for this proc
> + * @buffer: binder buffer to be accessed
> + * @offset: offset into @buffer data
> + * @bytes: bytes to access from offset
> + *
> + * Check that the @offset/@bytes are within the size of the given
> + * @buffer and that the buffer is currently active and not freeable.
> + * Offsets must also be multiples of sizeof(u32). The kernel is
In all callers of binder_alloc_copy_user_to_buffer, the alignment of offsets
is set to sizeof(void *). Then shouldn't this function check for sizeof(void *)
alignment instead of u32?
thanks,
- Joel
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v3 1/7] binder: create userspace-to-binder-buffer copy function
2019-02-14 19:45 ` Joel Fernandes
@ 2019-02-14 20:42 ` Todd Kjos
2019-02-14 20:53 ` Joel Fernandes
0 siblings, 1 reply; 16+ messages in thread
From: Todd Kjos @ 2019-02-14 20:42 UTC (permalink / raw)
To: Joel Fernandes
Cc: Todd Kjos, Greg Kroah-Hartman, Arve Hjønnevåg,
open list:ANDROID DRIVERS, LKML, Martijn Coenen, joel,
Android Kernel Team
On Thu, Feb 14, 2019 at 11:45 AM Joel Fernandes <joelaf@google.com> wrote:
>
> Hi Todd,
>
> One quick question:
>
> On Fri, Feb 08, 2019 at 10:35:14AM -0800, Todd Kjos wrote:
> > The binder driver uses a vm_area to map the per-process
> > binder buffer space. For 32-bit android devices, this is
> > now taking too much vmalloc space. This patch removes
> > the use of vm_area when copying the transaction data
> > from the sender to the buffer space. Instead of using
> > copy_from_user() for multi-page copies, it now uses
> > binder_alloc_copy_user_to_buffer() which uses kmap()
> > and kunmap() to map each page, and uses copy_from_user()
> > for copying to that page.
> >
> > Signed-off-by: Todd Kjos <tkjos@google.com>
> > ---
> > v2: remove casts as suggested by Dan Carpenter
> >
> > drivers/android/binder.c | 29 +++++++--
> > drivers/android/binder_alloc.c | 113 +++++++++++++++++++++++++++++++++
> > drivers/android/binder_alloc.h | 8 +++
> > 3 files changed, 143 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> > index 5f6ef5e63b91e..ab0b3eec363bc 100644
> > --- a/drivers/android/binder.c
> > +++ b/drivers/android/binder.c
> > @@ -3078,8 +3078,12 @@ static void binder_transaction(struct binder_proc *proc,
> > ALIGN(tr->data_size, sizeof(void *)));
> > offp = off_start;
> >
> > - if (copy_from_user(t->buffer->data, (const void __user *)(uintptr_t)
> > - tr->data.ptr.buffer, tr->data_size)) {
> > + if (binder_alloc_copy_user_to_buffer(
> > + &target_proc->alloc,
> > + t->buffer, 0,
> > + (const void __user *)
> > + (uintptr_t)tr->data.ptr.buffer,
> > + tr->data_size)) {
> > binder_user_error("%d:%d got transaction with invalid data ptr\n",
> > proc->pid, thread->pid);
> > return_error = BR_FAILED_REPLY;
> > @@ -3087,8 +3091,13 @@ static void binder_transaction(struct binder_proc *proc,
> > return_error_line = __LINE__;
> > goto err_copy_data_failed;
> > }
> > - if (copy_from_user(offp, (const void __user *)(uintptr_t)
> > - tr->data.ptr.offsets, tr->offsets_size)) {
> > + if (binder_alloc_copy_user_to_buffer(
> > + &target_proc->alloc,
> > + t->buffer,
> > + ALIGN(tr->data_size, sizeof(void *)),
> > + (const void __user *)
> > + (uintptr_t)tr->data.ptr.offsets,
> > + tr->offsets_size)) {
> > binder_user_error("%d:%d got transaction with invalid offsets ptr\n",
> > proc->pid, thread->pid);
> > return_error = BR_FAILED_REPLY;
> > @@ -3217,6 +3226,8 @@ static void binder_transaction(struct binder_proc *proc,
> > struct binder_buffer_object *bp =
> > to_binder_buffer_object(hdr);
> > size_t buf_left = sg_buf_end - sg_bufp;
> > + binder_size_t sg_buf_offset = (uintptr_t)sg_bufp -
> > + (uintptr_t)t->buffer->data;
> >
> > if (bp->length > buf_left) {
> > binder_user_error("%d:%d got transaction with too large buffer\n",
> > @@ -3226,9 +3237,13 @@ static void binder_transaction(struct binder_proc *proc,
> > return_error_line = __LINE__;
> > goto err_bad_offset;
> > }
> > - if (copy_from_user(sg_bufp,
> > - (const void __user *)(uintptr_t)
> > - bp->buffer, bp->length)) {
> > + if (binder_alloc_copy_user_to_buffer(
> > + &target_proc->alloc,
> > + t->buffer,
> > + sg_buf_offset,
> > + (const void __user *)
> > + (uintptr_t)bp->buffer,
> > + bp->length)) {
> > binder_user_error("%d:%d got transaction with invalid offsets ptr\n",
> > proc->pid, thread->pid);
> > return_error_param = -EFAULT;
> > diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
> > index 022cd80e80cc3..94c0d85c4e75b 100644
> > --- a/drivers/android/binder_alloc.c
> > +++ b/drivers/android/binder_alloc.c
> > @@ -29,6 +29,8 @@
> > #include <linux/list_lru.h>
> > #include <linux/ratelimit.h>
> > #include <asm/cacheflush.h>
> > +#include <linux/uaccess.h>
> > +#include <linux/highmem.h>
> > #include "binder_alloc.h"
> > #include "binder_trace.h"
> >
> > @@ -1053,3 +1055,114 @@ int binder_alloc_shrinker_init(void)
> > }
> > return ret;
> > }
> > +
> > +/**
> > + * check_buffer() - verify that buffer/offset is safe to access
> > + * @alloc: binder_alloc for this proc
> > + * @buffer: binder buffer to be accessed
> > + * @offset: offset into @buffer data
> > + * @bytes: bytes to access from offset
> > + *
> > + * Check that the @offset/@bytes are within the size of the given
> > + * @buffer and that the buffer is currently active and not freeable.
> > + * Offsets must also be multiples of sizeof(u32). The kernel is
>
> In all callers of binder_alloc_copy_user_to_buffer, the alignment of offsets
> is set to sizeof(void *). Then shouldn't this function check for sizeof(void *)
> alignment instead of u32?
But there are other callers of check_buffer() later in the series that
don't require pointer-size alignment. u32 alignment is consistent with
the alignment requirements of the binder driver before this change.
The copy functions don't actually need to insist on alignment, but
these binder buffer objects have always used u32 alignment which has
been checked in the driver. If user code misaligned it, then errors
are returned. The alignment checks are really to be consistent with
previous binder driver behavior.
-Todd
>
> thanks,
>
> - Joel
>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v3 1/7] binder: create userspace-to-binder-buffer copy function
2019-02-14 20:42 ` Todd Kjos
@ 2019-02-14 20:53 ` Joel Fernandes
2019-02-14 21:25 ` Joel Fernandes
0 siblings, 1 reply; 16+ messages in thread
From: Joel Fernandes @ 2019-02-14 20:53 UTC (permalink / raw)
To: Todd Kjos
Cc: Todd Kjos, Greg Kroah-Hartman, Arve Hjønnevåg,
open list:ANDROID DRIVERS, LKML, Martijn Coenen,
Joel Fernandes (Google),
Android Kernel Team
On Thu, Feb 14, 2019 at 3:42 PM Todd Kjos <tkjos@google.com> wrote:
>
> On Thu, Feb 14, 2019 at 11:45 AM Joel Fernandes <joelaf@google.com> wrote:
[snip]
> > > + * check_buffer() - verify that buffer/offset is safe to access
> > > + * @alloc: binder_alloc for this proc
> > > + * @buffer: binder buffer to be accessed
> > > + * @offset: offset into @buffer data
> > > + * @bytes: bytes to access from offset
> > > + *
> > > + * Check that the @offset/@bytes are within the size of the given
> > > + * @buffer and that the buffer is currently active and not freeable.
> > > + * Offsets must also be multiples of sizeof(u32). The kernel is
> >
> > In all callers of binder_alloc_copy_user_to_buffer, the alignment of offsets
> > is set to sizeof(void *). Then shouldn't this function check for sizeof(void *)
> > alignment instead of u32?
>
> But there are other callers of check_buffer() later in the series that
> don't require pointer-size alignment. u32 alignment is consistent with
> the alignment requirements of the binder driver before this change.
> The copy functions don't actually need to insist on alignment, but
> these binder buffer objects have always used u32 alignment which has
> been checked in the driver. If user code misaligned it, then errors
> are returned. The alignment checks are really to be consistent with
> previous binder driver behavior.
Got it, thanks.
- Joel
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 1/7] binder: create userspace-to-binder-buffer copy function
2019-02-14 20:53 ` Joel Fernandes
@ 2019-02-14 21:25 ` Joel Fernandes
2019-02-14 21:55 ` Todd Kjos
0 siblings, 1 reply; 16+ messages in thread
From: Joel Fernandes @ 2019-02-14 21:25 UTC (permalink / raw)
To: Todd Kjos
Cc: Todd Kjos, Greg Kroah-Hartman, Arve Hjønnevåg,
open list:ANDROID DRIVERS, LKML, Martijn Coenen,
Joel Fernandes (Google),
Android Kernel Team
On Thu, Feb 14, 2019 at 03:53:54PM -0500, Joel Fernandes wrote:
> On Thu, Feb 14, 2019 at 3:42 PM Todd Kjos <tkjos@google.com> wrote:
> >
> > On Thu, Feb 14, 2019 at 11:45 AM Joel Fernandes <joelaf@google.com> wrote:
> [snip]
> > > > + * check_buffer() - verify that buffer/offset is safe to access
> > > > + * @alloc: binder_alloc for this proc
> > > > + * @buffer: binder buffer to be accessed
> > > > + * @offset: offset into @buffer data
> > > > + * @bytes: bytes to access from offset
> > > > + *
> > > > + * Check that the @offset/@bytes are within the size of the given
> > > > + * @buffer and that the buffer is currently active and not freeable.
> > > > + * Offsets must also be multiples of sizeof(u32). The kernel is
> > >
> > > In all callers of binder_alloc_copy_user_to_buffer, the alignment of offsets
> > > is set to sizeof(void *). Then shouldn't this function check for sizeof(void *)
> > > alignment instead of u32?
> >
> > But there are other callers of check_buffer() later in the series that
> > don't require pointer-size alignment. u32 alignment is consistent with
> > the alignment requirements of the binder driver before this change.
> > The copy functions don't actually need to insist on alignment, but
> > these binder buffer objects have always used u32 alignment which has
> > been checked in the driver. If user code misaligned it, then errors
> > are returned. The alignment checks are really to be consistent with
> > previous binder driver behavior.
>
> Got it, thanks.
One more thing I wanted to ask is, kmap() will now cause global lock
contention because of using spin_lock due to kmap_high().
Previously the binder driver was made to not use global lock (as you had
done). Now these paths will start global locking on 32-bit architectures.
Would that degrade performance?
Are we not using kmap_atomic() in this patch because of any concern that the
kmap fixmap space is limited and may run out?
thanks,
- Joel
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 1/7] binder: create userspace-to-binder-buffer copy function
2019-02-14 21:25 ` Joel Fernandes
@ 2019-02-14 21:55 ` Todd Kjos
2019-02-14 22:07 ` Joel Fernandes
0 siblings, 1 reply; 16+ messages in thread
From: Todd Kjos @ 2019-02-14 21:55 UTC (permalink / raw)
To: Joel Fernandes
Cc: Todd Kjos, Greg Kroah-Hartman, Arve Hjønnevåg,
open list:ANDROID DRIVERS, LKML, Martijn Coenen,
Joel Fernandes (Google),
Android Kernel Team
On Thu, Feb 14, 2019 at 1:25 PM Joel Fernandes <joelaf@google.com> wrote:
>
> On Thu, Feb 14, 2019 at 03:53:54PM -0500, Joel Fernandes wrote:
> > On Thu, Feb 14, 2019 at 3:42 PM Todd Kjos <tkjos@google.com> wrote:
> > >
> > > On Thu, Feb 14, 2019 at 11:45 AM Joel Fernandes <joelaf@google.com> wrote:
> > [snip]
> > > > > + * check_buffer() - verify that buffer/offset is safe to access
> > > > > + * @alloc: binder_alloc for this proc
> > > > > + * @buffer: binder buffer to be accessed
> > > > > + * @offset: offset into @buffer data
> > > > > + * @bytes: bytes to access from offset
> > > > > + *
> > > > > + * Check that the @offset/@bytes are within the size of the given
> > > > > + * @buffer and that the buffer is currently active and not freeable.
> > > > > + * Offsets must also be multiples of sizeof(u32). The kernel is
> > > >
> > > > In all callers of binder_alloc_copy_user_to_buffer, the alignment of offsets
> > > > is set to sizeof(void *). Then shouldn't this function check for sizeof(void *)
> > > > alignment instead of u32?
> > >
> > > But there are other callers of check_buffer() later in the series that
> > > don't require pointer-size alignment. u32 alignment is consistent with
> > > the alignment requirements of the binder driver before this change.
> > > The copy functions don't actually need to insist on alignment, but
> > > these binder buffer objects have always used u32 alignment which has
> > > been checked in the driver. If user code misaligned it, then errors
> > > are returned. The alignment checks are really to be consistent with
> > > previous binder driver behavior.
> >
> > Got it, thanks.
>
> One more thing I wanted to ask is, kmap() will now cause global lock
> contention because of using spin_lock due to kmap_high().
>
> Previously the binder driver was made to not use global lock (as you had
> done). Now these paths will start global locking on 32-bit architectures.
> Would that degrade performance?
There was a lot of concern about 32-bit performance both for
lock-contention and the cost of map/unmap operations. Of course,
32-bit systems are also where the primary win is -- namely avoiding
vmalloc space depletion. So there was a several months-long evaluation
period on 32-bit devices by a silicon vendor who did a lot of testing
across a broad set of benchmarks / workloads to verify the performance
costs are acceptable. We also ran tests to try to exhaust the kmap
space with multiple large buffers.
The testing did find that there is some performance degradation for
large buffer transfers, but there are no cases where this
significantly impacted a meaningful user workload.
>
> Are we not using kmap_atomic() in this patch because of any concern that the
> kmap fixmap space is limited and may run out?
We're not using the atomic version here since we can't guarantee that
the loop will be atomic since we are calling copy_from_user(). Later
in the series, other cases do use kmap_atomic().
>
> thanks,
>
> - Joel
>
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 1/7] binder: create userspace-to-binder-buffer copy function
2019-02-14 21:55 ` Todd Kjos
@ 2019-02-14 22:07 ` Joel Fernandes
0 siblings, 0 replies; 16+ messages in thread
From: Joel Fernandes @ 2019-02-14 22:07 UTC (permalink / raw)
To: Todd Kjos
Cc: Todd Kjos, Greg Kroah-Hartman, Arve Hjønnevåg,
open list:ANDROID DRIVERS, LKML, Martijn Coenen,
Joel Fernandes (Google),
Android Kernel Team
On Thu, Feb 14, 2019 at 01:55:19PM -0800, 'Todd Kjos' via kernel-team wrote:
> On Thu, Feb 14, 2019 at 1:25 PM Joel Fernandes <joelaf@google.com> wrote:
> >
> > On Thu, Feb 14, 2019 at 03:53:54PM -0500, Joel Fernandes wrote:
> > > On Thu, Feb 14, 2019 at 3:42 PM Todd Kjos <tkjos@google.com> wrote:
> > > >
> > > > On Thu, Feb 14, 2019 at 11:45 AM Joel Fernandes <joelaf@google.com> wrote:
> > > [snip]
> > > > > > + * check_buffer() - verify that buffer/offset is safe to access
> > > > > > + * @alloc: binder_alloc for this proc
> > > > > > + * @buffer: binder buffer to be accessed
> > > > > > + * @offset: offset into @buffer data
> > > > > > + * @bytes: bytes to access from offset
> > > > > > + *
> > > > > > + * Check that the @offset/@bytes are within the size of the given
> > > > > > + * @buffer and that the buffer is currently active and not freeable.
> > > > > > + * Offsets must also be multiples of sizeof(u32). The kernel is
> > > > >
> > > > > In all callers of binder_alloc_copy_user_to_buffer, the alignment of offsets
> > > > > is set to sizeof(void *). Then shouldn't this function check for sizeof(void *)
> > > > > alignment instead of u32?
> > > >
> > > > But there are other callers of check_buffer() later in the series that
> > > > don't require pointer-size alignment. u32 alignment is consistent with
> > > > the alignment requirements of the binder driver before this change.
> > > > The copy functions don't actually need to insist on alignment, but
> > > > these binder buffer objects have always used u32 alignment which has
> > > > been checked in the driver. If user code misaligned it, then errors
> > > > are returned. The alignment checks are really to be consistent with
> > > > previous binder driver behavior.
> > >
> > > Got it, thanks.
> >
> > One more thing I wanted to ask is, kmap() will now cause global lock
> > contention because of using spin_lock due to kmap_high().
> >
> > Previously the binder driver was made to not use global lock (as you had
> > done). Now these paths will start global locking on 32-bit architectures.
> > Would that degrade performance?
>
> There was a lot of concern about 32-bit performance both for
> lock-contention and the cost of map/unmap operations. Of course,
> 32-bit systems are also where the primary win is -- namely avoiding
> vmalloc space depletion. So there was a several months-long evaluation
> period on 32-bit devices by a silicon vendor who did a lot of testing
> across a broad set of benchmarks / workloads to verify the performance
> costs are acceptable. We also ran tests to try to exhaust the kmap
> space with multiple large buffers.
>
> The testing did find that there is some performance degradation for
> large buffer transfers, but there are no cases where this
> significantly impacted a meaningful user workload.
>
> >
> > Are we not using kmap_atomic() in this patch because of any concern that the
> > kmap fixmap space is limited and may run out?
>
> We're not using the atomic version here since we can't guarantee that
> the loop will be atomic since we are calling copy_from_user(). Later
> in the series, other cases do use kmap_atomic().
Got it, thanks for all the clarifications,
- Joel
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v3 2/7] binder: add functions to copy to/from binder buffers
2019-02-08 18:35 [PATCH v3 0/7] binder: eliminate use of vmalloc space for binder buffers Todd Kjos
2019-02-08 18:35 ` [PATCH v3 1/7] binder: create userspace-to-binder-buffer copy function Todd Kjos
@ 2019-02-08 18:35 ` Todd Kjos
2019-02-08 18:35 ` [PATCH v3 3/7] binder: add function to copy binder object from buffer Todd Kjos
` (5 subsequent siblings)
7 siblings, 0 replies; 16+ messages in thread
From: Todd Kjos @ 2019-02-08 18:35 UTC (permalink / raw)
To: tkjos, gregkh, arve, devel, linux-kernel, maco; +Cc: joel, kernel-team
Avoid vm_area when copying to or from binder buffers.
Instead, new copy functions are added that copy from
kernel space to binder buffer space. These use
kmap_atomic() and kunmap_atomic() to create temporary
mappings and then memcpy() is used to copy within
that page.
Also, kmap_atomic() / kunmap_atomic() use the appropriate
cache flushing to support VIVT cache architectures.
Allow binder to build if CPU_CACHE_VIVT is defined.
Several uses of the new functions are added here. More
to follow in subsequent patches.
Signed-off-by: Todd Kjos <tkjos@google.com>
---
v2: remove casts as suggested by Dan Carpenter
drivers/android/Kconfig | 2 +-
drivers/android/binder.c | 119 +++++++++++++++++++++------------
drivers/android/binder_alloc.c | 59 ++++++++++++++++
drivers/android/binder_alloc.h | 12 ++++
4 files changed, 147 insertions(+), 45 deletions(-)
diff --git a/drivers/android/Kconfig b/drivers/android/Kconfig
index 4c190f8d1f4c6..6fdf2abe4598a 100644
--- a/drivers/android/Kconfig
+++ b/drivers/android/Kconfig
@@ -10,7 +10,7 @@ if ANDROID
config ANDROID_BINDER_IPC
bool "Android Binder IPC Driver"
- depends on MMU && !CPU_CACHE_VIVT
+ depends on MMU
default n
---help---
Binder is used in Android for both communication between processes,
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index ab0b3eec363bc..74d0c1ff874e2 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -2244,14 +2244,22 @@ static void binder_transaction_buffer_release(struct binder_proc *proc,
off_end = (void *)off_start + buffer->offsets_size;
for (offp = off_start; offp < off_end; offp++) {
struct binder_object_header *hdr;
- size_t object_size = binder_validate_object(buffer, *offp);
-
+ size_t object_size;
+ binder_size_t object_offset;
+ binder_size_t buffer_offset = (uintptr_t)offp -
+ (uintptr_t)buffer->data;
+
+ binder_alloc_copy_from_buffer(&proc->alloc, &object_offset,
+ buffer, buffer_offset,
+ sizeof(object_offset));
+ object_size = binder_validate_object(buffer, object_offset);
if (object_size == 0) {
pr_err("transaction release %d bad object at offset %lld, size %zd\n",
- debug_id, (u64)*offp, buffer->data_size);
+ debug_id, (u64)object_offset, buffer->data_size);
continue;
}
- hdr = (struct binder_object_header *)(buffer->data + *offp);
+ hdr = (struct binder_object_header *)
+ (buffer->data + object_offset);
switch (hdr->type) {
case BINDER_TYPE_BINDER:
case BINDER_TYPE_WEAK_BINDER: {
@@ -2359,8 +2367,20 @@ static void binder_transaction_buffer_release(struct binder_proc *proc,
continue;
}
fd_array = (u32 *)(parent_buffer + (uintptr_t)fda->parent_offset);
- for (fd_index = 0; fd_index < fda->num_fds; fd_index++)
- binder_deferred_fd_close(fd_array[fd_index]);
+ for (fd_index = 0; fd_index < fda->num_fds;
+ fd_index++) {
+ u32 fd;
+ binder_size_t offset =
+ (uintptr_t)&fd_array[fd_index] -
+ (uintptr_t)buffer->data;
+
+ binder_alloc_copy_from_buffer(&proc->alloc,
+ &fd,
+ buffer,
+ offset,
+ sizeof(fd));
+ binder_deferred_fd_close(fd);
+ }
} break;
default:
pr_err("transaction release %d bad object type %x\n",
@@ -2496,7 +2516,7 @@ static int binder_translate_handle(struct flat_binder_object *fp,
return ret;
}
-static int binder_translate_fd(u32 *fdp,
+static int binder_translate_fd(u32 fd, binder_size_t fd_offset,
struct binder_transaction *t,
struct binder_thread *thread,
struct binder_transaction *in_reply_to)
@@ -2507,7 +2527,6 @@ static int binder_translate_fd(u32 *fdp,
struct file *file;
int ret = 0;
bool target_allows_fd;
- int fd = *fdp;
if (in_reply_to)
target_allows_fd = !!(in_reply_to->flags & TF_ACCEPT_FDS);
@@ -2546,7 +2565,7 @@ static int binder_translate_fd(u32 *fdp,
goto err_alloc;
}
fixup->file = file;
- fixup->offset = (uintptr_t)fdp - (uintptr_t)t->buffer->data;
+ fixup->offset = fd_offset;
trace_binder_transaction_fd_send(t, fd, fixup->offset);
list_add_tail(&fixup->fixup_entry, &t->fd_fixups);
@@ -2598,8 +2617,17 @@ static int binder_translate_fd_array(struct binder_fd_array_object *fda,
return -EINVAL;
}
for (fdi = 0; fdi < fda->num_fds; fdi++) {
- int ret = binder_translate_fd(&fd_array[fdi], t, thread,
- in_reply_to);
+ u32 fd;
+ int ret;
+ binder_size_t offset =
+ (uintptr_t)&fd_array[fdi] -
+ (uintptr_t)t->buffer->data;
+
+ binder_alloc_copy_from_buffer(&target_proc->alloc,
+ &fd, t->buffer,
+ offset, sizeof(fd));
+ ret = binder_translate_fd(fd, offset, t, thread,
+ in_reply_to);
if (ret < 0)
return ret;
}
@@ -3066,7 +3094,9 @@ static void binder_transaction(struct binder_proc *proc,
t->security_ctx = (uintptr_t)kptr +
binder_alloc_get_user_buffer_offset(&target_proc->alloc);
- memcpy(kptr, secctx, secctx_sz);
+ binder_alloc_copy_to_buffer(&target_proc->alloc,
+ t->buffer, buf_offset,
+ secctx, secctx_sz);
security_release_secctx(secctx, secctx_sz);
secctx = NULL;
}
@@ -3128,11 +3158,21 @@ static void binder_transaction(struct binder_proc *proc,
off_min = 0;
for (; offp < off_end; offp++) {
struct binder_object_header *hdr;
- size_t object_size = binder_validate_object(t->buffer, *offp);
-
- if (object_size == 0 || *offp < off_min) {
+ size_t object_size;
+ binder_size_t object_offset;
+ binder_size_t buffer_offset =
+ (uintptr_t)offp - (uintptr_t)t->buffer->data;
+
+ binder_alloc_copy_from_buffer(&target_proc->alloc,
+ &object_offset,
+ t->buffer,
+ buffer_offset,
+ sizeof(object_offset));
+ object_size = binder_validate_object(t->buffer, object_offset);
+ if (object_size == 0 || object_offset < off_min) {
binder_user_error("%d:%d got transaction with invalid offset (%lld, min %lld max %lld) or object.\n",
- proc->pid, thread->pid, (u64)*offp,
+ proc->pid, thread->pid,
+ (u64)object_offset,
(u64)off_min,
(u64)t->buffer->data_size);
return_error = BR_FAILED_REPLY;
@@ -3141,8 +3181,9 @@ static void binder_transaction(struct binder_proc *proc,
goto err_bad_offset;
}
- hdr = (struct binder_object_header *)(t->buffer->data + *offp);
- off_min = *offp + object_size;
+ hdr = (struct binder_object_header *)
+ (t->buffer->data + object_offset);
+ off_min = object_offset + object_size;
switch (hdr->type) {
case BINDER_TYPE_BINDER:
case BINDER_TYPE_WEAK_BINDER: {
@@ -3173,8 +3214,10 @@ static void binder_transaction(struct binder_proc *proc,
case BINDER_TYPE_FD: {
struct binder_fd_object *fp = to_binder_fd_object(hdr);
- int ret = binder_translate_fd(&fp->fd, t, thread,
- in_reply_to);
+ 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);
if (ret < 0) {
return_error = BR_FAILED_REPLY;
@@ -3967,6 +4010,7 @@ static int binder_wait_for_work(struct binder_thread *thread,
/**
* binder_apply_fd_fixups() - finish fd translation
+ * @proc: binder_proc associated @t->buffer
* @t: binder transaction with list of fd fixups
*
* Now that we are in the context of the transaction target
@@ -3978,14 +4022,14 @@ static int binder_wait_for_work(struct binder_thread *thread,
* fput'ing files that have not been processed and ksys_close'ing
* any fds that have already been allocated.
*/
-static int binder_apply_fd_fixups(struct binder_transaction *t)
+static int binder_apply_fd_fixups(struct binder_proc *proc,
+ struct binder_transaction *t)
{
struct binder_txn_fd_fixup *fixup, *tmp;
int ret = 0;
list_for_each_entry(fixup, &t->fd_fixups, fixup_entry) {
int fd = get_unused_fd_flags(O_CLOEXEC);
- u32 *fdp;
if (fd < 0) {
binder_debug(BINDER_DEBUG_TRANSACTION,
@@ -4000,33 +4044,20 @@ static int binder_apply_fd_fixups(struct binder_transaction *t)
trace_binder_transaction_fd_recv(t, fd, fixup->offset);
fd_install(fd, fixup->file);
fixup->file = NULL;
- fdp = (u32 *)(t->buffer->data + fixup->offset);
- /*
- * This store can cause problems for CPUs with a
- * VIVT cache (eg ARMv5) since the cache cannot
- * detect virtual aliases to the same physical cacheline.
- * To support VIVT, this address and the user-space VA
- * would both need to be flushed. Since this kernel
- * VA is not constructed via page_to_virt(), we can't
- * use flush_dcache_page() on it, so we'd have to use
- * an internal function. If devices with VIVT ever
- * need to run Android, we'll either need to go back
- * to patching the translated fd from the sender side
- * (using the non-standard kernel functions), or rework
- * how the kernel uses the buffer to use page_to_virt()
- * addresses instead of allocating in our own vm area.
- *
- * For now, we disable compilation if CONFIG_CPU_CACHE_VIVT.
- */
- *fdp = fd;
+ binder_alloc_copy_to_buffer(&proc->alloc, t->buffer,
+ fixup->offset, &fd,
+ sizeof(u32));
}
list_for_each_entry_safe(fixup, tmp, &t->fd_fixups, fixup_entry) {
if (fixup->file) {
fput(fixup->file);
} else if (ret) {
- u32 *fdp = (u32 *)(t->buffer->data + fixup->offset);
+ u32 fd;
- binder_deferred_fd_close(*fdp);
+ binder_alloc_copy_from_buffer(&proc->alloc, &fd,
+ t->buffer, fixup->offset,
+ sizeof(fd));
+ binder_deferred_fd_close(fd);
}
list_del(&fixup->fixup_entry);
kfree(fixup);
@@ -4324,7 +4355,7 @@ static int binder_thread_read(struct binder_proc *proc,
trd->sender_pid = 0;
}
- ret = binder_apply_fd_fixups(t);
+ ret = binder_apply_fd_fixups(proc, t);
if (ret) {
struct binder_buffer *buffer = t->buffer;
bool oneway = !!(t->flags & TF_ONE_WAY);
diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
index 94c0d85c4e75b..2eebff4be83e0 100644
--- a/drivers/android/binder_alloc.c
+++ b/drivers/android/binder_alloc.c
@@ -1166,3 +1166,62 @@ binder_alloc_copy_user_to_buffer(struct binder_alloc *alloc,
}
return 0;
}
+
+static void binder_alloc_do_buffer_copy(struct binder_alloc *alloc,
+ bool to_buffer,
+ struct binder_buffer *buffer,
+ binder_size_t buffer_offset,
+ void *ptr,
+ size_t bytes)
+{
+ /* All copies must be 32-bit aligned and 32-bit size */
+ BUG_ON(!check_buffer(alloc, buffer, buffer_offset, bytes));
+
+ while (bytes) {
+ unsigned long size;
+ struct page *page;
+ pgoff_t pgoff;
+ void *tmpptr;
+ void *base_ptr;
+
+ page = binder_alloc_get_page(alloc, buffer,
+ buffer_offset, &pgoff);
+ size = min_t(size_t, bytes, PAGE_SIZE - pgoff);
+ base_ptr = kmap_atomic(page);
+ tmpptr = base_ptr + pgoff;
+ if (to_buffer)
+ memcpy(tmpptr, ptr, size);
+ else
+ memcpy(ptr, tmpptr, size);
+ /*
+ * kunmap_atomic() takes care of flushing the cache
+ * if this device has VIVT cache arch
+ */
+ kunmap_atomic(base_ptr);
+ bytes -= size;
+ pgoff = 0;
+ ptr = ptr + size;
+ buffer_offset += size;
+ }
+}
+
+void binder_alloc_copy_to_buffer(struct binder_alloc *alloc,
+ struct binder_buffer *buffer,
+ binder_size_t buffer_offset,
+ void *src,
+ size_t bytes)
+{
+ binder_alloc_do_buffer_copy(alloc, true, buffer, buffer_offset,
+ src, bytes);
+}
+
+void binder_alloc_copy_from_buffer(struct binder_alloc *alloc,
+ void *dest,
+ struct binder_buffer *buffer,
+ binder_size_t buffer_offset,
+ size_t bytes)
+{
+ binder_alloc_do_buffer_copy(alloc, false, buffer, buffer_offset,
+ dest, bytes);
+}
+
diff --git a/drivers/android/binder_alloc.h b/drivers/android/binder_alloc.h
index 995155f31dbd4..9d682b9d6c241 100644
--- a/drivers/android/binder_alloc.h
+++ b/drivers/android/binder_alloc.h
@@ -191,5 +191,17 @@ binder_alloc_copy_user_to_buffer(struct binder_alloc *alloc,
const void __user *from,
size_t bytes);
+void binder_alloc_copy_to_buffer(struct binder_alloc *alloc,
+ struct binder_buffer *buffer,
+ binder_size_t buffer_offset,
+ void *src,
+ size_t bytes);
+
+void binder_alloc_copy_from_buffer(struct binder_alloc *alloc,
+ void *dest,
+ struct binder_buffer *buffer,
+ binder_size_t buffer_offset,
+ size_t bytes);
+
#endif /* _LINUX_BINDER_ALLOC_H */
--
2.20.1.791.gb4d0f1c61a-goog
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH v3 3/7] binder: add function to copy binder object from buffer
2019-02-08 18:35 [PATCH v3 0/7] binder: eliminate use of vmalloc space for binder buffers Todd Kjos
2019-02-08 18:35 ` [PATCH v3 1/7] binder: create userspace-to-binder-buffer copy function Todd Kjos
2019-02-08 18:35 ` [PATCH v3 2/7] binder: add functions to copy to/from binder buffers Todd Kjos
@ 2019-02-08 18:35 ` Todd Kjos
2019-02-08 18:35 ` [PATCH v3 4/7] binder: avoid kernel vm_area for buffer fixups Todd Kjos
` (4 subsequent siblings)
7 siblings, 0 replies; 16+ messages in thread
From: Todd Kjos @ 2019-02-08 18:35 UTC (permalink / raw)
To: tkjos, gregkh, arve, devel, linux-kernel, maco; +Cc: joel, kernel-team
When creating or tearing down a transaction, the binder driver
examines objects in the buffer and takes appropriate action.
To do this without needing to dereference pointers into the
buffer, the local copies of the objects are needed. This patch
introduces a function to validate and copy binder objects
from the buffer to a local structure.
Signed-off-by: Todd Kjos <tkjos@google.com>
---
v2: remove casts as suggested by Dan Carpenter
drivers/android/binder.c | 75 +++++++++++++++++++++++++++++++---------
1 file changed, 58 insertions(+), 17 deletions(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 74d0c1ff874e2..8063b405e4fa1 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -628,6 +628,26 @@ struct binder_transaction {
spinlock_t lock;
};
+/**
+ * struct binder_object - union of flat binder object types
+ * @hdr: generic object header
+ * @fbo: binder object (nodes and refs)
+ * @fdo: file descriptor object
+ * @bbo: binder buffer pointer
+ * @fdao: file descriptor array
+ *
+ * Used for type-independent object copies
+ */
+struct binder_object {
+ union {
+ struct binder_object_header hdr;
+ struct flat_binder_object fbo;
+ struct binder_fd_object fdo;
+ struct binder_buffer_object bbo;
+ struct binder_fd_array_object fdao;
+ };
+};
+
/**
* binder_proc_lock() - Acquire outer lock for given binder_proc
* @proc: struct binder_proc to acquire
@@ -2017,26 +2037,33 @@ static void binder_cleanup_transaction(struct binder_transaction *t,
}
/**
- * binder_validate_object() - checks for a valid metadata object in a buffer.
+ * binder_get_object() - gets object and checks for valid metadata
+ * @proc: binder_proc owning the buffer
* @buffer: binder_buffer that we're parsing.
- * @offset: offset in the buffer at which to validate an object.
+ * @offset: offset in the @buffer at which to validate an object.
+ * @object: struct binder_object to read into
*
* Return: If there's a valid metadata object at @offset in @buffer, the
- * size of that object. Otherwise, it returns zero.
+ * size of that object. Otherwise, it returns zero. The object
+ * is read into the struct binder_object pointed to by @object.
*/
-static size_t binder_validate_object(struct binder_buffer *buffer, u64 offset)
+static size_t binder_get_object(struct binder_proc *proc,
+ struct binder_buffer *buffer,
+ unsigned long offset,
+ struct binder_object *object)
{
- /* Check if we can read a header first */
+ size_t read_size;
struct binder_object_header *hdr;
size_t object_size = 0;
- if (buffer->data_size < sizeof(*hdr) ||
- offset > buffer->data_size - sizeof(*hdr) ||
- !IS_ALIGNED(offset, sizeof(u32)))
+ read_size = min_t(size_t, sizeof(*object), buffer->data_size - offset);
+ if (read_size < sizeof(*hdr))
return 0;
+ binder_alloc_copy_from_buffer(&proc->alloc, object, buffer,
+ offset, read_size);
- /* Ok, now see if we can read a complete object. */
- hdr = (struct binder_object_header *)(buffer->data + offset);
+ /* Ok, now see if we read a complete object. */
+ hdr = &object->hdr;
switch (hdr->type) {
case BINDER_TYPE_BINDER:
case BINDER_TYPE_WEAK_BINDER:
@@ -2245,6 +2272,7 @@ static void binder_transaction_buffer_release(struct binder_proc *proc,
for (offp = off_start; offp < off_end; offp++) {
struct binder_object_header *hdr;
size_t object_size;
+ struct binder_object object;
binder_size_t object_offset;
binder_size_t buffer_offset = (uintptr_t)offp -
(uintptr_t)buffer->data;
@@ -2252,14 +2280,14 @@ static void binder_transaction_buffer_release(struct binder_proc *proc,
binder_alloc_copy_from_buffer(&proc->alloc, &object_offset,
buffer, buffer_offset,
sizeof(object_offset));
- object_size = binder_validate_object(buffer, object_offset);
+ object_size = binder_get_object(proc, buffer,
+ object_offset, &object);
if (object_size == 0) {
pr_err("transaction release %d bad object at offset %lld, size %zd\n",
debug_id, (u64)object_offset, buffer->data_size);
continue;
}
- hdr = (struct binder_object_header *)
- (buffer->data + object_offset);
+ hdr = &object.hdr;
switch (hdr->type) {
case BINDER_TYPE_BINDER:
case BINDER_TYPE_WEAK_BINDER: {
@@ -3159,6 +3187,7 @@ static void binder_transaction(struct binder_proc *proc,
for (; offp < off_end; offp++) {
struct binder_object_header *hdr;
size_t object_size;
+ struct binder_object object;
binder_size_t object_offset;
binder_size_t buffer_offset =
(uintptr_t)offp - (uintptr_t)t->buffer->data;
@@ -3168,7 +3197,8 @@ static void binder_transaction(struct binder_proc *proc,
t->buffer,
buffer_offset,
sizeof(object_offset));
- object_size = binder_validate_object(t->buffer, object_offset);
+ object_size = binder_get_object(target_proc, t->buffer,
+ object_offset, &object);
if (object_size == 0 || object_offset < off_min) {
binder_user_error("%d:%d got transaction with invalid offset (%lld, min %lld max %lld) or object.\n",
proc->pid, thread->pid,
@@ -3181,8 +3211,7 @@ static void binder_transaction(struct binder_proc *proc,
goto err_bad_offset;
}
- hdr = (struct binder_object_header *)
- (t->buffer->data + object_offset);
+ hdr = &object.hdr;
off_min = object_offset + object_size;
switch (hdr->type) {
case BINDER_TYPE_BINDER:
@@ -3197,6 +3226,9 @@ static void binder_transaction(struct binder_proc *proc,
return_error_line = __LINE__;
goto err_translate_failed;
}
+ binder_alloc_copy_to_buffer(&target_proc->alloc,
+ t->buffer, object_offset,
+ fp, sizeof(*fp));
} break;
case BINDER_TYPE_HANDLE:
case BINDER_TYPE_WEAK_HANDLE: {
@@ -3210,6 +3242,9 @@ static void binder_transaction(struct binder_proc *proc,
return_error_line = __LINE__;
goto err_translate_failed;
}
+ binder_alloc_copy_to_buffer(&target_proc->alloc,
+ t->buffer, object_offset,
+ fp, sizeof(*fp));
} break;
case BINDER_TYPE_FD: {
@@ -3226,6 +3261,9 @@ static void binder_transaction(struct binder_proc *proc,
goto err_translate_failed;
}
fp->pad_binder = 0;
+ binder_alloc_copy_to_buffer(&target_proc->alloc,
+ t->buffer, object_offset,
+ fp, sizeof(*fp));
} break;
case BINDER_TYPE_FDA: {
struct binder_fd_array_object *fda =
@@ -3310,7 +3348,10 @@ static void binder_transaction(struct binder_proc *proc,
return_error_line = __LINE__;
goto err_translate_failed;
}
- last_fixup_obj = bp;
+ binder_alloc_copy_to_buffer(&target_proc->alloc,
+ t->buffer, object_offset,
+ bp, sizeof(*bp));
+ last_fixup_obj = t->buffer->data + object_offset;
last_fixup_min_off = 0;
} break;
default:
--
2.20.1.791.gb4d0f1c61a-goog
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH v3 4/7] binder: avoid kernel vm_area for buffer fixups
2019-02-08 18:35 [PATCH v3 0/7] binder: eliminate use of vmalloc space for binder buffers Todd Kjos
` (2 preceding siblings ...)
2019-02-08 18:35 ` [PATCH v3 3/7] binder: add function to copy binder object from buffer Todd Kjos
@ 2019-02-08 18:35 ` Todd Kjos
2019-02-08 18:35 ` [PATCH v3 5/7] binder: remove kernel vm_area for buffer space Todd Kjos
` (3 subsequent siblings)
7 siblings, 0 replies; 16+ messages in thread
From: Todd Kjos @ 2019-02-08 18:35 UTC (permalink / raw)
To: tkjos, gregkh, arve, devel, linux-kernel, maco; +Cc: joel, kernel-team
Refactor the functions to validate and fixup struct
binder_buffer pointer objects to avoid using vm_area
pointers. Instead copy to/from kernel space using
binder_alloc_copy_to_buffer() and
binder_alloc_copy_from_buffer(). The following
functions were refactored:
refactor binder_validate_ptr()
binder_validate_fixup()
binder_fixup_parent()
Signed-off-by: Todd Kjos <tkjos@google.com>
---
drivers/android/binder.c | 146 ++++++++++++++++++++++++++-------------
1 file changed, 97 insertions(+), 49 deletions(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 8063b405e4fa1..98163bf5f35c7 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -2092,10 +2092,13 @@ static size_t binder_get_object(struct binder_proc *proc,
/**
* binder_validate_ptr() - validates binder_buffer_object in a binder_buffer.
+ * @proc: binder_proc owning the buffer
* @b: binder_buffer containing the object
+ * @object: struct binder_object to read into
* @index: index in offset array at which the binder_buffer_object is
* located
- * @start: points to the start of the offset array
+ * @start_offset: points to the start of the offset array
+ * @object_offsetp: offset of @object read from @b
* @num_valid: the number of valid offsets in the offset array
*
* Return: If @index is within the valid range of the offset array
@@ -2106,34 +2109,46 @@ static size_t binder_get_object(struct binder_proc *proc,
* Note that the offset found in index @index itself is not
* verified; this function assumes that @num_valid elements
* from @start were previously verified to have valid offsets.
+ * If @object_offsetp is non-NULL, then the offset within
+ * @b is written to it.
*/
-static struct binder_buffer_object *binder_validate_ptr(struct binder_buffer *b,
- binder_size_t index,
- binder_size_t *start,
- binder_size_t num_valid)
+static struct binder_buffer_object *binder_validate_ptr(
+ struct binder_proc *proc,
+ struct binder_buffer *b,
+ struct binder_object *object,
+ binder_size_t index,
+ binder_size_t start_offset,
+ binder_size_t *object_offsetp,
+ binder_size_t num_valid)
{
- struct binder_buffer_object *buffer_obj;
- binder_size_t *offp;
+ size_t object_size;
+ binder_size_t object_offset;
+ unsigned long buffer_offset;
if (index >= num_valid)
return NULL;
- offp = start + index;
- buffer_obj = (struct binder_buffer_object *)(b->data + *offp);
- if (buffer_obj->hdr.type != BINDER_TYPE_PTR)
+ buffer_offset = start_offset + sizeof(binder_size_t) * index;
+ binder_alloc_copy_from_buffer(&proc->alloc, &object_offset,
+ b, buffer_offset, sizeof(object_offset));
+ object_size = binder_get_object(proc, b, object_offset, object);
+ if (!object_size || object->hdr.type != BINDER_TYPE_PTR)
return NULL;
+ if (object_offsetp)
+ *object_offsetp = object_offset;
- return buffer_obj;
+ return &object->bbo;
}
/**
* binder_validate_fixup() - validates pointer/fd fixups happen in order.
+ * @proc: binder_proc owning the buffer
* @b: transaction buffer
- * @objects_start start of objects buffer
- * @buffer: binder_buffer_object in which to fix up
- * @offset: start offset in @buffer to fix up
- * @last_obj: last binder_buffer_object that we fixed up in
- * @last_min_offset: minimum fixup offset in @last_obj
+ * @objects_start_offset: offset to start of objects buffer
+ * @buffer_obj_offset: offset to binder_buffer_object in which to fix up
+ * @fixup_offset: start offset in @buffer to fix up
+ * @last_obj_offset: offset to last binder_buffer_object that we fixed
+ * @last_min_offset: minimum fixup offset in object at @last_obj_offset
*
* Return: %true if a fixup in buffer @buffer at offset @offset is
* allowed.
@@ -2164,28 +2179,41 @@ static struct binder_buffer_object *binder_validate_ptr(struct binder_buffer *b,
* C (parent = A, offset = 16)
* D (parent = B, offset = 0) // B is not A or any of A's parents
*/
-static bool binder_validate_fixup(struct binder_buffer *b,
- binder_size_t *objects_start,
- struct binder_buffer_object *buffer,
+static bool binder_validate_fixup(struct binder_proc *proc,
+ struct binder_buffer *b,
+ binder_size_t objects_start_offset,
+ binder_size_t buffer_obj_offset,
binder_size_t fixup_offset,
- struct binder_buffer_object *last_obj,
+ binder_size_t last_obj_offset,
binder_size_t last_min_offset)
{
- if (!last_obj) {
+ if (!last_obj_offset) {
/* Nothing to fix up in */
return false;
}
- while (last_obj != buffer) {
+ while (last_obj_offset != buffer_obj_offset) {
+ unsigned long buffer_offset;
+ struct binder_object last_object;
+ struct binder_buffer_object *last_bbo;
+ size_t object_size = binder_get_object(proc, b, last_obj_offset,
+ &last_object);
+ if (object_size != sizeof(*last_bbo))
+ return false;
+
+ last_bbo = &last_object.bbo;
/*
* Safe to retrieve the parent of last_obj, since it
* was already previously verified by the driver.
*/
- if ((last_obj->flags & BINDER_BUFFER_FLAG_HAS_PARENT) == 0)
+ if ((last_bbo->flags & BINDER_BUFFER_FLAG_HAS_PARENT) == 0)
return false;
- last_min_offset = last_obj->parent_offset + sizeof(uintptr_t);
- last_obj = (struct binder_buffer_object *)
- (b->data + *(objects_start + last_obj->parent));
+ last_min_offset = last_bbo->parent_offset + sizeof(uintptr_t);
+ buffer_offset = objects_start_offset +
+ sizeof(binder_size_t) * last_bbo->parent,
+ binder_alloc_copy_from_buffer(&proc->alloc, &last_obj_offset,
+ b, buffer_offset,
+ sizeof(last_obj_offset));
}
return (fixup_offset >= last_min_offset);
}
@@ -2254,6 +2282,7 @@ static void binder_transaction_buffer_release(struct binder_proc *proc,
{
binder_size_t *offp, *off_start, *off_end;
int debug_id = buffer->debug_id;
+ binder_size_t off_start_offset;
binder_debug(BINDER_DEBUG_TRANSACTION,
"%d buffer release %d, size %zd-%zd, failed at %pK\n",
@@ -2263,8 +2292,8 @@ static void binder_transaction_buffer_release(struct binder_proc *proc,
if (buffer->target_node)
binder_dec_node(buffer->target_node, 1, 0);
- off_start = (binder_size_t *)(buffer->data +
- ALIGN(buffer->data_size, sizeof(void *)));
+ off_start_offset = ALIGN(buffer->data_size, sizeof(void *));
+ off_start = (binder_size_t *)(buffer->data + off_start_offset);
if (failed_at)
off_end = failed_at;
else
@@ -2350,6 +2379,7 @@ static void binder_transaction_buffer_release(struct binder_proc *proc,
case BINDER_TYPE_FDA: {
struct binder_fd_array_object *fda;
struct binder_buffer_object *parent;
+ struct binder_object ptr_object;
uintptr_t parent_buffer;
u32 *fd_array;
size_t fd_index;
@@ -2365,8 +2395,10 @@ static void binder_transaction_buffer_release(struct binder_proc *proc,
}
fda = to_binder_fd_array_object(hdr);
- parent = binder_validate_ptr(buffer, fda->parent,
- off_start,
+ parent = binder_validate_ptr(proc, buffer, &ptr_object,
+ fda->parent,
+ off_start_offset,
+ NULL,
offp - off_start);
if (!parent) {
pr_err("transaction release %d bad parent offset\n",
@@ -2665,9 +2697,9 @@ static int binder_translate_fd_array(struct binder_fd_array_object *fda,
static int binder_fixup_parent(struct binder_transaction *t,
struct binder_thread *thread,
struct binder_buffer_object *bp,
- binder_size_t *off_start,
+ binder_size_t off_start_offset,
binder_size_t num_valid,
- struct binder_buffer_object *last_fixup_obj,
+ binder_size_t last_fixup_obj_off,
binder_size_t last_fixup_min_off)
{
struct binder_buffer_object *parent;
@@ -2675,20 +2707,25 @@ static int binder_fixup_parent(struct binder_transaction *t,
struct binder_buffer *b = t->buffer;
struct binder_proc *proc = thread->proc;
struct binder_proc *target_proc = t->to_proc;
+ struct binder_object object;
+ binder_size_t buffer_offset;
+ binder_size_t parent_offset;
if (!(bp->flags & BINDER_BUFFER_FLAG_HAS_PARENT))
return 0;
- parent = binder_validate_ptr(b, bp->parent, off_start, num_valid);
+ parent = binder_validate_ptr(target_proc, b, &object, bp->parent,
+ off_start_offset, &parent_offset,
+ num_valid);
if (!parent) {
binder_user_error("%d:%d got transaction with invalid parent offset or type\n",
proc->pid, thread->pid);
return -EINVAL;
}
- if (!binder_validate_fixup(b, off_start,
- parent, bp->parent_offset,
- last_fixup_obj,
+ if (!binder_validate_fixup(target_proc, b, off_start_offset,
+ parent_offset, bp->parent_offset,
+ last_fixup_obj_off,
last_fixup_min_off)) {
binder_user_error("%d:%d got transaction with out-of-order buffer fixup\n",
proc->pid, thread->pid);
@@ -2705,7 +2742,10 @@ static int binder_fixup_parent(struct binder_transaction *t,
parent_buffer = (u8 *)((uintptr_t)parent->buffer -
binder_alloc_get_user_buffer_offset(
&target_proc->alloc));
- *(binder_uintptr_t *)(parent_buffer + bp->parent_offset) = bp->buffer;
+ buffer_offset = bp->parent_offset +
+ (uintptr_t)parent_buffer - (uintptr_t)b->data;
+ binder_alloc_copy_to_buffer(&target_proc->alloc, b, buffer_offset,
+ &bp->buffer, sizeof(bp->buffer));
return 0;
}
@@ -2825,6 +2865,7 @@ static void binder_transaction(struct binder_proc *proc,
struct binder_work *w;
struct binder_work *tcomplete;
binder_size_t *offp, *off_end, *off_start;
+ binder_size_t off_start_offset;
binder_size_t off_min;
u8 *sg_bufp, *sg_buf_end;
struct binder_proc *target_proc = NULL;
@@ -2835,7 +2876,7 @@ static void binder_transaction(struct binder_proc *proc,
uint32_t return_error = 0;
uint32_t return_error_param = 0;
uint32_t return_error_line = 0;
- struct binder_buffer_object *last_fixup_obj = NULL;
+ binder_size_t last_fixup_obj_off = 0;
binder_size_t last_fixup_min_off = 0;
struct binder_context *context = proc->context;
int t_debug_id = atomic_inc_return(&binder_last_id);
@@ -3132,8 +3173,8 @@ static void binder_transaction(struct binder_proc *proc,
t->buffer->transaction = t;
t->buffer->target_node = target_node;
trace_binder_transaction_alloc_buf(t->buffer);
- off_start = (binder_size_t *)(t->buffer->data +
- ALIGN(tr->data_size, sizeof(void *)));
+ off_start_offset = ALIGN(tr->data_size, sizeof(void *));
+ off_start = (binder_size_t *)(t->buffer->data + off_start_offset);
offp = off_start;
if (binder_alloc_copy_user_to_buffer(
@@ -3266,11 +3307,15 @@ static void binder_transaction(struct binder_proc *proc,
fp, sizeof(*fp));
} break;
case BINDER_TYPE_FDA: {
+ struct binder_object ptr_object;
+ binder_size_t parent_offset;
struct binder_fd_array_object *fda =
to_binder_fd_array_object(hdr);
struct binder_buffer_object *parent =
- binder_validate_ptr(t->buffer, fda->parent,
- off_start,
+ binder_validate_ptr(target_proc, t->buffer,
+ &ptr_object, fda->parent,
+ off_start_offset,
+ &parent_offset,
offp - off_start);
if (!parent) {
binder_user_error("%d:%d got transaction with invalid parent offset or type\n",
@@ -3280,9 +3325,11 @@ static void binder_transaction(struct binder_proc *proc,
return_error_line = __LINE__;
goto err_bad_parent;
}
- if (!binder_validate_fixup(t->buffer, off_start,
- parent, fda->parent_offset,
- last_fixup_obj,
+ if (!binder_validate_fixup(target_proc, t->buffer,
+ off_start_offset,
+ parent_offset,
+ fda->parent_offset,
+ last_fixup_obj_off,
last_fixup_min_off)) {
binder_user_error("%d:%d got transaction with out-of-order buffer fixup\n",
proc->pid, thread->pid);
@@ -3299,7 +3346,7 @@ static void binder_transaction(struct binder_proc *proc,
return_error_line = __LINE__;
goto err_translate_failed;
}
- last_fixup_obj = parent;
+ last_fixup_obj_off = parent_offset;
last_fixup_min_off =
fda->parent_offset + sizeof(u32) * fda->num_fds;
} break;
@@ -3338,9 +3385,10 @@ static void binder_transaction(struct binder_proc *proc,
&target_proc->alloc);
sg_bufp += ALIGN(bp->length, sizeof(u64));
- ret = binder_fixup_parent(t, thread, bp, off_start,
+ ret = binder_fixup_parent(t, thread, bp,
+ off_start_offset,
offp - off_start,
- last_fixup_obj,
+ last_fixup_obj_off,
last_fixup_min_off);
if (ret < 0) {
return_error = BR_FAILED_REPLY;
@@ -3351,7 +3399,7 @@ static void binder_transaction(struct binder_proc *proc,
binder_alloc_copy_to_buffer(&target_proc->alloc,
t->buffer, object_offset,
bp, sizeof(*bp));
- last_fixup_obj = t->buffer->data + object_offset;
+ last_fixup_obj_off = object_offset;
last_fixup_min_off = 0;
} break;
default:
--
2.20.1.791.gb4d0f1c61a-goog
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH v3 5/7] binder: remove kernel vm_area for buffer space
2019-02-08 18:35 [PATCH v3 0/7] binder: eliminate use of vmalloc space for binder buffers Todd Kjos
` (3 preceding siblings ...)
2019-02-08 18:35 ` [PATCH v3 4/7] binder: avoid kernel vm_area for buffer fixups Todd Kjos
@ 2019-02-08 18:35 ` Todd Kjos
2019-02-08 18:35 ` [PATCH v3 6/7] binder: remove user_buffer_offset Todd Kjos
` (2 subsequent siblings)
7 siblings, 0 replies; 16+ messages in thread
From: Todd Kjos @ 2019-02-08 18:35 UTC (permalink / raw)
To: tkjos, gregkh, arve, devel, linux-kernel, maco; +Cc: joel, kernel-team
Remove the kernel's vm_area and the code that maps
buffer pages into it.
Signed-off-by: Todd Kjos <tkjos@google.com>
---
drivers/android/binder_alloc.c | 40 ++--------------------------------
1 file changed, 2 insertions(+), 38 deletions(-)
diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
index 2eebff4be83e0..d4cbe4b3947a6 100644
--- a/drivers/android/binder_alloc.c
+++ b/drivers/android/binder_alloc.c
@@ -265,16 +265,6 @@ static int binder_update_page_range(struct binder_alloc *alloc, int allocate,
page->alloc = alloc;
INIT_LIST_HEAD(&page->lru);
- ret = map_kernel_range_noflush((unsigned long)page_addr,
- PAGE_SIZE, PAGE_KERNEL,
- &page->page_ptr);
- flush_cache_vmap((unsigned long)page_addr,
- (unsigned long)page_addr + PAGE_SIZE);
- if (ret != 1) {
- pr_err("%d: binder_alloc_buf failed to map page at %pK in kernel\n",
- alloc->pid, page_addr);
- goto err_map_kernel_failed;
- }
user_page_addr =
(uintptr_t)page_addr + alloc->user_buffer_offset;
ret = vm_insert_page(vma, user_page_addr, page[0].page_ptr);
@@ -314,8 +304,6 @@ static int binder_update_page_range(struct binder_alloc *alloc, int allocate,
continue;
err_vm_insert_page_failed:
- unmap_kernel_range((unsigned long)page_addr, PAGE_SIZE);
-err_map_kernel_failed:
__free_page(page->page_ptr);
page->page_ptr = NULL;
err_alloc_page_failed:
@@ -695,7 +683,6 @@ int binder_alloc_mmap_handler(struct binder_alloc *alloc,
struct vm_area_struct *vma)
{
int ret;
- struct vm_struct *area;
const char *failure_string;
struct binder_buffer *buffer;
@@ -706,28 +693,10 @@ int binder_alloc_mmap_handler(struct binder_alloc *alloc,
goto err_already_mapped;
}
- area = get_vm_area(vma->vm_end - vma->vm_start, VM_ALLOC);
- if (area == NULL) {
- ret = -ENOMEM;
- failure_string = "get_vm_area";
- goto err_get_vm_area_failed;
- }
- alloc->buffer = area->addr;
- alloc->user_buffer_offset =
- vma->vm_start - (uintptr_t)alloc->buffer;
+ alloc->buffer = (void *)vma->vm_start;
+ alloc->user_buffer_offset = 0;
mutex_unlock(&binder_alloc_mmap_lock);
-#ifdef CONFIG_CPU_CACHE_VIPT
- if (cache_is_vipt_aliasing()) {
- while (CACHE_COLOUR(
- (vma->vm_start ^ (uint32_t)alloc->buffer))) {
- pr_info("%s: %d %lx-%lx maps %pK bad alignment\n",
- __func__, alloc->pid, vma->vm_start,
- vma->vm_end, alloc->buffer);
- vma->vm_start += PAGE_SIZE;
- }
- }
-#endif
alloc->pages = kcalloc((vma->vm_end - vma->vm_start) / PAGE_SIZE,
sizeof(alloc->pages[0]),
GFP_KERNEL);
@@ -760,9 +729,7 @@ int binder_alloc_mmap_handler(struct binder_alloc *alloc,
alloc->pages = NULL;
err_alloc_pages_failed:
mutex_lock(&binder_alloc_mmap_lock);
- vfree(alloc->buffer);
alloc->buffer = NULL;
-err_get_vm_area_failed:
err_already_mapped:
mutex_unlock(&binder_alloc_mmap_lock);
binder_alloc_debug(BINDER_DEBUG_USER_ERROR,
@@ -821,12 +788,10 @@ void binder_alloc_deferred_release(struct binder_alloc *alloc)
"%s: %d: page %d at %pK %s\n",
__func__, alloc->pid, i, page_addr,
on_lru ? "on lru" : "active");
- unmap_kernel_range((unsigned long)page_addr, PAGE_SIZE);
__free_page(alloc->pages[i].page_ptr);
page_count++;
}
kfree(alloc->pages);
- vfree(alloc->buffer);
}
mutex_unlock(&alloc->mutex);
if (alloc->vma_vm_mm)
@@ -988,7 +953,6 @@ enum lru_status binder_alloc_free_page(struct list_head *item,
trace_binder_unmap_kernel_start(alloc, index);
- unmap_kernel_range(page_addr, PAGE_SIZE);
__free_page(page->page_ptr);
page->page_ptr = NULL;
--
2.20.1.791.gb4d0f1c61a-goog
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH v3 6/7] binder: remove user_buffer_offset
2019-02-08 18:35 [PATCH v3 0/7] binder: eliminate use of vmalloc space for binder buffers Todd Kjos
` (4 preceding siblings ...)
2019-02-08 18:35 ` [PATCH v3 5/7] binder: remove kernel vm_area for buffer space Todd Kjos
@ 2019-02-08 18:35 ` Todd Kjos
2019-02-08 18:35 ` [PATCH v3 7/7] binder: use userspace pointer as base of buffer space Todd Kjos
2019-02-11 16:57 ` [PATCH v3 0/7] binder: eliminate use of vmalloc space for binder buffers Christoph Hellwig
7 siblings, 0 replies; 16+ messages in thread
From: Todd Kjos @ 2019-02-08 18:35 UTC (permalink / raw)
To: tkjos, gregkh, arve, devel, linux-kernel, maco; +Cc: joel, kernel-team
Remove user_buffer_offset since there is no kernel
buffer pointer anymore.
Signed-off-by: Todd Kjos <tkjos@google.com>
---
v2: remove casts as suggested by Dan Carpenter
drivers/android/binder.c | 39 ++++++----------------------------
drivers/android/binder_alloc.c | 16 ++++++--------
drivers/android/binder_alloc.h | 23 --------------------
3 files changed, 13 insertions(+), 65 deletions(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 98163bf5f35c7..b3d609b5935a6 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -2380,7 +2380,6 @@ static void binder_transaction_buffer_release(struct binder_proc *proc,
struct binder_fd_array_object *fda;
struct binder_buffer_object *parent;
struct binder_object ptr_object;
- uintptr_t parent_buffer;
u32 *fd_array;
size_t fd_index;
binder_size_t fd_buf_size;
@@ -2405,14 +2404,6 @@ static void binder_transaction_buffer_release(struct binder_proc *proc,
debug_id);
continue;
}
- /*
- * Since the parent was already fixed up, convert it
- * back to kernel address space to access it
- */
- parent_buffer = parent->buffer -
- binder_alloc_get_user_buffer_offset(
- &proc->alloc);
-
fd_buf_size = sizeof(u32) * fda->num_fds;
if (fda->num_fds >= SIZE_MAX / sizeof(u32)) {
pr_err("transaction release %d invalid number of fds (%lld)\n",
@@ -2426,7 +2417,8 @@ static void binder_transaction_buffer_release(struct binder_proc *proc,
debug_id, (u64)fda->num_fds);
continue;
}
- fd_array = (u32 *)(parent_buffer + (uintptr_t)fda->parent_offset);
+ fd_array = (u32 *)(uintptr_t)
+ (parent->buffer + fda->parent_offset);
for (fd_index = 0; fd_index < fda->num_fds;
fd_index++) {
u32 fd;
@@ -2646,7 +2638,6 @@ static int binder_translate_fd_array(struct binder_fd_array_object *fda,
struct binder_transaction *in_reply_to)
{
binder_size_t fdi, fd_buf_size;
- uintptr_t parent_buffer;
u32 *fd_array;
struct binder_proc *proc = thread->proc;
struct binder_proc *target_proc = t->to_proc;
@@ -2664,13 +2655,7 @@ static int binder_translate_fd_array(struct binder_fd_array_object *fda,
proc->pid, thread->pid, (u64)fda->num_fds);
return -EINVAL;
}
- /*
- * Since the parent was already fixed up, convert it
- * back to the kernel address space to access it
- */
- parent_buffer = parent->buffer -
- binder_alloc_get_user_buffer_offset(&target_proc->alloc);
- fd_array = (u32 *)(parent_buffer + (uintptr_t)fda->parent_offset);
+ fd_array = (u32 *)(uintptr_t)(parent->buffer + fda->parent_offset);
if (!IS_ALIGNED((unsigned long)fd_array, sizeof(u32))) {
binder_user_error("%d:%d parent offset not aligned correctly.\n",
proc->pid, thread->pid);
@@ -2703,7 +2688,6 @@ static int binder_fixup_parent(struct binder_transaction *t,
binder_size_t last_fixup_min_off)
{
struct binder_buffer_object *parent;
- u8 *parent_buffer;
struct binder_buffer *b = t->buffer;
struct binder_proc *proc = thread->proc;
struct binder_proc *target_proc = t->to_proc;
@@ -2739,11 +2723,8 @@ static int binder_fixup_parent(struct binder_transaction *t,
proc->pid, thread->pid);
return -EINVAL;
}
- parent_buffer = (u8 *)((uintptr_t)parent->buffer -
- binder_alloc_get_user_buffer_offset(
- &target_proc->alloc));
buffer_offset = bp->parent_offset +
- (uintptr_t)parent_buffer - (uintptr_t)b->data;
+ (uintptr_t)parent->buffer - (uintptr_t)b->data;
binder_alloc_copy_to_buffer(&target_proc->alloc, b, buffer_offset,
&bp->buffer, sizeof(bp->buffer));
@@ -3159,10 +3140,8 @@ static void binder_transaction(struct binder_proc *proc,
ALIGN(tr->offsets_size, sizeof(void *)) +
ALIGN(extra_buffers_size, sizeof(void *)) -
ALIGN(secctx_sz, sizeof(u64));
- char *kptr = t->buffer->data + buf_offset;
- t->security_ctx = (uintptr_t)kptr +
- binder_alloc_get_user_buffer_offset(&target_proc->alloc);
+ t->security_ctx = (uintptr_t)t->buffer->data + buf_offset;
binder_alloc_copy_to_buffer(&target_proc->alloc,
t->buffer, buf_offset,
secctx, secctx_sz);
@@ -3380,9 +3359,7 @@ static void binder_transaction(struct binder_proc *proc,
goto err_copy_data_failed;
}
/* Fixup buffer pointer to target proc address space */
- bp->buffer = (uintptr_t)sg_bufp +
- binder_alloc_get_user_buffer_offset(
- &target_proc->alloc);
+ bp->buffer = (uintptr_t)sg_bufp;
sg_bufp += ALIGN(bp->length, sizeof(u64));
ret = binder_fixup_parent(t, thread, bp,
@@ -4474,9 +4451,7 @@ static int binder_thread_read(struct binder_proc *proc,
}
trd->data_size = t->buffer->data_size;
trd->offsets_size = t->buffer->offsets_size;
- trd->data.ptr.buffer = (binder_uintptr_t)
- ((uintptr_t)t->buffer->data +
- binder_alloc_get_user_buffer_offset(&proc->alloc));
+ trd->data.ptr.buffer = (uintptr_t)t->buffer->data;
trd->data.ptr.offsets = trd->data.ptr.buffer +
ALIGN(t->buffer->data_size,
sizeof(void *));
diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
index d4cbe4b3947a6..0e7f0aa967c32 100644
--- a/drivers/android/binder_alloc.c
+++ b/drivers/android/binder_alloc.c
@@ -138,17 +138,17 @@ static struct binder_buffer *binder_alloc_prepare_to_free_locked(
{
struct rb_node *n = alloc->allocated_buffers.rb_node;
struct binder_buffer *buffer;
- void *kern_ptr;
+ void *uptr;
- kern_ptr = (void *)(user_ptr - alloc->user_buffer_offset);
+ uptr = (void *)user_ptr;
while (n) {
buffer = rb_entry(n, struct binder_buffer, rb_node);
BUG_ON(buffer->free);
- if (kern_ptr < buffer->data)
+ if (uptr < buffer->data)
n = n->rb_left;
- else if (kern_ptr > buffer->data)
+ else if (uptr > buffer->data)
n = n->rb_right;
else {
/*
@@ -265,8 +265,7 @@ static int binder_update_page_range(struct binder_alloc *alloc, int allocate,
page->alloc = alloc;
INIT_LIST_HEAD(&page->lru);
- user_page_addr =
- (uintptr_t)page_addr + alloc->user_buffer_offset;
+ user_page_addr = (uintptr_t)page_addr;
ret = vm_insert_page(vma, user_page_addr, page[0].page_ptr);
if (ret) {
pr_err("%d: binder_alloc_buf failed to map page at %lx in userspace\n",
@@ -694,7 +693,6 @@ int binder_alloc_mmap_handler(struct binder_alloc *alloc,
}
alloc->buffer = (void *)vma->vm_start;
- alloc->user_buffer_offset = 0;
mutex_unlock(&binder_alloc_mmap_lock);
alloc->pages = kcalloc((vma->vm_end - vma->vm_start) / PAGE_SIZE,
@@ -941,9 +939,7 @@ enum lru_status binder_alloc_free_page(struct list_head *item,
if (vma) {
trace_binder_unmap_user_start(alloc, index);
- zap_page_range(vma,
- page_addr + alloc->user_buffer_offset,
- PAGE_SIZE);
+ zap_page_range(vma, page_addr, PAGE_SIZE);
trace_binder_unmap_user_end(alloc, index);
diff --git a/drivers/android/binder_alloc.h b/drivers/android/binder_alloc.h
index 9d682b9d6c241..1026e9fb20db1 100644
--- a/drivers/android/binder_alloc.h
+++ b/drivers/android/binder_alloc.h
@@ -82,7 +82,6 @@ struct binder_lru_page {
* (invariant after init)
* @vma_vm_mm: copy of vma->vm_mm (invarient after mmap)
* @buffer: base of per-proc address space mapped via mmap
- * @user_buffer_offset: offset between user and kernel VAs for buffer
* @buffers: list of all buffers for this proc
* @free_buffers: rb tree of buffers available for allocation
* sorted by size
@@ -104,7 +103,6 @@ struct binder_alloc {
struct vm_area_struct *vma;
struct mm_struct *vma_vm_mm;
void *buffer;
- ptrdiff_t user_buffer_offset;
struct list_head buffers;
struct rb_root free_buffers;
struct rb_root allocated_buffers;
@@ -163,27 +161,6 @@ binder_alloc_get_free_async_space(struct binder_alloc *alloc)
return free_async_space;
}
-/**
- * binder_alloc_get_user_buffer_offset() - get offset between kernel/user addrs
- * @alloc: binder_alloc for this proc
- *
- * Return: the offset between kernel and user-space addresses to use for
- * virtual address conversion
- */
-static inline ptrdiff_t
-binder_alloc_get_user_buffer_offset(struct binder_alloc *alloc)
-{
- /*
- * user_buffer_offset is constant if vma is set and
- * undefined if vma is not set. It is possible to
- * get here with !alloc->vma if the target process
- * is dying while a transaction is being initiated.
- * Returning the old value is ok in this case and
- * the transaction will fail.
- */
- return alloc->user_buffer_offset;
-}
-
unsigned long
binder_alloc_copy_user_to_buffer(struct binder_alloc *alloc,
struct binder_buffer *buffer,
--
2.20.1.791.gb4d0f1c61a-goog
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH v3 7/7] binder: use userspace pointer as base of buffer space
2019-02-08 18:35 [PATCH v3 0/7] binder: eliminate use of vmalloc space for binder buffers Todd Kjos
` (5 preceding siblings ...)
2019-02-08 18:35 ` [PATCH v3 6/7] binder: remove user_buffer_offset Todd Kjos
@ 2019-02-08 18:35 ` Todd Kjos
2019-02-11 16:57 ` [PATCH v3 0/7] binder: eliminate use of vmalloc space for binder buffers Christoph Hellwig
7 siblings, 0 replies; 16+ messages in thread
From: Todd Kjos @ 2019-02-08 18:35 UTC (permalink / raw)
To: tkjos, gregkh, arve, devel, linux-kernel, maco; +Cc: joel, kernel-team
Now that alloc->buffer points to the userspace vm_area
rename buffer->data to buffer->user_data and rename
local pointers that hold user addresses. Also use the
"__user" tag to annotate all user pointers so sparse
can flag cases where user pointer vaues are copied to
kernel pointers. Refactor code to use offsets instead
of user pointers.
Signed-off-by: Todd Kjos <tkjos@google.com>
---
v2: remove casts as suggested by Dan Carpenter
v3: fix build-break when CONFIG_ANDROID_BINDER_IPC_SELFTEST enabled
drivers/android/binder.c | 118 ++++++++++++++----------
drivers/android/binder_alloc.c | 87 ++++++++---------
drivers/android/binder_alloc.h | 6 +-
drivers/android/binder_alloc_selftest.c | 4 +-
drivers/android/binder_trace.h | 2 +-
5 files changed, 118 insertions(+), 99 deletions(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index b3d609b5935a6..25491eceb7503 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -2278,33 +2278,30 @@ static void binder_deferred_fd_close(int fd)
static void binder_transaction_buffer_release(struct binder_proc *proc,
struct binder_buffer *buffer,
- binder_size_t *failed_at)
+ binder_size_t failed_at,
+ bool is_failure)
{
- binder_size_t *offp, *off_start, *off_end;
int debug_id = buffer->debug_id;
- binder_size_t off_start_offset;
+ binder_size_t off_start_offset, buffer_offset, off_end_offset;
binder_debug(BINDER_DEBUG_TRANSACTION,
- "%d buffer release %d, size %zd-%zd, failed at %pK\n",
+ "%d buffer release %d, size %zd-%zd, failed at %llx\n",
proc->pid, buffer->debug_id,
- buffer->data_size, buffer->offsets_size, failed_at);
+ buffer->data_size, buffer->offsets_size,
+ (unsigned long long)failed_at);
if (buffer->target_node)
binder_dec_node(buffer->target_node, 1, 0);
off_start_offset = ALIGN(buffer->data_size, sizeof(void *));
- off_start = (binder_size_t *)(buffer->data + off_start_offset);
- if (failed_at)
- off_end = failed_at;
- else
- off_end = (void *)off_start + buffer->offsets_size;
- for (offp = off_start; offp < off_end; offp++) {
+ off_end_offset = is_failure ? failed_at :
+ off_start_offset + buffer->offsets_size;
+ for (buffer_offset = off_start_offset; buffer_offset < off_end_offset;
+ buffer_offset += sizeof(binder_size_t)) {
struct binder_object_header *hdr;
size_t object_size;
struct binder_object object;
binder_size_t object_offset;
- binder_size_t buffer_offset = (uintptr_t)offp -
- (uintptr_t)buffer->data;
binder_alloc_copy_from_buffer(&proc->alloc, &object_offset,
buffer, buffer_offset,
@@ -2380,9 +2377,10 @@ static void binder_transaction_buffer_release(struct binder_proc *proc,
struct binder_fd_array_object *fda;
struct binder_buffer_object *parent;
struct binder_object ptr_object;
- u32 *fd_array;
+ binder_size_t fda_offset;
size_t fd_index;
binder_size_t fd_buf_size;
+ binder_size_t num_valid;
if (proc->tsk != current->group_leader) {
/*
@@ -2393,12 +2391,14 @@ static void binder_transaction_buffer_release(struct binder_proc *proc,
continue;
}
+ num_valid = (buffer_offset - off_start_offset) /
+ sizeof(binder_size_t);
fda = to_binder_fd_array_object(hdr);
parent = binder_validate_ptr(proc, buffer, &ptr_object,
fda->parent,
off_start_offset,
NULL,
- offp - off_start);
+ num_valid);
if (!parent) {
pr_err("transaction release %d bad parent offset\n",
debug_id);
@@ -2417,14 +2417,21 @@ static void binder_transaction_buffer_release(struct binder_proc *proc,
debug_id, (u64)fda->num_fds);
continue;
}
- fd_array = (u32 *)(uintptr_t)
- (parent->buffer + fda->parent_offset);
+ /*
+ * the source data for binder_buffer_object is visible
+ * to user-space and the @buffer element is the user
+ * pointer to the buffer_object containing the fd_array.
+ * Convert the address to an offset relative to
+ * the base of the transaction buffer.
+ */
+ fda_offset =
+ (parent->buffer - (uintptr_t)buffer->user_data) +
+ fda->parent_offset;
for (fd_index = 0; fd_index < fda->num_fds;
fd_index++) {
u32 fd;
- binder_size_t offset =
- (uintptr_t)&fd_array[fd_index] -
- (uintptr_t)buffer->data;
+ binder_size_t offset = fda_offset +
+ fd_index * sizeof(fd);
binder_alloc_copy_from_buffer(&proc->alloc,
&fd,
@@ -2638,7 +2645,7 @@ static int binder_translate_fd_array(struct binder_fd_array_object *fda,
struct binder_transaction *in_reply_to)
{
binder_size_t fdi, fd_buf_size;
- u32 *fd_array;
+ binder_size_t fda_offset;
struct binder_proc *proc = thread->proc;
struct binder_proc *target_proc = t->to_proc;
@@ -2655,8 +2662,16 @@ static int binder_translate_fd_array(struct binder_fd_array_object *fda,
proc->pid, thread->pid, (u64)fda->num_fds);
return -EINVAL;
}
- fd_array = (u32 *)(uintptr_t)(parent->buffer + fda->parent_offset);
- if (!IS_ALIGNED((unsigned long)fd_array, sizeof(u32))) {
+ /*
+ * the source data for binder_buffer_object is visible
+ * to user-space and the @buffer element is the user
+ * pointer to the buffer_object containing the fd_array.
+ * Convert the address to an offset relative to
+ * the base of the transaction buffer.
+ */
+ fda_offset = (parent->buffer - (uintptr_t)t->buffer->user_data) +
+ fda->parent_offset;
+ if (!IS_ALIGNED((unsigned long)fda_offset, sizeof(u32))) {
binder_user_error("%d:%d parent offset not aligned correctly.\n",
proc->pid, thread->pid);
return -EINVAL;
@@ -2664,9 +2679,7 @@ static int binder_translate_fd_array(struct binder_fd_array_object *fda,
for (fdi = 0; fdi < fda->num_fds; fdi++) {
u32 fd;
int ret;
- binder_size_t offset =
- (uintptr_t)&fd_array[fdi] -
- (uintptr_t)t->buffer->data;
+ binder_size_t offset = fda_offset + fdi * sizeof(fd);
binder_alloc_copy_from_buffer(&target_proc->alloc,
&fd, t->buffer,
@@ -2724,7 +2737,7 @@ static int binder_fixup_parent(struct binder_transaction *t,
return -EINVAL;
}
buffer_offset = bp->parent_offset +
- (uintptr_t)parent->buffer - (uintptr_t)b->data;
+ (uintptr_t)parent->buffer - (uintptr_t)b->user_data;
binder_alloc_copy_to_buffer(&target_proc->alloc, b, buffer_offset,
&bp->buffer, sizeof(bp->buffer));
@@ -2845,10 +2858,10 @@ static void binder_transaction(struct binder_proc *proc,
struct binder_transaction *t;
struct binder_work *w;
struct binder_work *tcomplete;
- binder_size_t *offp, *off_end, *off_start;
- binder_size_t off_start_offset;
+ binder_size_t buffer_offset = 0;
+ binder_size_t off_start_offset, off_end_offset;
binder_size_t off_min;
- u8 *sg_bufp, *sg_buf_end;
+ binder_size_t sg_buf_offset, sg_buf_end_offset;
struct binder_proc *target_proc = NULL;
struct binder_thread *target_thread = NULL;
struct binder_node *target_node = NULL;
@@ -3141,7 +3154,7 @@ static void binder_transaction(struct binder_proc *proc,
ALIGN(extra_buffers_size, sizeof(void *)) -
ALIGN(secctx_sz, sizeof(u64));
- t->security_ctx = (uintptr_t)t->buffer->data + buf_offset;
+ t->security_ctx = (uintptr_t)t->buffer->user_data + buf_offset;
binder_alloc_copy_to_buffer(&target_proc->alloc,
t->buffer, buf_offset,
secctx, secctx_sz);
@@ -3152,9 +3165,6 @@ static void binder_transaction(struct binder_proc *proc,
t->buffer->transaction = t;
t->buffer->target_node = target_node;
trace_binder_transaction_alloc_buf(t->buffer);
- off_start_offset = ALIGN(tr->data_size, sizeof(void *));
- off_start = (binder_size_t *)(t->buffer->data + off_start_offset);
- offp = off_start;
if (binder_alloc_copy_user_to_buffer(
&target_proc->alloc,
@@ -3200,17 +3210,18 @@ static void binder_transaction(struct binder_proc *proc,
return_error_line = __LINE__;
goto err_bad_offset;
}
- off_end = (void *)off_start + tr->offsets_size;
- sg_bufp = (u8 *)(PTR_ALIGN(off_end, sizeof(void *)));
- sg_buf_end = sg_bufp + extra_buffers_size;
+ off_start_offset = ALIGN(tr->data_size, sizeof(void *));
+ buffer_offset = off_start_offset;
+ off_end_offset = off_start_offset + tr->offsets_size;
+ sg_buf_offset = ALIGN(off_end_offset, sizeof(void *));
+ sg_buf_end_offset = sg_buf_offset + extra_buffers_size;
off_min = 0;
- for (; offp < off_end; offp++) {
+ for (buffer_offset = off_start_offset; buffer_offset < off_end_offset;
+ buffer_offset += sizeof(binder_size_t)) {
struct binder_object_header *hdr;
size_t object_size;
struct binder_object object;
binder_size_t object_offset;
- binder_size_t buffer_offset =
- (uintptr_t)offp - (uintptr_t)t->buffer->data;
binder_alloc_copy_from_buffer(&target_proc->alloc,
&object_offset,
@@ -3290,12 +3301,14 @@ static void binder_transaction(struct binder_proc *proc,
binder_size_t parent_offset;
struct binder_fd_array_object *fda =
to_binder_fd_array_object(hdr);
+ size_t num_valid = (buffer_offset - off_start_offset) *
+ sizeof(binder_size_t);
struct binder_buffer_object *parent =
binder_validate_ptr(target_proc, t->buffer,
&ptr_object, fda->parent,
off_start_offset,
&parent_offset,
- offp - off_start);
+ num_valid);
if (!parent) {
binder_user_error("%d:%d got transaction with invalid parent offset or type\n",
proc->pid, thread->pid);
@@ -3332,9 +3345,8 @@ static void binder_transaction(struct binder_proc *proc,
case BINDER_TYPE_PTR: {
struct binder_buffer_object *bp =
to_binder_buffer_object(hdr);
- size_t buf_left = sg_buf_end - sg_bufp;
- binder_size_t sg_buf_offset = (uintptr_t)sg_bufp -
- (uintptr_t)t->buffer->data;
+ size_t buf_left = sg_buf_end_offset - sg_buf_offset;
+ size_t num_valid;
if (bp->length > buf_left) {
binder_user_error("%d:%d got transaction with too large buffer\n",
@@ -3359,12 +3371,15 @@ static void binder_transaction(struct binder_proc *proc,
goto err_copy_data_failed;
}
/* Fixup buffer pointer to target proc address space */
- bp->buffer = (uintptr_t)sg_bufp;
- sg_bufp += ALIGN(bp->length, sizeof(u64));
+ bp->buffer = (uintptr_t)
+ t->buffer->user_data + sg_buf_offset;
+ sg_buf_offset += ALIGN(bp->length, sizeof(u64));
+ num_valid = (buffer_offset - off_start_offset) *
+ sizeof(binder_size_t);
ret = binder_fixup_parent(t, thread, bp,
off_start_offset,
- offp - off_start,
+ num_valid,
last_fixup_obj_off,
last_fixup_min_off);
if (ret < 0) {
@@ -3456,7 +3471,8 @@ static void binder_transaction(struct binder_proc *proc,
err_copy_data_failed:
binder_free_txn_fixups(t);
trace_binder_transaction_failed_buffer_release(t->buffer);
- binder_transaction_buffer_release(target_proc, t->buffer, offp);
+ binder_transaction_buffer_release(target_proc, t->buffer,
+ buffer_offset, true);
if (target_node)
binder_dec_node_tmpref(target_node);
target_node = NULL;
@@ -3557,7 +3573,7 @@ binder_free_buf(struct binder_proc *proc, struct binder_buffer *buffer)
binder_node_inner_unlock(buf_node);
}
trace_binder_transaction_buffer_release(buffer);
- binder_transaction_buffer_release(proc, buffer, NULL);
+ binder_transaction_buffer_release(proc, buffer, 0, false);
binder_alloc_free_buf(&proc->alloc, buffer);
}
@@ -4451,7 +4467,7 @@ static int binder_thread_read(struct binder_proc *proc,
}
trd->data_size = t->buffer->data_size;
trd->offsets_size = t->buffer->offsets_size;
- trd->data.ptr.buffer = (uintptr_t)t->buffer->data;
+ trd->data.ptr.buffer = (uintptr_t)t->buffer->user_data;
trd->data.ptr.offsets = trd->data.ptr.buffer +
ALIGN(t->buffer->data_size,
sizeof(void *));
@@ -5489,7 +5505,7 @@ static void print_binder_transaction_ilocked(struct seq_file *m,
seq_printf(m, " node %d", buffer->target_node->debug_id);
seq_printf(m, " size %zd:%zd data %pK\n",
buffer->data_size, buffer->offsets_size,
- buffer->data);
+ buffer->user_data);
}
static void print_binder_work_ilocked(struct seq_file *m,
diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
index 0e7f0aa967c32..000dd4d145bae 100644
--- a/drivers/android/binder_alloc.c
+++ b/drivers/android/binder_alloc.c
@@ -69,9 +69,8 @@ static size_t binder_alloc_buffer_size(struct binder_alloc *alloc,
struct binder_buffer *buffer)
{
if (list_is_last(&buffer->entry, &alloc->buffers))
- return (u8 *)alloc->buffer +
- alloc->buffer_size - (u8 *)buffer->data;
- return (u8 *)binder_buffer_next(buffer)->data - (u8 *)buffer->data;
+ return alloc->buffer + alloc->buffer_size - buffer->user_data;
+ return binder_buffer_next(buffer)->user_data - buffer->user_data;
}
static void binder_insert_free_buffer(struct binder_alloc *alloc,
@@ -121,9 +120,9 @@ static void binder_insert_allocated_buffer_locked(
buffer = rb_entry(parent, struct binder_buffer, rb_node);
BUG_ON(buffer->free);
- if (new_buffer->data < buffer->data)
+ if (new_buffer->user_data < buffer->user_data)
p = &parent->rb_left;
- else if (new_buffer->data > buffer->data)
+ else if (new_buffer->user_data > buffer->user_data)
p = &parent->rb_right;
else
BUG();
@@ -138,17 +137,17 @@ static struct binder_buffer *binder_alloc_prepare_to_free_locked(
{
struct rb_node *n = alloc->allocated_buffers.rb_node;
struct binder_buffer *buffer;
- void *uptr;
+ void __user *uptr;
- uptr = (void *)user_ptr;
+ uptr = (void __user *)user_ptr;
while (n) {
buffer = rb_entry(n, struct binder_buffer, rb_node);
BUG_ON(buffer->free);
- if (uptr < buffer->data)
+ if (uptr < buffer->user_data)
n = n->rb_left;
- else if (uptr > buffer->data)
+ else if (uptr > buffer->user_data)
n = n->rb_right;
else {
/*
@@ -188,9 +187,9 @@ struct binder_buffer *binder_alloc_prepare_to_free(struct binder_alloc *alloc,
}
static int binder_update_page_range(struct binder_alloc *alloc, int allocate,
- void *start, void *end)
+ void __user *start, void __user *end)
{
- void *page_addr;
+ void __user *page_addr;
unsigned long user_page_addr;
struct binder_lru_page *page;
struct vm_area_struct *vma = NULL;
@@ -357,8 +356,8 @@ static struct binder_buffer *binder_alloc_new_buf_locked(
struct binder_buffer *buffer;
size_t buffer_size;
struct rb_node *best_fit = NULL;
- void *has_page_addr;
- void *end_page_addr;
+ void __user *has_page_addr;
+ void __user *end_page_addr;
size_t size, data_offsets_size;
int ret;
@@ -456,15 +455,15 @@ static struct binder_buffer *binder_alloc_new_buf_locked(
"%d: binder_alloc_buf size %zd got buffer %pK size %zd\n",
alloc->pid, size, buffer, buffer_size);
- has_page_addr =
- (void *)(((uintptr_t)buffer->data + buffer_size) & PAGE_MASK);
+ has_page_addr = (void __user *)
+ (((uintptr_t)buffer->user_data + buffer_size) & PAGE_MASK);
WARN_ON(n && buffer_size != size);
end_page_addr =
- (void *)PAGE_ALIGN((uintptr_t)buffer->data + size);
+ (void __user *)PAGE_ALIGN((uintptr_t)buffer->user_data + size);
if (end_page_addr > has_page_addr)
end_page_addr = has_page_addr;
- ret = binder_update_page_range(alloc, 1,
- (void *)PAGE_ALIGN((uintptr_t)buffer->data), end_page_addr);
+ ret = binder_update_page_range(alloc, 1, (void __user *)
+ PAGE_ALIGN((uintptr_t)buffer->user_data), end_page_addr);
if (ret)
return ERR_PTR(ret);
@@ -477,7 +476,7 @@ static struct binder_buffer *binder_alloc_new_buf_locked(
__func__, alloc->pid);
goto err_alloc_buf_struct_failed;
}
- new_buffer->data = (u8 *)buffer->data + size;
+ new_buffer->user_data = (u8 __user *)buffer->user_data + size;
list_add(&new_buffer->entry, &buffer->entry);
new_buffer->free = 1;
binder_insert_free_buffer(alloc, new_buffer);
@@ -503,8 +502,8 @@ static struct binder_buffer *binder_alloc_new_buf_locked(
return buffer;
err_alloc_buf_struct_failed:
- binder_update_page_range(alloc, 0,
- (void *)PAGE_ALIGN((uintptr_t)buffer->data),
+ binder_update_page_range(alloc, 0, (void __user *)
+ PAGE_ALIGN((uintptr_t)buffer->user_data),
end_page_addr);
return ERR_PTR(-ENOMEM);
}
@@ -539,14 +538,15 @@ struct binder_buffer *binder_alloc_new_buf(struct binder_alloc *alloc,
return buffer;
}
-static void *buffer_start_page(struct binder_buffer *buffer)
+static void __user *buffer_start_page(struct binder_buffer *buffer)
{
- return (void *)((uintptr_t)buffer->data & PAGE_MASK);
+ return (void __user *)((uintptr_t)buffer->user_data & PAGE_MASK);
}
-static void *prev_buffer_end_page(struct binder_buffer *buffer)
+static void __user *prev_buffer_end_page(struct binder_buffer *buffer)
{
- return (void *)(((uintptr_t)(buffer->data) - 1) & PAGE_MASK);
+ return (void __user *)
+ (((uintptr_t)(buffer->user_data) - 1) & PAGE_MASK);
}
static void binder_delete_free_buffer(struct binder_alloc *alloc,
@@ -561,7 +561,8 @@ static void binder_delete_free_buffer(struct binder_alloc *alloc,
to_free = false;
binder_alloc_debug(BINDER_DEBUG_BUFFER_ALLOC,
"%d: merge free, buffer %pK share page with %pK\n",
- alloc->pid, buffer->data, prev->data);
+ alloc->pid, buffer->user_data,
+ prev->user_data);
}
if (!list_is_last(&buffer->entry, &alloc->buffers)) {
@@ -571,23 +572,24 @@ static void binder_delete_free_buffer(struct binder_alloc *alloc,
binder_alloc_debug(BINDER_DEBUG_BUFFER_ALLOC,
"%d: merge free, buffer %pK share page with %pK\n",
alloc->pid,
- buffer->data,
- next->data);
+ buffer->user_data,
+ next->user_data);
}
}
- if (PAGE_ALIGNED(buffer->data)) {
+ if (PAGE_ALIGNED(buffer->user_data)) {
binder_alloc_debug(BINDER_DEBUG_BUFFER_ALLOC,
"%d: merge free, buffer start %pK is page aligned\n",
- alloc->pid, buffer->data);
+ alloc->pid, buffer->user_data);
to_free = false;
}
if (to_free) {
binder_alloc_debug(BINDER_DEBUG_BUFFER_ALLOC,
"%d: merge free, buffer %pK do not share page with %pK or %pK\n",
- alloc->pid, buffer->data,
- prev->data, next ? next->data : NULL);
+ alloc->pid, buffer->user_data,
+ prev->user_data,
+ next ? next->user_data : NULL);
binder_update_page_range(alloc, 0, buffer_start_page(buffer),
buffer_start_page(buffer) + PAGE_SIZE);
}
@@ -613,8 +615,8 @@ static void binder_free_buf_locked(struct binder_alloc *alloc,
BUG_ON(buffer->free);
BUG_ON(size > buffer_size);
BUG_ON(buffer->transaction != NULL);
- BUG_ON(buffer->data < alloc->buffer);
- BUG_ON(buffer->data > alloc->buffer + alloc->buffer_size);
+ BUG_ON(buffer->user_data < alloc->buffer);
+ BUG_ON(buffer->user_data > alloc->buffer + alloc->buffer_size);
if (buffer->async_transaction) {
alloc->free_async_space += size + sizeof(struct binder_buffer);
@@ -625,8 +627,9 @@ static void binder_free_buf_locked(struct binder_alloc *alloc,
}
binder_update_page_range(alloc, 0,
- (void *)PAGE_ALIGN((uintptr_t)buffer->data),
- (void *)(((uintptr_t)buffer->data + buffer_size) & PAGE_MASK));
+ (void __user *)PAGE_ALIGN((uintptr_t)buffer->user_data),
+ (void __user *)(((uintptr_t)
+ buffer->user_data + buffer_size) & PAGE_MASK));
rb_erase(&buffer->rb_node, &alloc->allocated_buffers);
buffer->free = 1;
@@ -692,7 +695,7 @@ int binder_alloc_mmap_handler(struct binder_alloc *alloc,
goto err_already_mapped;
}
- alloc->buffer = (void *)vma->vm_start;
+ alloc->buffer = (void __user *)vma->vm_start;
mutex_unlock(&binder_alloc_mmap_lock);
alloc->pages = kcalloc((vma->vm_end - vma->vm_start) / PAGE_SIZE,
@@ -712,7 +715,7 @@ int binder_alloc_mmap_handler(struct binder_alloc *alloc,
goto err_alloc_buf_struct_failed;
}
- buffer->data = alloc->buffer;
+ buffer->user_data = alloc->buffer;
list_add(&buffer->entry, &alloc->buffers);
buffer->free = 1;
binder_insert_free_buffer(alloc, buffer);
@@ -773,7 +776,7 @@ void binder_alloc_deferred_release(struct binder_alloc *alloc)
int i;
for (i = 0; i < alloc->buffer_size / PAGE_SIZE; i++) {
- void *page_addr;
+ void __user *page_addr;
bool on_lru;
if (!alloc->pages[i].page_ptr)
@@ -804,7 +807,7 @@ static void print_binder_buffer(struct seq_file *m, const char *prefix,
struct binder_buffer *buffer)
{
seq_printf(m, "%s %d: %pK size %zd:%zd:%zd %s\n",
- prefix, buffer->debug_id, buffer->data,
+ prefix, buffer->debug_id, buffer->user_data,
buffer->data_size, buffer->offsets_size,
buffer->extra_buffers_size,
buffer->transaction ? "active" : "delivered");
@@ -1056,7 +1059,7 @@ static inline bool check_buffer(struct binder_alloc *alloc,
* @pgoffp: address to copy final page offset to
*
* Lookup the struct page corresponding to the address
- * at @buffer_offset into @buffer->data. If @pgoffp is not
+ * at @buffer_offset into @buffer->user_data. If @pgoffp is not
* NULL, the byte-offset into the page is written there.
*
* The caller is responsible to ensure that the offset points
@@ -1073,7 +1076,7 @@ static struct page *binder_alloc_get_page(struct binder_alloc *alloc,
pgoff_t *pgoffp)
{
binder_size_t buffer_space_offset = buffer_offset +
- (buffer->data - alloc->buffer);
+ (buffer->user_data - alloc->buffer);
pgoff_t pgoff = buffer_space_offset & ~PAGE_MASK;
size_t index = buffer_space_offset >> PAGE_SHIFT;
struct binder_lru_page *lru_page;
diff --git a/drivers/android/binder_alloc.h b/drivers/android/binder_alloc.h
index 1026e9fb20db1..b60d161b7a7ae 100644
--- a/drivers/android/binder_alloc.h
+++ b/drivers/android/binder_alloc.h
@@ -40,7 +40,7 @@ struct binder_transaction;
* @data_size: size of @transaction data
* @offsets_size: size of array of offsets
* @extra_buffers_size: size of space for other objects (like sg lists)
- * @data: pointer to base of buffer space
+ * @user_data: user pointer to base of buffer space
*
* Bookkeeping structure for binder transaction buffers
*/
@@ -59,7 +59,7 @@ struct binder_buffer {
size_t data_size;
size_t offsets_size;
size_t extra_buffers_size;
- void *data;
+ void __user *user_data;
};
/**
@@ -102,7 +102,7 @@ struct binder_alloc {
struct mutex mutex;
struct vm_area_struct *vma;
struct mm_struct *vma_vm_mm;
- void *buffer;
+ void __user *buffer;
struct list_head buffers;
struct rb_root free_buffers;
struct rb_root allocated_buffers;
diff --git a/drivers/android/binder_alloc_selftest.c b/drivers/android/binder_alloc_selftest.c
index 8bd7bcef967d2..f0f4d7d026351 100644
--- a/drivers/android/binder_alloc_selftest.c
+++ b/drivers/android/binder_alloc_selftest.c
@@ -105,8 +105,8 @@ static bool check_buffer_pages_allocated(struct binder_alloc *alloc,
void *page_addr, *end;
int page_index;
- end = (void *)PAGE_ALIGN((uintptr_t)buffer->data + size);
- page_addr = buffer->data;
+ end = (void *)PAGE_ALIGN((uintptr_t)buffer->user_data + size);
+ page_addr = buffer->user_data;
for (; page_addr < end; page_addr += PAGE_SIZE) {
page_index = (page_addr - alloc->buffer) / PAGE_SIZE;
if (!alloc->pages[page_index].page_ptr ||
diff --git a/drivers/android/binder_trace.h b/drivers/android/binder_trace.h
index 14de7ac57a34d..83cc254d2335a 100644
--- a/drivers/android/binder_trace.h
+++ b/drivers/android/binder_trace.h
@@ -293,7 +293,7 @@ DEFINE_EVENT(binder_buffer_class, binder_transaction_failed_buffer_release,
TRACE_EVENT(binder_update_page_range,
TP_PROTO(struct binder_alloc *alloc, bool allocate,
- void *start, void *end),
+ void __user *start, void __user *end),
TP_ARGS(alloc, allocate, start, end),
TP_STRUCT__entry(
__field(int, proc)
--
2.20.1.791.gb4d0f1c61a-goog
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v3 0/7] binder: eliminate use of vmalloc space for binder buffers
2019-02-08 18:35 [PATCH v3 0/7] binder: eliminate use of vmalloc space for binder buffers Todd Kjos
` (6 preceding siblings ...)
2019-02-08 18:35 ` [PATCH v3 7/7] binder: use userspace pointer as base of buffer space Todd Kjos
@ 2019-02-11 16:57 ` Christoph Hellwig
2019-02-11 17:08 ` Todd Kjos
7 siblings, 1 reply; 16+ messages in thread
From: Christoph Hellwig @ 2019-02-11 16:57 UTC (permalink / raw)
To: Todd Kjos
Cc: tkjos, gregkh, arve, devel, linux-kernel, maco, joel, kernel-team
On Fri, Feb 08, 2019 at 10:35:13AM -0800, Todd Kjos wrote:
> Binder buffers have always been mapped into kernel space
> via map_kernel_range_noflush() to allow the binder driver
> to modify the buffer before posting to userspace for
> processing.
>
> In recent Android releases, the number of long-running
> binder processes has increased to the point that for
> 32-bit systems, there is a risk of running out of
> vmalloc space.
>
> This patch set removes the persistent mapping of the
> binder buffers into kernel space. Instead, the binder
> driver creates temporary mappings with kmap() or
> kmap_atomic() to copy to or from the buffer only when
> necessary.
Is there any good reason to actually map the user memory to kernel
space instead of just using copy_{to,from}_user?
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v3 0/7] binder: eliminate use of vmalloc space for binder buffers
2019-02-11 16:57 ` [PATCH v3 0/7] binder: eliminate use of vmalloc space for binder buffers Christoph Hellwig
@ 2019-02-11 17:08 ` Todd Kjos
0 siblings, 0 replies; 16+ messages in thread
From: Todd Kjos @ 2019-02-11 17:08 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Todd Kjos, Greg Kroah-Hartman, Arve Hjønnevåg,
open list:ANDROID DRIVERS, LKML, Martijn Coenen, joel,
Android Kernel Team
On Mon, Feb 11, 2019 at 8:57 AM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Fri, Feb 08, 2019 at 10:35:13AM -0800, Todd Kjos wrote:
> > Binder buffers have always been mapped into kernel space
> > via map_kernel_range_noflush() to allow the binder driver
> > to modify the buffer before posting to userspace for
> > processing.
> >
> > In recent Android releases, the number of long-running
> > binder processes has increased to the point that for
> > 32-bit systems, there is a risk of running out of
> > vmalloc space.
> >
> > This patch set removes the persistent mapping of the
> > binder buffers into kernel space. Instead, the binder
> > driver creates temporary mappings with kmap() or
> > kmap_atomic() to copy to or from the buffer only when
> > necessary.
>
> Is there any good reason to actually map the user memory to kernel
> space instead of just using copy_{to,from}_user?
Yes, the mappings are needed for cases where we are accessing binder
buffers of the target while in sender context. For example, we copy
the message from the sender to the target with 1 copy while in the
sender's context. For this we use copy_from_user(), but use these
temporary mappings for the destination (target process).
-Todd
^ permalink raw reply [flat|nested] 16+ messages in thread