unwind deferred: Annotate struct unwind_cache with __counted_by

[PATCH] unwind deferred: Annotate struct unwind_cache with __counted_by

Posted by Thorsten Blum 2 months, 3 weeks ago

Add the __counted_by() compiler attribute to the flexible array member
'entries' to improve access bounds-checking via CONFIG_UBSAN_BOUNDS and
CONFIG_FORTIFY_SOURCE.

Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
---
 include/linux/unwind_deferred_types.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/unwind_deferred_types.h b/include/linux/unwind_deferred_types.h
index 33b62ac25c86..d4b67f0116f3 100644
--- a/include/linux/unwind_deferred_types.h
+++ b/include/linux/unwind_deferred_types.h
@@ -5,7 +5,7 @@
 struct unwind_cache {
 	unsigned long		unwind_completed;
 	unsigned int		nr_entries;
-	unsigned long		entries[];
+	unsigned long		entries[] __counted_by(nr_entries);
 };
 
 /*
-- 
2.51.1

Re: [PATCH] unwind deferred: Annotate struct unwind_cache with __counted_by

Posted by Steven Rostedt 2 months, 3 weeks ago

On Fri, 14 Nov 2025 13:27:47 +0100
Thorsten Blum <thorsten.blum@linux.dev> wrote:

> Add the __counted_by() compiler attribute to the flexible array member
> 'entries' to improve access bounds-checking via CONFIG_UBSAN_BOUNDS and
> CONFIG_FORTIFY_SOURCE.
> 
> Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
> ---
>  include/linux/unwind_deferred_types.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/unwind_deferred_types.h b/include/linux/unwind_deferred_types.h
> index 33b62ac25c86..d4b67f0116f3 100644
> --- a/include/linux/unwind_deferred_types.h
> +++ b/include/linux/unwind_deferred_types.h
> @@ -5,7 +5,7 @@
>  struct unwind_cache {
>  	unsigned long		unwind_completed;
>  	unsigned int		nr_entries;
> -	unsigned long		entries[];
> +	unsigned long		entries[] __counted_by(nr_entries);
>  };
>  
>  /*

Wrong!

I need to add a comment here that entries is not bound by nr_entries.

   https://lore.kernel.org/all/20250730093249.4833be14@gandalf.local.home/

Maybe this?:

diff --git a/include/linux/unwind_deferred_types.h b/include/linux/unwind_deferred_types.h
index 33b62ac25c86..d05409bb14fa 100644
--- a/include/linux/unwind_deferred_types.h
+++ b/include/linux/unwind_deferred_types.h
@@ -5,7 +5,7 @@
 struct unwind_cache {
 	unsigned long		unwind_completed;
 	unsigned int		nr_entries;
-	unsigned long		entries[];
+	unsigned long		entries[]; /* Fixed size, not bound by nr_entries */
 };
 
 /*



-- Steve

Re: [PATCH] unwind deferred: Annotate struct unwind_cache with __counted_by

Posted by David Laight 2 months, 3 weeks ago

On Fri, 14 Nov 2025 08:43:46 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Fri, 14 Nov 2025 13:27:47 +0100
> Thorsten Blum <thorsten.blum@linux.dev> wrote:
> 
> > Add the __counted_by() compiler attribute to the flexible array member
> > 'entries' to improve access bounds-checking via CONFIG_UBSAN_BOUNDS and
> > CONFIG_FORTIFY_SOURCE.
> > 
> > Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
> > ---
> >  include/linux/unwind_deferred_types.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/include/linux/unwind_deferred_types.h b/include/linux/unwind_deferred_types.h
> > index 33b62ac25c86..d4b67f0116f3 100644
> > --- a/include/linux/unwind_deferred_types.h
> > +++ b/include/linux/unwind_deferred_types.h
> > @@ -5,7 +5,7 @@
> >  struct unwind_cache {
> >  	unsigned long		unwind_completed;
> >  	unsigned int		nr_entries;
> > -	unsigned long		entries[];
> > +	unsigned long		entries[] __counted_by(nr_entries);
> >  };
> >  
> >  /*  
> 
> Wrong!
> 
> I need to add a comment here that entries is not bound by nr_entries.
> 
>    https://lore.kernel.org/all/20250730093249.4833be14@gandalf.local.home/
> 
> Maybe this?:
> 
> diff --git a/include/linux/unwind_deferred_types.h b/include/linux/unwind_deferred_types.h
> index 33b62ac25c86..d05409bb14fa 100644
> --- a/include/linux/unwind_deferred_types.h
> +++ b/include/linux/unwind_deferred_types.h
> @@ -5,7 +5,7 @@
>  struct unwind_cache {
>  	unsigned long		unwind_completed;

Does that need to be 'long' - 'int' would fit in the padding on 64bit.

>  	unsigned int		nr_entries;
> -	unsigned long		entries[];
> +	unsigned long		entries[]; /* Fixed size, not bound by nr_entries */
>  };

