Re: [PATCH net-next v8 2/2] binder: report txn errors via generic netlink

[Jakub Kicinski]

On Wed, 13 Nov 2024 11:32:39 -0800 Li Li wrote:
> +/**
> + * binder_find_proc() - set binder report flags
> + * @pid:	the target process
> + */
> +static struct binder_proc *binder_find_proc(int pid)
> +{
> +	struct binder_proc *proc;
> +
> +	mutex_lock(&binder_procs_lock);
> +	hlist_for_each_entry(proc, &binder_procs, proc_node) {
> +		if (proc->pid == pid) {
> +			mutex_unlock(&binder_procs_lock);
> +			return proc;
> +		}
> +	}
> +	mutex_unlock(&binder_procs_lock);
> +
> +	return NULL;
> +}
> +
> +/**
> + * binder_genl_set_report() - set binder report flags
> + * @context:	the binder context to set the flags
> + * @pid:	the target process
> + * @flags:	the flags to set
> + *
> + * If pid is 0, the flags are applied to the whole binder context.
> + * Otherwise, the flags are applied to the specific process only.
> + */
> +static int binder_genl_set_report(struct binder_context *context, u32 pid,
> +				  u32 flags)
> +{
> +	struct binder_proc *proc;
> +
> +	if (flags != (flags & (BINDER_GENL_FLAG_OVERRIDE
> +			| BINDER_GENL_FLAG_FAILED
> +			| BINDER_GENL_FLAG_DELAYED
> +			| BINDER_GENL_FLAG_SPAM))) {
> +		pr_err("Invalid binder report flags: %u\n", flags);
> +		return -EINVAL;

no need, netlink already checks that only bits from the flags are used:

                                    vvvvvvvvvvvvvvvvvvvvvvvvvvvvv
+	[BINDER_GENL_A_CMD_FLAGS] = NLA_POLICY_MASK(NLA_U32, 0xf),

> +	}
> +
> +	if (!pid) {
> +		/* Set the global flags for the whole binder context */
> +		context->report_flags = flags;
> +	} else {
> +		/* Set the per-process flags */
> +		proc = binder_find_proc(pid);
> +		if (!proc) {
> +			pr_err("Invalid binder report pid %u\n", pid);
> +			return -EINVAL;
> +		}
> +
> +		proc->report_flags = flags;
> +	}
> +
> +	return 0;
> +}

> +static void binder_genl_send_report(struct binder_context *context, u32 err,
> +				    u32 pid, u32 tid, u32 to_pid, u32 to_tid,
> +				    u32 reply,
> +				    struct binder_transaction_data *tr)
> +{
> +	int payload;
> +	int ret;
> +	struct sk_buff *skb;
> +	void *hdr;
> +
> +	trace_binder_send_report(context->name, err, pid, tid, to_pid, to_tid,
> +				 reply, tr);
> +
> +	payload = nla_total_size(strlen(context->name) + 1) +
> +		  nla_total_size(sizeof(u32)) * (BINDER_GENL_A_REPORT_MAX - 1);
> +	skb = genlmsg_new(payload + GENL_HDRLEN, GFP_KERNEL);

 genlmsg_new() adds the GENL_HDRLEN already

> +	if (!skb) {
> +		pr_err("Failed to alloc binder genl message\n");
> +		return;
> +	}
> +
> +	hdr = genlmsg_put(skb, 0, atomic_inc_return(&context->report_seq),
> +			  &binder_genl_nl_family, 0, BINDER_GENL_CMD_REPORT);
> +	if (!hdr)
> +		goto free_skb;
> +
> +	if (nla_put_string(skb, BINDER_GENL_A_REPORT_CONTEXT, context->name) ||
> +	    nla_put_u32(skb, BINDER_GENL_A_REPORT_ERR, err) ||
> +	    nla_put_u32(skb, BINDER_GENL_A_REPORT_FROM_PID, pid) ||
> +	    nla_put_u32(skb, BINDER_GENL_A_REPORT_FROM_TID, tid) ||
> +	    nla_put_u32(skb, BINDER_GENL_A_REPORT_TO_PID, to_pid) ||
> +	    nla_put_u32(skb, BINDER_GENL_A_REPORT_TO_TID, to_tid) ||
> +	    nla_put_u32(skb, BINDER_GENL_A_REPORT_REPLY, reply) ||
> +	    nla_put_u32(skb, BINDER_GENL_A_REPORT_FLAGS, tr->flags) ||
> +	    nla_put_u32(skb, BINDER_GENL_A_REPORT_CODE, tr->code) ||
> +	    nla_put_u32(skb, BINDER_GENL_A_REPORT_DATA_SIZE, tr->data_size))
> +		goto cancel_skb;
> +
> +	genlmsg_end(skb, hdr);
> +
> +	ret = genlmsg_unicast(&init_net, skb, context->report_portid);
> +	if (ret < 0)
> +		pr_err("Failed to send binder genl message to %d: %d\n",
> +		       context->report_portid, ret);
> +	return;
> +
> +cancel_skb:
> +	pr_err("Failed to add report attributes to binder genl message\n");
> +	genlmsg_cancel(skb, hdr);
> +free_skb:
> +	pr_err("Free binder genl report message on error\n");
> +	nlmsg_free(skb);
> +}

