[PATCH v13 1/3] tools: ynl-gen: add trampolines for sock-priv

Li Li posted 3 patches 3 weeks ago
There is a newer version of this series
[PATCH v13 1/3] tools: ynl-gen: add trampolines for sock-priv
Posted by Li Li 3 weeks ago
From: Li Li <dualli@google.com>

This fixes the CFI failure at genl-sk_priv_get().

Suggested-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Li Li <dualli@google.com>
---
 tools/net/ynl/pyynl/ynl_gen_c.py | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

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 @@ def print_kernel_family_struct_src(family, cw):
     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 @@ def print_kernel_family_struct_src(family, cw):
         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.48.0.rc2.279.g1de40edade-goog
Re: [PATCH v13 1/3] tools: ynl-gen: add trampolines for sock-priv
Posted by Jakub Kicinski 3 weeks ago
On Wed, 15 Jan 2025 02:29:48 -0800 Li Li wrote:
> From: Li Li <dualli@google.com>
> 
> This fixes the CFI failure at genl-sk_priv_get().
> 
> Suggested-by: Jakub Kicinski <kuba@kernel.org>
> Signed-off-by: Li Li <dualli@google.com>

No, no, this is a fix. We'll try to send it to Linus tomorrow.
Re: [PATCH v13 1/3] tools: ynl-gen: add trampolines for sock-priv
Posted by Li Li 3 weeks ago
On Wed, Jan 15, 2025 at 8:13 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Wed, 15 Jan 2025 02:29:48 -0800 Li Li wrote:
> > From: Li Li <dualli@google.com>
> >
> > This fixes the CFI failure at genl-sk_priv_get().
> >
> > Suggested-by: Jakub Kicinski <kuba@kernel.org>
> > Signed-off-by: Li Li <dualli@google.com>
>
> No, no, this is a fix. We'll try to send it to Linus tomorrow.

Thank you for prioritizing the fix!

There's another trivial issue which I just realized after sending out the
patchset. When "sock-priv" is a pointer (like the example below), ynl-gen
generates ugly code and fails scripts/checkpatch.

Should I use typedef instead although it seems discouraged according to
https://www.kernel.org/doc/html/latest/process/coding-style.html?

YAML:
+  sock-priv: struct binder_context *

==>

Generated code:
+void binder_nl_sock_priv_init(struct binder_context * *priv);
                                                     ^^^ extra space
+void binder_nl_sock_priv_destroy(struct binder_context * *priv);
                                                        ^^^ extra space
Re: [PATCH v13 1/3] tools: ynl-gen: add trampolines for sock-priv
Posted by Jakub Kicinski 3 weeks ago
On Wed, 15 Jan 2025 08:44:37 -0800 Li Li wrote:
> On Wed, Jan 15, 2025 at 8:13 AM Jakub Kicinski <kuba@kernel.org> wrote:
> >
> > On Wed, 15 Jan 2025 02:29:48 -0800 Li Li wrote:  
> > > From: Li Li <dualli@google.com>
> > >
> > > This fixes the CFI failure at genl-sk_priv_get().
> > >
> > > Suggested-by: Jakub Kicinski <kuba@kernel.org>
> > > Signed-off-by: Li Li <dualli@google.com>  
> >
> > No, no, this is a fix. We'll try to send it to Linus tomorrow.  
> 
> Thank you for prioritizing the fix!
> 
> There's another trivial issue which I just realized after sending out the
> patchset. When "sock-priv" is a pointer (like the example below), ynl-gen
> generates ugly code and fails scripts/checkpatch.
> 
> Should I use typedef instead although it seems discouraged according to
> https://www.kernel.org/doc/html/latest/process/coding-style.html?
> 
> YAML:
> +  sock-priv: struct binder_context *

We can adjust that later, but I think cleanest fix may be to wrap the
priv in a separate struct, even if it only has one member.