Perhaps it should be:
	unsigned long entries[ /* MAX_UNWIND_ENTRIES */ ];

    David

>  
>  /*
> 
> 
> 
> -- Steve
>

Re: [PATCH] unwind deferred: Annotate struct unwind_cache with __counted_by

Posted by Steven Rostedt 2 months, 3 weeks ago

On Fri, 14 Nov 2025 14:31:04 +0000
David Laight <david.laight.linux@gmail.com> wrote:

> On Fri, 14 Nov 2025 08:43:46 -0500
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > On Fri, 14 Nov 2025 13:27:47 +0100
> > Thorsten Blum <thorsten.blum@linux.dev> wrote:
> >   
> > > Add the __counted_by() compiler attribute to the flexible array member
> > > 'entries' to improve access bounds-checking via CONFIG_UBSAN_BOUNDS and
> > > CONFIG_FORTIFY_SOURCE.
> > > 
> > > Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
> > > ---
> > >  include/linux/unwind_deferred_types.h | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/include/linux/unwind_deferred_types.h b/include/linux/unwind_deferred_types.h
> > > index 33b62ac25c86..d4b67f0116f3 100644
> > > --- a/include/linux/unwind_deferred_types.h
> > > +++ b/include/linux/unwind_deferred_types.h
> > > @@ -5,7 +5,7 @@
> > >  struct unwind_cache {
> > >  	unsigned long		unwind_completed;
> > >  	unsigned int		nr_entries;
> > > -	unsigned long		entries[];
> > > +	unsigned long		entries[] __counted_by(nr_entries);
> > >  };
> > >  
> > >  /*    
> > 
> > Wrong!
> > 
> > I need to add a comment here that entries is not bound by nr_entries.
> > 
> >    https://lore.kernel.org/all/20250730093249.4833be14@gandalf.local.home/
> > 
> > Maybe this?:
> > 
> > diff --git a/include/linux/unwind_deferred_types.h b/include/linux/unwind_deferred_types.h
> > index 33b62ac25c86..d05409bb14fa 100644
> > --- a/include/linux/unwind_deferred_types.h
> > +++ b/include/linux/unwind_deferred_types.h
> > @@ -5,7 +5,7 @@
> >  struct unwind_cache {
> >  	unsigned long		unwind_completed;  
> 
> Does that need to be 'long' - 'int' would fit in the padding on 64bit.

We could make it 32 bit, then the number of tracers attached would be the
same as it would be on 32 bit archs (which would be 30). Maybe that's enough.

> 
> >  	unsigned int		nr_entries;
> > -	unsigned long		entries[];
> > +	unsigned long		entries[]; /* Fixed size, not bound by nr_entries */
> >  };  
> 
> Perhaps it should be:
> 	unsigned long entries[ /* MAX_UNWIND_ENTRIES */ ];

Whatever would keep the coccinelle folks from sending more patches.

-- Steve

Re: [PATCH] unwind deferred: Annotate struct unwind_cache with __counted_by

