Re: [PATCH v11 2/2] binder: report txn errors via generic netlink

[Jakub Kicinski <kuba@kernel.org>]

On Mon, 13 Jan 2025 22:01:00 -0800 Li Li wrote:
> On Thu, Jan 9, 2025 at 4:18 PM Jakub Kicinski <kuba@kernel.org> wrote:
> > > Sorry, it was me that suggested NETLINK_URELEASE. BTW, I did try those
> > > genl_family callbacks first but I couldn't get them to work right away
> > > so I moved on. I'll have a closer look now to figure out what I did
> > > wrong. Thanks for the suggestion Jakub!  
> >
> > Hm, that's probably because there is no real multicast group here :(
> > genl_sk_priv_get() and co. may work better in that case.
> > your suggestion of NETLINK_URELEASE may work too, tho, I think it's
> > the most error prone  
> 
> sock_priv_destroy works with genl_sk_priv_get().
> 
> But, I have to manually modify the generated netlink header file to satisfy CFI.
> 
> -void binder_nl_sock_priv_init(struct my_struct *priv);
> -void binder_nl_sock_priv_destroy(struct my_struct *priv);
> +void binder_nl_sock_priv_init(void *priv);
> +void binder_nl_sock_priv_destroy(void *priv);
> 
> The reason is that these 2 callback functions are defined in
> include/net/genetlink.h as below
> void (*sock_priv_init)(void *priv);
> void (*sock_priv_destroy)(void *priv);
> 
> Otherwise, kernel panic when CFI is enabled.
> 
> CFI failure at genl_sk_priv_get+0x60/0x138 (target:
> binder_nl_sock_priv_init+0x0/0x34; expected type: 0x0ef81b7d)
> 
> Jakub, we probably need this patch. Please let me know if you have a
> better idea. Thanks!
> 
> diff --git a/tools/net/ynl/ynl-gen-c.py b/tools/net/ynl/ynl-gen-c.py
> index 8155ff6d2a38..84033938a75f 100755
> --- a/tools/net/ynl/ynl-gen-c.py
> +++ b/tools/net/ynl/ynl-gen-c.py
> @@ -2352,8 +2352,8 @@ def print_kernel_family_struct_hdr(family, cw):
>      cw.p(f"extern struct genl_family {family.c_name}_nl_family;")
>      cw.nl()
>      if 'sock-priv' in family.kernel_family:
> -        cw.p(f'void
> {family.c_name}_nl_sock_priv_init({family.kernel_family["sock-priv"]}
> *priv);')
> -        cw.p(f'void
> {family.c_name}_nl_sock_priv_destroy({family.kernel_family["sock-priv"]}
> *priv);')
> +        cw.p(f'void {family.c_name}_nl_sock_priv_init(void *priv);')
> +        cw.p(f'void {family.c_name}_nl_sock_priv_destroy(void *priv);')
>          cw.nl()
> 

Maybe we can codegen a little wrapper call. Can you try this with CFI?

---->8------------

diff --git a/tools/net/ynl/pyynl/ynl_gen_c.py b/tools/net/ynl/pyynl/ynl_gen_c.py
index d3a7dfbcf929..9852ba6fd9c3 100755
--- a/tools/net/ynl/pyynl/ynl_gen_c.py
+++ b/tools/net/ynl/pyynl/ynl_gen_c.py
@@ -2411,6 +2411,15 @@ _C_KW = {
     if not kernel_can_gen_family_struct(family):
         return
 
+    if 'sock-priv' in family.kernel_family:
+        # Generate "trampolines" to make CFI happy
+        cw.write_func("static void", f"__{family.c_name}_nl_sock_priv_init",
+                      [f"{family.c_name}_nl_sock_priv_init(priv);"], ["void *priv"])
+        cw.nl()
+        cw.write_func("static void", f"__{family.c_name}_nl_sock_priv_destroy",
+                      [f"{family.c_name}_nl_sock_priv_destroy(priv);"], ["void *priv"])
+        cw.nl()
+
     cw.block_start(f"struct genl_family {family.ident_name}_nl_family __ro_after_init =")
     cw.p('.name\t\t= ' + family.fam_key + ',')
     cw.p('.version\t= ' + family.ver_key + ',')
@@ -2428,9 +2437,8 @@ _C_KW = {
         cw.p(f'.n_mcgrps\t= ARRAY_SIZE({family.c_name}_nl_mcgrps),')
     if 'sock-priv' in family.kernel_family:
         cw.p(f'.sock_priv_size\t= sizeof({family.kernel_family["sock-priv"]}),')
-        # Force cast here, actual helpers take pointer to the real type.
-        cw.p(f'.sock_priv_init\t= (void *){family.c_name}_nl_sock_priv_init,')
-        cw.p(f'.sock_priv_destroy = (void *){family.c_name}_nl_sock_priv_destroy,')
+        cw.p(f'.sock_priv_init\t= __{family.c_name}_nl_sock_priv_init,')
+        cw.p(f'.sock_priv_destroy = __{family.c_name}_nl_sock_priv_destroy,')
     cw.block_end(';')
 
 
-- 
2.47.1