Re: [PATCH v4 1/1] binder: report txn errors via generic netlink (genl)

[Simon Horman <horms@kernel.org>]

On Tue, Oct 22, 2024 at 11:11:13AM -0700, Li Li wrote:
> On Tue, Oct 22, 2024 at 10:06 AM Simon Horman <horms@kernel.org> wrote:
> >
> > On Mon, Oct 21, 2024 at 12:12:33PM -0700, Li Li wrote:
> > > + * @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)
> >
> > ...
> >
> > >  static int __init init_binder_device(const char *name)
> > >  {
> > >       int ret;
> > > @@ -6920,6 +7196,11 @@ static int __init init_binder_device(const char *name)
> >
> > The code above this hunk looks like this:
> >
> >
> >         ret = misc_register(&binder_device->miscdev);
> >         if (ret < 0) {
> >                 kfree(binder_device);
> >                 return ret;
> >         }
> >
> > >
> > >       hlist_add_head(&binder_device->hlist, &binder_devices);
> > >
> > > +     binder_device->context.report_seq = (atomic_t)ATOMIC_INIT(0);
> > > +     ret = binder_genl_init(&binder_device->context.genl_family, name);
> > > +     if (ret < 0)
> > > +             kfree(binder_device);
> >
> > So I think that binder_device->miscdev needs to be misc_deregister'ed
> > if we hit this error condition.
> >
> > > +
> > >       return ret;
> >
> > Probably adding an unwind ladder like this makes sense (completely untested!):
> >
> >         ret = misc_register(&binder_device->miscdev);
> >         if (ret < 0)
> >                 goto err_misc_deregister;

This should be:

		goto err_free_dev;

> >
> >         hlist_add_head(&binder_device->hlist, &binder_devices);
> >
> >         binder_device->context.report_seq = (atomic_t)ATOMIC_INIT(0);
> >         ret = binder_genl_init(&binder_device->context.genl_family, name);
> >         if (ret < 0);
> >                 goto err_misc_deregister;
> >
> >         return 0;
> >
> > err_misc_deregister:
> >         misc_deregister(&binder_device->miscdev);
> > err_free_dev:
> >         kfree(binder_device);
> >         return ret;
> >
> 
> Good catch! Will fix it in the next revision.

Thanks.

> 
> > ...
> >
> > > diff --git a/drivers/android/binder_genl.h b/drivers/android/binder_genl.h
> >
> > Perhaps it is because of a different version of net-next,
> > but with this patch applied on top of the current head commit
> > 13feb6074a9f ("binder: report txn errors via generic netlink (genl)")
> > I see:
> >
> > $ ./tools/net/ynl/ynl-regen.sh -f
> > $ git diff
> >
> > diff --git a/include/uapi/linux/android/binder_genl.h b/include/uapi/linux/android/binder_genl.h
> > index ef5289133be5..93e58b370420 100644
> > --- a/include/uapi/linux/android/binder_genl.h
> > +++ b/include/uapi/linux/android/binder_genl.h
> > @@ -3,12 +3,17 @@
> >  /*     Documentation/netlink/specs/binder_genl.yaml */
> >  /* YNL-GEN uapi header */
> >
> > -#ifndef _UAPI_LINUX_BINDER_GENL_H
> > -#define _UAPI_LINUX_BINDER_GENL_H
> > +#ifndef _UAPI_LINUX_ANDROID/BINDER_GENL_H
> > +#define _UAPI_LINUX_ANDROID/BINDER_GENL_H
> >
> >  #define BINDER_GENL_FAMILY_NAME                "binder_genl"
> >  #define BINDER_GENL_FAMILY_VERSION     1
> >
> > +/**
> > + * enum binder_genl_flag - Used with "set" and "reply" command below, defining
> > + *   what kind \ of binder transactions should be reported to the user space \
> > + *   administration process.
> > + */
> >  enum binder_genl_flag {
> >         BINDER_GENL_FLAG_FAILED = 1,
> >         BINDER_GENL_FLAG_DELAYED = 2,
> > @@ -34,4 +39,4 @@ enum {
> >         BINDER_GENL_CMD_MAX = (__BINDER_GENL_CMD_MAX - 1)
> >  };
> >
> > -#endif /* _UAPI_LINUX_BINDER_GENL_H */
> > +#endif /* _UAPI_LINUX_ANDROID/BINDER_GENL_H */
> 
> The patch was based on the top of Linus's tree instead of net-next.
> I'll investigate this difference.

Likewise, thanks.
If this patch is targeted at net-next you should base it on that tree.

Also, on that topic, if so the target should be noted in the subject.

	Subject: [PATCH net-next vX] ...