Posted by Steven Rostedt 2 months, 3 weeks ago

On Fri, 14 Nov 2025 09:56:44 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> >   
> > >  	unsigned int		nr_entries;
> > > -	unsigned long		entries[];
> > > +	unsigned long		entries[]; /* Fixed size, not bound by nr_entries */
> > >  };    
> > 
> > Perhaps it should be:
> > 	unsigned long entries[ /* MAX_UNWIND_ENTRIES */ ];  
> 
> Whatever would keep the coccinelle folks from sending more patches.

Thorsten,

Which comment would you feel is more obvious that entries is not bound by
nr_entries and prevent this patch from being sent again?

-- Steve

Re: [PATCH] unwind deferred: Annotate struct unwind_cache with __counted_by

Posted by Thorsten Blum 2 months, 3 weeks ago

On 14. Nov 2025, at 16:49, Steven Rostedt wrote:
> Which comment would you feel is more obvious that entries is not bound by
> nr_entries and prevent this patch from being sent again?

Both seem fine to me.

Re: [PATCH] unwind deferred: Annotate struct unwind_cache with __counted_by

Posted by Steven Rostedt 2 months, 3 weeks ago

On Fri, 14 Nov 2025 10:49:52 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Fri, 14 Nov 2025 09:56:44 -0500
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > >     
> > > >  	unsigned int		nr_entries;
> > > > -	unsigned long		entries[];
> > > > +	unsigned long		entries[]; /* Fixed size, not bound by nr_entries */
> > > >  };      
> > > 
> > > Perhaps it should be:
> > > 	unsigned long entries[ /* MAX_UNWIND_ENTRIES */ ];    
> > 
> > Whatever would keep the coccinelle folks from sending more patches.  
> 
> Thorsten,
> 
> Which comment would you feel is more obvious that entries is not bound by
> nr_entries and prevent this patch from being sent again?
> 

Or we can simply move the macro to the header:

diff --git a/include/linux/unwind_deferred_types.h b/include/linux/unwind_deferred_types.h
index 18fa3932f61c..0e43556b9710 100644
--- a/include/linux/unwind_deferred_types.h
+++ b/include/linux/unwind_deferred_types.h
@@ -5,10 +5,14 @@
 #include <linux/types.h>
 #include <linux/atomic.h>
 
+/* Make the cache fit in a 4K page */
+#define UNWIND_MAX_ENTRIES					\
+	((SZ_4K - sizeof(struct unwind_cache)) / sizeof(long))
+
 struct unwind_cache {
 	unsigned long		unwind_completed;
 	unsigned int		nr_entries;
-	unsigned long		entries[];
+	unsigned long		entries[ /* UNWIND_MAX_ENTRIES */ ];
 };
 
 /*
diff --git a/kernel/unwind/deferred.c b/kernel/unwind/deferred.c
index a88fb481c4a3..6ea7cb5f336d 100644
--- a/kernel/unwind/deferred.c
+++ b/kernel/unwind/deferred.c
@@ -37,10 +37,6 @@ static inline bool try_assign_cnt(struct unwind_task_info *info, u32 cnt)
 }
 #endif
 
-/* Make the cache fit in a 4K page */
-#define UNWIND_MAX_ENTRIES					\
-	((SZ_4K - sizeof(struct unwind_cache)) / sizeof(long))
-
 /* Guards adding to or removing from the list of callbacks */
 static DEFINE_MUTEX(callback_mutex);
 static LIST_HEAD(callbacks);


-- Steve

Re: [PATCH] unwind deferred: Annotate struct unwind_cache with __counted_by

Posted by Steven Rostedt 2 months, 3 weeks ago

On Fri, 14 Nov 2025 08:43:46 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> I need to add a comment here that entries is not bound by nr_entries.
> 
>    https://lore.kernel.org/all/20250730093249.4833be14@gandalf.local.home/
> 
> Maybe this?:

