Re: [PATCH] ANDROID: binder: Add BINDER_GET_NODE_INFO_FOR_REF ioctl.
On Wed, Sep 5, 2018 at 11:09 AM, Dan Carpenter wrote: > What's the reserved for? On 64 bit systems there is a 4 byte struct > hole between weak_count and reserved. There's many more pieces of information that we hold for a node. While we don't have a use for most of that now, we may want some of it in the future, and so I thought it would be wise to reserve some space here so we don't need a new ioctl when that happens. I'm actually not sure it's common to do things this way. > Why not just make reserved a > __u32 and get rid of the hole? (Not rhetorical, I have no idea). Because I thought 8 bytes of reserved space would be nice :-) But you have a good point re:alignment, I should make it two __u32's then. > > Btw, people sometimes complain about that we don't check that user input > is zeroed in ioctls. Like for example maybe they're passing random data > in the the strong_count field and then later we decide that actually > that field should mean something but we can't make it mean anything > because we've been letting the user put whatever they want there. These > are just random thoughts in my head, not necessarily important. That's a good point, I will change the code to check for that. Thanks, Martijn > > regards, > dan carpenter >
Re: [PATCH] ANDROID: binder: Add BINDER_GET_NODE_INFO_FOR_REF ioctl.
On Wed, Sep 5, 2018 at 11:09 AM, Dan Carpenter wrote: > What's the reserved for? On 64 bit systems there is a 4 byte struct > hole between weak_count and reserved. There's many more pieces of information that we hold for a node. While we don't have a use for most of that now, we may want some of it in the future, and so I thought it would be wise to reserve some space here so we don't need a new ioctl when that happens. I'm actually not sure it's common to do things this way. > Why not just make reserved a > __u32 and get rid of the hole? (Not rhetorical, I have no idea). Because I thought 8 bytes of reserved space would be nice :-) But you have a good point re:alignment, I should make it two __u32's then. > > Btw, people sometimes complain about that we don't check that user input > is zeroed in ioctls. Like for example maybe they're passing random data > in the the strong_count field and then later we decide that actually > that field should mean something but we can't make it mean anything > because we've been letting the user put whatever they want there. These > are just random thoughts in my head, not necessarily important. That's a good point, I will change the code to check for that. Thanks, Martijn > > regards, > dan carpenter >
Re: [PATCH] ANDROID: binder: Add BINDER_GET_NODE_INFO_FOR_REF ioctl.
On Wed, Sep 05, 2018 at 09:33:46AM +0200, Martijn Coenen wrote:
> diff --git a/include/uapi/linux/android/binder.h
> b/include/uapi/linux/android/binder.h
> index bfaec6903b8bc..a54a680ff2936 100644
> --- a/include/uapi/linux/android/binder.h
> +++ b/include/uapi/linux/android/binder.h
> @@ -200,6 +200,13 @@ struct binder_node_debug_info {
> __u32has_weak_ref;
> };
>
> +struct binder_node_info_for_ref {
> + __u32handle;
> + __u32strong_count;
> + __u32weak_count;
> + __u64reserved;
> +};
What's the reserved for? On 64 bit systems there is a 4 byte struct
hole between weak_count and reserved. Why not just make reserved a
__u32 and get rid of the hole? (Not rhetorical, I have no idea).
Btw, people sometimes complain about that we don't check that user input
is zeroed in ioctls. Like for example maybe they're passing random data
in the the strong_count field and then later we decide that actually
that field should mean something but we can't make it mean anything
because we've been letting the user put whatever they want there. These
are just random thoughts in my head, not necessarily important.
regards,
dan carpenter
Re: [PATCH] ANDROID: binder: Add BINDER_GET_NODE_INFO_FOR_REF ioctl.
On Wed, Sep 05, 2018 at 09:33:46AM +0200, Martijn Coenen wrote:
> diff --git a/include/uapi/linux/android/binder.h
> b/include/uapi/linux/android/binder.h
> index bfaec6903b8bc..a54a680ff2936 100644
> --- a/include/uapi/linux/android/binder.h
> +++ b/include/uapi/linux/android/binder.h
> @@ -200,6 +200,13 @@ struct binder_node_debug_info {
> __u32has_weak_ref;
> };
>
> +struct binder_node_info_for_ref {
> + __u32handle;
> + __u32strong_count;
> + __u32weak_count;
> + __u64reserved;
> +};
What's the reserved for? On 64 bit systems there is a 4 byte struct
hole between weak_count and reserved. Why not just make reserved a
__u32 and get rid of the hole? (Not rhetorical, I have no idea).
Btw, people sometimes complain about that we don't check that user input
is zeroed in ioctls. Like for example maybe they're passing random data
in the the strong_count field and then later we decide that actually
that field should mean something but we can't make it mean anything
because we've been letting the user put whatever they want there. These
are just random thoughts in my head, not necessarily important.
regards,
dan carpenter
[PATCH] ANDROID: binder: Add BINDER_GET_NODE_INFO_FOR_REF ioctl.
This allows the context manager to retrieve information about nodes
that it holds a reference to, such as the current number of
references to those nodes.
Such information can for example be used to determine whether the
servicemanager is the only process holding a reference to a node.
This information can then be passed on to the process holding the
node, which can in turn decide whether it wants to shut down to
reduce resource usage.
Signed-off-by: Martijn Coenen
---
drivers/android/binder.c| 50 +
include/uapi/linux/android/binder.h | 8 +
2 files changed, 58 insertions(+)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index d58763b6b0090..1c7e965241fb8 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -4544,6 +4544,37 @@ static int binder_ioctl_set_ctx_mgr(struct file *filp)
return ret;
}
+static int binder_ioctl_get_node_info_for_ref(struct binder_proc *proc,
+ struct binder_node_info_for_ref *info)
+{
+ struct binder_node *node;
+ struct binder_context *context = proc->context;
+ __u32 handle = info->handle;
+
+ memset(info, 0, sizeof(*info));
+
+ /* This ioctl may only be used by the context manager */
+ mutex_lock(>context_mgr_node_lock);
+ if (!context->binder_context_mgr_node ||
+ context->binder_context_mgr_node->proc != proc) {
+ mutex_unlock(>context_mgr_node_lock);
+ return -EPERM;
+ }
+ mutex_unlock(>context_mgr_node_lock);
+
+ node = binder_get_node_from_ref(proc, handle, true, NULL);
+ if (!node)
+ return -EINVAL;
+
+ info->strong_count = node->local_strong_refs +
+ node->internal_strong_refs;
+ info->weak_count = node->local_weak_refs;
+
+ binder_put_node(node);
+
+ return 0;
+}
+
static int binder_ioctl_get_node_debug_info(struct binder_proc *proc,
struct binder_node_debug_info *info)
{
@@ -4638,6 +4669,25 @@ static long binder_ioctl(struct file *filp, unsigned int
cmd, unsigned long arg)
}
break;
}
+ case BINDER_GET_NODE_INFO_FOR_REF: {
+ struct binder_node_info_for_ref info;
+
+ if (copy_from_user(, ubuf, sizeof(info))) {
+ ret = -EFAULT;
+ goto err;
+ }
+
+ ret = binder_ioctl_get_node_info_for_ref(proc, );
+ if (ret < 0)
+ goto err;
+
+ if (copy_to_user(ubuf, , sizeof(info))) {
+ ret = -EFAULT;
+ goto err;
+ }
+
+ break;
+ }
case BINDER_GET_NODE_DEBUG_INFO: {
struct binder_node_debug_info info;
diff --git a/include/uapi/linux/android/binder.h
b/include/uapi/linux/android/binder.h
index bfaec6903b8bc..a54a680ff2936 100644
--- a/include/uapi/linux/android/binder.h
+++ b/include/uapi/linux/android/binder.h
@@ -200,6 +200,13 @@ struct binder_node_debug_info {
__u32has_weak_ref;
};
+struct binder_node_info_for_ref {
+ __u32handle;
+ __u32strong_count;
+ __u32weak_count;
+ __u64reserved;
+};
+
#define BINDER_WRITE_READ _IOWR('b', 1, struct binder_write_read)
#define BINDER_SET_IDLE_TIMEOUT_IOW('b', 3, __s64)
#define BINDER_SET_MAX_THREADS _IOW('b', 5, __u32)
@@ -208,6 +215,7 @@ struct binder_node_debug_info {
#define BINDER_THREAD_EXIT _IOW('b', 8, __s32)
#define BINDER_VERSION _IOWR('b', 9, struct binder_version)
#define BINDER_GET_NODE_DEBUG_INFO _IOWR('b', 11, struct
binder_node_debug_info)
+#define BINDER_GET_NODE_INFO_FOR_REF _IOWR('b', 12, struct
binder_node_info_for_ref)
/*
* NOTE: Two special error codes you should check for when calling
--
2.19.0.rc1.350.ge57e33dbd1-goog
[PATCH] ANDROID: binder: Add BINDER_GET_NODE_INFO_FOR_REF ioctl.
This allows the context manager to retrieve information about nodes
that it holds a reference to, such as the current number of
references to those nodes.
Such information can for example be used to determine whether the
servicemanager is the only process holding a reference to a node.
This information can then be passed on to the process holding the
node, which can in turn decide whether it wants to shut down to
reduce resource usage.
Signed-off-by: Martijn Coenen
---
drivers/android/binder.c| 50 +
include/uapi/linux/android/binder.h | 8 +
2 files changed, 58 insertions(+)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index d58763b6b0090..1c7e965241fb8 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -4544,6 +4544,37 @@ static int binder_ioctl_set_ctx_mgr(struct file *filp)
return ret;
}
+static int binder_ioctl_get_node_info_for_ref(struct binder_proc *proc,
+ struct binder_node_info_for_ref *info)
+{
+ struct binder_node *node;
+ struct binder_context *context = proc->context;
+ __u32 handle = info->handle;
+
+ memset(info, 0, sizeof(*info));
+
+ /* This ioctl may only be used by the context manager */
+ mutex_lock(>context_mgr_node_lock);
+ if (!context->binder_context_mgr_node ||
+ context->binder_context_mgr_node->proc != proc) {
+ mutex_unlock(>context_mgr_node_lock);
+ return -EPERM;
+ }
+ mutex_unlock(>context_mgr_node_lock);
+
+ node = binder_get_node_from_ref(proc, handle, true, NULL);
+ if (!node)
+ return -EINVAL;
+
+ info->strong_count = node->local_strong_refs +
+ node->internal_strong_refs;
+ info->weak_count = node->local_weak_refs;
+
+ binder_put_node(node);
+
+ return 0;
+}
+
static int binder_ioctl_get_node_debug_info(struct binder_proc *proc,
struct binder_node_debug_info *info)
{
@@ -4638,6 +4669,25 @@ static long binder_ioctl(struct file *filp, unsigned int
cmd, unsigned long arg)
}
break;
}
+ case BINDER_GET_NODE_INFO_FOR_REF: {
+ struct binder_node_info_for_ref info;
+
+ if (copy_from_user(, ubuf, sizeof(info))) {
+ ret = -EFAULT;
+ goto err;
+ }
+
+ ret = binder_ioctl_get_node_info_for_ref(proc, );
+ if (ret < 0)
+ goto err;
+
+ if (copy_to_user(ubuf, , sizeof(info))) {
+ ret = -EFAULT;
+ goto err;
+ }
+
+ break;
+ }
case BINDER_GET_NODE_DEBUG_INFO: {
struct binder_node_debug_info info;
diff --git a/include/uapi/linux/android/binder.h
b/include/uapi/linux/android/binder.h
index bfaec6903b8bc..a54a680ff2936 100644
--- a/include/uapi/linux/android/binder.h
+++ b/include/uapi/linux/android/binder.h
@@ -200,6 +200,13 @@ struct binder_node_debug_info {
__u32has_weak_ref;
};
+struct binder_node_info_for_ref {
+ __u32handle;
+ __u32strong_count;
+ __u32weak_count;
+ __u64reserved;
+};
+
#define BINDER_WRITE_READ _IOWR('b', 1, struct binder_write_read)
#define BINDER_SET_IDLE_TIMEOUT_IOW('b', 3, __s64)
#define BINDER_SET_MAX_THREADS _IOW('b', 5, __u32)
@@ -208,6 +215,7 @@ struct binder_node_debug_info {
#define BINDER_THREAD_EXIT _IOW('b', 8, __s32)
#define BINDER_VERSION _IOWR('b', 9, struct binder_version)
#define BINDER_GET_NODE_DEBUG_INFO _IOWR('b', 11, struct
binder_node_debug_info)
+#define BINDER_GET_NODE_INFO_FOR_REF _IOWR('b', 12, struct
binder_node_info_for_ref)
/*
* NOTE: Two special error codes you should check for when calling
--
2.19.0.rc1.350.ge57e33dbd1-goog