> +/**
> + * binder_genl_nl_set_doit() - .doit handler for BINDER_GENL_CMD_SET
> + * @skb:	the metadata struct passed from netlink driver
> + * @info:	the generic netlink struct passed from netlink driver
> + *
> + * Implements the .doit function to process binder genl commands.
> + */
> +int binder_genl_nl_set_doit(struct sk_buff *skb, struct genl_info *info)
> +{
> +	int payload;
> +	int portid;
> +	u32 pid;
> +	u32 flags;
> +	void *hdr;
> +	struct binder_device *device;
> +	struct binder_context *context = NULL;
> +
> +	if (GENL_REQ_ATTR_CHECK(info, BINDER_GENL_A_CMD_CONTEXT) ||
> +	    GENL_REQ_ATTR_CHECK(info, BINDER_GENL_A_CMD_PID) ||
> +	    GENL_REQ_ATTR_CHECK(info, BINDER_GENL_A_CMD_FLAGS))
> +		return -EINVAL;
> +
> +	hlist_for_each_entry(device, &binder_devices, hlist) {
> +		if (!nla_strcmp(info->attrs[BINDER_GENL_A_CMD_CONTEXT],
> +				device->context.name)) {
> +			context = &device->context;
> +			break;
> +		}
> +	}
> +
> +	if (!context) {
> +		NL_SET_ERR_MSG(info->extack, "Unknown binder context\n");
> +		return -EINVAL;
> +	}
> +
> +	portid = nlmsg_hdr(skb)->nlmsg_pid;
> +	pid = nla_get_u32(info->attrs[BINDER_GENL_A_CMD_PID]);
> +	flags = nla_get_u32(info->attrs[BINDER_GENL_A_CMD_FLAGS]);
> +
> +	if (context->report_portid && context->report_portid != portid) {
> +		NL_SET_ERR_MSG_FMT(info->extack,
> +				   "No permission to set flags from %d\n",
> +				   portid);
> +		return -EPERM;
> +	}
> +
> +	if (binder_genl_set_report(context, pid, flags) < 0) {
> +		pr_err("Failed to set report flags %u for %u\n", flags, pid);
> +		return -EINVAL;
> +	}
> +
> +	payload = nla_total_size(sizeof(pid)) + nla_total_size(sizeof(flags));
> +	skb = genlmsg_new(payload + GENL_HDRLEN, GFP_KERNEL);
> +	if (!skb) {
> +		pr_err("Failed to alloc binder genl reply message\n");
> +		return -ENOMEM;

no need for error messages on allocation failures, kernel will print an
OOM report

> +	}
> +
> +	hdr = genlmsg_iput(skb, info);
> +	if (!hdr)
> +		goto free_skb;
> +
> +	if (nla_put_string(skb, BINDER_GENL_A_CMD_CONTEXT, context->name) ||

Have you counted strlen(context->name) to payload?
TBH for small messages counting payload size is probably an overkill,
you can use NLMSG_GOODSIZE as the size of the skb.

> +	    nla_put_u32(skb, BINDER_GENL_A_CMD_PID, pid) ||
> +	    nla_put_u32(skb, BINDER_GENL_A_CMD_FLAGS, flags))
> +		goto cancel_skb;
> +
> +	genlmsg_end(skb, hdr);
> +
> +	if (genlmsg_reply(skb, info)) {
> +		pr_err("Failed to send binder genl reply message\n");
> +		return -EFAULT;
> +	}
> +
> +	if (!context->report_portid)
> +		context->report_portid = portid;

Is there any locking required?

> +	return 0;
> +
> +cancel_skb:
> +	pr_err("Failed to add reply attributes to binder genl message\n");
> +	genlmsg_cancel(skb, hdr);
> +free_skb:
> +	pr_err("Free binder genl reply message on error\n");
> +	nlmsg_free(skb);
> +	return -EMSGSIZE;
> +}