Or better yet, if this compiles (I haven't tried):

diff --git a/include/linux/unwind_deferred_types.h b/include/linux/unwind_deferred_types.h
index 33b62ac25c86..253a69b21e76 100644
--- a/include/linux/unwind_deferred_types.h
+++ b/include/linux/unwind_deferred_types.h
@@ -2,10 +2,14 @@
 #ifndef _LINUX_UNWIND_USER_DEFERRED_TYPES_H
 #define _LINUX_UNWIND_USER_DEFERRED_TYPES_H
 
+/* Make the cache fit in a 4K page */
+#define UNWIND_MAX_ENTRIES					\
+	((SZ_4K - offset_of(struct unwind_cache, entries)) / sizeof(long))
+
 struct unwind_cache {
 	unsigned long		unwind_completed;
 	unsigned int		nr_entries;
-	unsigned long		entries[];
+	unsigned long		entries[UNWIND_MAX_ENTRIES];
 };
 
 /*
diff --git a/kernel/unwind/deferred.c b/kernel/unwind/deferred.c
index dc6040aae3ee..5dfd0ac264d1 100644
--- a/kernel/unwind/deferred.c
+++ b/kernel/unwind/deferred.c
@@ -37,10 +37,6 @@ static inline bool try_assign_cnt(struct unwind_task_info *info, u32 cnt)
 }
 #endif
 
-/* Make the cache fit in a 4K page */
-#define UNWIND_MAX_ENTRIES					\
-	((SZ_4K - sizeof(struct unwind_cache)) / sizeof(long))
-
 /* Guards adding to or removing from the list of callbacks */
 static DEFINE_MUTEX(callback_mutex);
 static LIST_HEAD(callbacks);
@@ -118,8 +114,7 @@ int unwind_user_faultable(struct unwind_stacktrace *trace)
 		return -EINVAL;
 
 	if (!info->cache) {
-		info->cache = kzalloc(struct_size(cache, entries, UNWIND_MAX_ENTRIES),
-				      GFP_KERNEL);
+		info->cache = kzalloc(sizeof(*cache), GFP_KERNEL);
 		if (!info->cache)
 			return -ENOMEM;
 	}

-- Steve

Re: [PATCH] unwind deferred: Annotate struct unwind_cache with __counted_by

Posted by David Laight 2 months, 3 weeks ago

On Fri, 14 Nov 2025 08:53:32 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Fri, 14 Nov 2025 08:43:46 -0500
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > I need to add a comment here that entries is not bound by nr_entries.
> > 
> >    https://lore.kernel.org/all/20250730093249.4833be14@gandalf.local.home/
> > 
> > Maybe this?:  
> 
> Or better yet, if this compiles (I haven't tried):
> 
> diff --git a/include/linux/unwind_deferred_types.h b/include/linux/unwind_deferred_types.h
> index 33b62ac25c86..253a69b21e76 100644
> --- a/include/linux/unwind_deferred_types.h
> +++ b/include/linux/unwind_deferred_types.h
> @@ -2,10 +2,14 @@
>  #ifndef _LINUX_UNWIND_USER_DEFERRED_TYPES_H
>  #define _LINUX_UNWIND_USER_DEFERRED_TYPES_H
>  
> +/* Make the cache fit in a 4K page */
> +#define UNWIND_MAX_ENTRIES					\
> +	((SZ_4K - offset_of(struct unwind_cache, entries)) / sizeof(long))
> +
>  struct unwind_cache {
>  	unsigned long		unwind_completed;
>  	unsigned int		nr_entries;
> -	unsigned long		entries[];
> +	unsigned long		entries[UNWIND_MAX_ENTRIES];

That won't compile - I tried it.

You could add __aligned(4096) to force the structure to be padded to 4k,
and then define UNWIND_MAX_ENTRIES in terms of the structure size.

	David

>  };
>  
>  /*
> diff --git a/kernel/unwind/deferred.c b/kernel/unwind/deferred.c
> index dc6040aae3ee..5dfd0ac264d1 100644
> --- a/kernel/unwind/deferred.c
> +++ b/kernel/unwind/deferred.c
> @@ -37,10 +37,6 @@ static inline bool try_assign_cnt(struct unwind_task_info *info, u32 cnt)
>  }
>  #endif
>  
> -/* Make the cache fit in a 4K page */
> -#define UNWIND_MAX_ENTRIES					\
> -	((SZ_4K - sizeof(struct unwind_cache)) / sizeof(long))
> -
>  /* Guards adding to or removing from the list of callbacks */
>  static DEFINE_MUTEX(callback_mutex);
>  static LIST_HEAD(callbacks);
> @@ -118,8 +114,7 @@ int unwind_user_faultable(struct unwind_stacktrace *trace)
>  		return -EINVAL;
>  
>  	if (!info->cache) {
> -		info->cache = kzalloc(struct_size(cache, entries, UNWIND_MAX_ENTRIES),
> -				      GFP_KERNEL);
> +		info->cache = kzalloc(sizeof(*cache), GFP_KERNEL);
>  		if (!info->cache)
>  			return -ENOMEM;
>  	}
> 
> -- Steve
>

Re: [PATCH] unwind deferred: Annotate struct unwind_cache with __counted_by

Posted by Steven Rostedt 2 months, 3 weeks ago

On Fri, 14 Nov 2025 15:02:09 +0000
David Laight <david.laight.linux@gmail.com> wrote:

> > +/* Make the cache fit in a 4K page */
> > +#define UNWIND_MAX_ENTRIES					\
> > +	((SZ_4K - offset_of(struct unwind_cache, entries)) / sizeof(long))
> > +
> >  struct unwind_cache {
> >  	unsigned long		unwind_completed;
> >  	unsigned int		nr_entries;
> > -	unsigned long		entries[];
> > +	unsigned long		entries[UNWIND_MAX_ENTRIES];  
> 
> That won't compile - I tried it.
> 
> You could add __aligned(4096) to force the structure to be padded to 4k,
> and then define UNWIND_MAX_ENTRIES in terms of the structure size.

I rather not do more tricks than what a comment would solve.

-- Steve

Re: [PATCH] unwind deferred: Annotate struct unwind_cache with __counted_by

Posted by Steven Rostedt 2 months, 3 weeks ago

On Fri, 14 Nov 2025 08:53:32 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> --- a/include/linux/unwind_deferred_types.h
> +++ b/include/linux/unwind_deferred_types.h
> @@ -2,10 +2,14 @@
>  #ifndef _LINUX_UNWIND_USER_DEFERRED_TYPES_H
>  #define _LINUX_UNWIND_USER_DEFERRED_TYPES_H
>  
> +/* Make the cache fit in a 4K page */
> +#define UNWIND_MAX_ENTRIES					\
> +	((SZ_4K - offset_of(struct unwind_cache, entries)) / sizeof(long))
> +
>  struct unwind_cache {
>  	unsigned long		unwind_completed;
>  	unsigned int		nr_entries;
> -	unsigned long		entries[];
> +	unsigned long		entries[UNWIND_MAX_ENTRIES];
>  };
>  

Nope, this fails with:

/work/git/test-linux.git/include/linux/stddef.h:16:33: error: invalid use of undefined type ‘struct unwind_cache’
   16 | #define offsetof(TYPE, MEMBER)  __builtin_offsetof(TYPE, MEMBER)
      |                                 ^~~~~~~~~~~~~~~~~~
/work/git/test-linux.git/include/linux/unwind_deferred_types.h:9:19: note: in expansion of macro ‘offsetof’
    9 |         ((SZ_4K - offsetof(struct unwind_cache, entries)) / sizeof(long))
      |                   ^~~~~~~~
/work/git/test-linux.git/include/linux/unwind_deferred_types.h:14:41: note: in expansion of macro ‘UNWIND_MAX_ENTRIES’
   14 |         unsigned long           entries[UNWIND_MAX_ENTRIES];
      |                                         ^~~~~~~~~~~~~~~~~~

-- Steve