[PATCH] ANDROID: binder: Add BINDER_GET_NODE_INFO_FOR_REF ioctl.

[PATCH] ANDROID: binder: Add BINDER_GET_NODE_INFO_FOR_REF ioctl.

[Martijn Coenen]

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 <maco@android.com>
---
 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->context_mgr_node_lock);
+	if (!context->binder_context_mgr_node ||
+		context->binder_context_mgr_node->proc != proc) {
+		mutex_unlock(&context->context_mgr_node_lock);
+		return -EPERM;
+	}
+	mutex_unlock(&context->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(&info, ubuf, sizeof(info))) {
+			ret = -EFAULT;
+			goto err;
+		}
+
+		ret = binder_ioctl_get_node_info_for_ref(proc, &info);
+		if (ret < 0)
+			goto err;
+
+		if (copy_to_user(ubuf, &info, 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 {
 	__u32            has_weak_ref;
 };
 
+struct binder_node_info_for_ref {
+	__u32            handle;
+	__u32            strong_count;
+	__u32            weak_count;
+	__u64            reserved;
+};
+
 #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

Re: [PATCH] ANDROID: binder: Add BINDER_GET_NODE_INFO_FOR_REF ioctl.

[Dan Carpenter]

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 {
>  	__u32            has_weak_ref;
>  };
>  
> +struct binder_node_info_for_ref {
> +	__u32            handle;
> +	__u32            strong_count;
> +	__u32            weak_count;
> +	__u64            reserved;
> +};

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.

[Martijn Coenen]

On Wed, Sep 5, 2018 at 11:09 AM, Dan Carpenter <dan.carpenter@oracle.com> 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
>