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

[Li Li]

On Wed, Dec 4, 2024 at 6:35 PM Jakub Kicinski <kuba@xxxxxxxxxx> wrote:
>
> On Wed, 13 Nov 2024 11:32:39 -0800 Li Li wrote:
> > +     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:

Ah, yes, let me remove this redundant check.

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

> > +     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
>

Yes, genlmsg_new calls genlmsg_msg_size to include GENL_HDRLEN already.
I'll just use NLMSG_DEFAULT_SIZE as suggested below.

> > +     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
>

Will remove this unnecessary pr_err.

> > +     }
> > +
> > +     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.
>

Yes, the message is known to be small. I'll use GENLMSG_DEFAULT_SIZE instead.

> > +         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?

A lock isn't necessary here. The system administration process always runs
before any other user apps. Even though this is not true, the design is to
allow the first process to claim this netlink. Having a lock doesn't help
in any case.

>
> > +     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;
> > +}
>