[PATCH] selftests/user_events: Avoid taking address of packed member in perf_test

[PATCH] selftests/user_events: Avoid taking address of packed member in perf_test

[Ankit Khushwaha]

Accessing 'reg.write_index' directly triggers a -Waddress-of-packed-member
warning due to potential unaligned pointer access:

perf_test.c:239:38: warning: taking address of packed member 'write_index'
of class or structure 'user_reg' may result in an unaligned pointer value 
[-Waddress-of-packed-member]
  239 |         ASSERT_NE(-1, write(self->data_fd, &reg.write_index,
      |                                             ^~~~~~~~~~~~~~~ 

Use memcpy() instead to safely copy the value and avoid unaligned pointer
access across architectures.

Signed-off-by: Ankit Khushwaha <ankitkhushwaha.linux@gmail.com>
---
 tools/testing/selftests/user_events/perf_test.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/user_events/perf_test.c b/tools/testing/selftests/user_events/perf_test.c
index 201459d8094d..e4385f4aa231 100644
--- a/tools/testing/selftests/user_events/perf_test.c
+++ b/tools/testing/selftests/user_events/perf_test.c
@@ -201,6 +201,7 @@ TEST_F(user, perf_empty_events) {
 	struct perf_event_mmap_page *perf_page;
 	int page_size = sysconf(_SC_PAGESIZE);
 	int id, fd;
+	__u32 write_index;
 	__u32 *val;
 
 	reg.size = sizeof(reg);
@@ -236,7 +237,8 @@ TEST_F(user, perf_empty_events) {
 	ASSERT_EQ(1 << reg.enable_bit, self->check);
 
 	/* Ensure write shows up at correct offset */
-	ASSERT_NE(-1, write(self->data_fd, &reg.write_index,
+	memcpy(&write_index, &reg.write_index, sizeof(reg.write_index));
+	ASSERT_NE(-1, write(self->data_fd, &write_index,
 	                    sizeof(reg.write_index)));
 	val = (void *)(((char *)perf_page) + perf_page->data_offset);
 	ASSERT_EQ(PERF_RECORD_SAMPLE, *val);
-- 
2.51.0

Re: [PATCH] selftests/user_events: Avoid taking address of packed member in perf_test

[Andrew Morton]

On Mon, 27 Oct 2025 17:04:39 +0530 Ankit Khushwaha <ankitkhushwaha.linux@gmail.com> wrote:

> Accessing 'reg.write_index' directly triggers a -Waddress-of-packed-member
> warning due to potential unaligned pointer access:
> 
> perf_test.c:239:38: warning: taking address of packed member 'write_index'
> of class or structure 'user_reg' may result in an unaligned pointer value 
> [-Waddress-of-packed-member]
>   239 |         ASSERT_NE(-1, write(self->data_fd, &reg.write_index,
>       |                                             ^~~~~~~~~~~~~~~ 
> 
> Use memcpy() instead to safely copy the value and avoid unaligned pointer
> access across architectures.
> 
> ...
>
> --- a/tools/testing/selftests/user_events/perf_test.c
> +++ b/tools/testing/selftests/user_events/perf_test.c
> @@ -201,6 +201,7 @@ TEST_F(user, perf_empty_events) {
>  	struct perf_event_mmap_page *perf_page;
>  	int page_size = sysconf(_SC_PAGESIZE);
>  	int id, fd;
> +	__u32 write_index;
>  	__u32 *val;
>  
>  	reg.size = sizeof(reg);
> @@ -236,7 +237,8 @@ TEST_F(user, perf_empty_events) {
>  	ASSERT_EQ(1 << reg.enable_bit, self->check);
>  
>  	/* Ensure write shows up at correct offset */
> -	ASSERT_NE(-1, write(self->data_fd, &reg.write_index,
> +	memcpy(&write_index, &reg.write_index, sizeof(reg.write_index));
> +	ASSERT_NE(-1, write(self->data_fd, &write_index,
>  	                    sizeof(reg.write_index)));

Simply casting &write_index to void* would fix this?

>  	val = (void *)(((char *)perf_page) + perf_page->data_offset);
>  	ASSERT_EQ(PERF_RECORD_SAMPLE, *val);

Re: [PATCH] selftests/user_events: Avoid taking address of packed member in perf_test

[Ankit Khushwaha]

On Mon, Oct 27, 2025 at 04:25:21PM -0700, Andrew Morton wrote:
> On Mon, 27 Oct 2025 17:04:39 +0530 Ankit Khushwaha <ankitkhushwaha.linux@gmail.com> wrote:
> 
> > Accessing 'reg.write_index' directly triggers a -Waddress-of-packed-member
> > warning due to potential unaligned pointer access:
> > 
> > perf_test.c:239:38: warning: taking address of packed member 'write_index'
> > of class or structure 'user_reg' may result in an unaligned pointer value 
> > [-Waddress-of-packed-member]
> >   239 |         ASSERT_NE(-1, write(self->data_fd, &reg.write_index,
> >       |                                             ^~~~~~~~~~~~~~~ 
> > 
> > Use memcpy() instead to safely copy the value and avoid unaligned pointer
> > access across architectures.
> > 
> > ...
> >
> > --- a/tools/testing/selftests/user_events/perf_test.c
> > +++ b/tools/testing/selftests/user_events/perf_test.c
> > @@ -201,6 +201,7 @@ TEST_F(user, perf_empty_events) {
> >  	struct perf_event_mmap_page *perf_page;
> >  	int page_size = sysconf(_SC_PAGESIZE);
> >  	int id, fd;
> > +	__u32 write_index;
> >  	__u32 *val;
> >  
> >  	reg.size = sizeof(reg);
> > @@ -236,7 +237,8 @@ TEST_F(user, perf_empty_events) {
> >  	ASSERT_EQ(1 << reg.enable_bit, self->check);
> >  
> >  	/* Ensure write shows up at correct offset */
> > -	ASSERT_NE(-1, write(self->data_fd, &reg.write_index,
> > +	memcpy(&write_index, &reg.write_index, sizeof(reg.write_index));
> > +	ASSERT_NE(-1, write(self->data_fd, &write_index,
> >  	                    sizeof(reg.write_index)));
> 
> Simply casting &write_index to void* would fix this?

yes, this hides the type mismatch from the compiler. But i think
casting to void * will not fix the alignment mismatch for packed struct.
It works on x86, but might break on other platform.

> 
> >  	val = (void *)(((char *)perf_page) + perf_page->data_offset);
> >  	ASSERT_EQ(PERF_RECORD_SAMPLE, *val);

Thanks
Ankit

Re: [PATCH] selftests/user_events: Avoid taking address of packed member in perf_test

[Andrew Morton]

On Tue, 28 Oct 2025 22:28:10 +0530 Ankit Khushwaha <ankitkhushwaha.linux@gmail.com> wrote:

> > > @@ -236,7 +237,8 @@ TEST_F(user, perf_empty_events) {
> > >  	ASSERT_EQ(1 << reg.enable_bit, self->check);
> > >  
> > >  	/* Ensure write shows up at correct offset */
> > > -	ASSERT_NE(-1, write(self->data_fd, &reg.write_index,
> > > +	memcpy(&write_index, &reg.write_index, sizeof(reg.write_index));
> > > +	ASSERT_NE(-1, write(self->data_fd, &write_index,
> > >  	                    sizeof(reg.write_index)));
> > 
> > Simply casting &write_index to void* would fix this?
> 
> yes, this hides the type mismatch from the compiler. But i think
> casting to void * will not fix the alignment mismatch for packed struct.
> It works on x86, but might break on other platform.

It's the second argument to write(2)!  write(2) expects a const char *,
but void* will work.

Re: [PATCH] selftests/user_events: Avoid taking address of packed member in perf_test

[Ankit Khushwaha]

On Tue, Oct 28, 2025 at 01:26:05PM -0700, Andrew Morton wrote:
> On Tue, 28 Oct 2025 22:28:10 +0530 Ankit Khushwaha <ankitkhushwaha.linux@gmail.com> wrote:
> 
> > > > @@ -236,7 +237,8 @@ TEST_F(user, perf_empty_events) {
> > > >  	ASSERT_EQ(1 << reg.enable_bit, self->check);
> > > >  
> > > >  	/* Ensure write shows up at correct offset */
> > > > -	ASSERT_NE(-1, write(self->data_fd, &reg.write_index,
> > > > +	memcpy(&write_index, &reg.write_index, sizeof(reg.write_index));
> > > > +	ASSERT_NE(-1, write(self->data_fd, &write_index,
> > > >  	                    sizeof(reg.write_index)));
> > > 
> > > Simply casting &write_index to void* would fix this?
> > 
> > yes, this hides the type mismatch from the compiler. But i think
> > casting to void * will not fix the alignment mismatch for packed struct.
> > It works on x86, but might break on other platform.
> 
> It's the second argument to write(2)!  write(2) expects a const char *,
> but void* will work.

Hi Andrew,
Indeed 
`ASSERT_NE(-1, write(self->data_fd, (void *)&reg.write_index, 
		     sizeof(reg.write_index)));`

would work. However since `reg` is packed struct, directly taking the 
address of its member  `&reg.write_index` may lead to unaligned access 
on some architectures. as indicated by the compiler warning

	perf_test.c:239:38: warning: taking address of packed member
	'write_index' of class or structure 'user_reg' may result in 
	an unaligned pointer value [-Waddress-of-packed-member]
	239 |         ASSERT_NE(-1, write(self->data_fd, &reg.write_index,
      	    |						  ^~~~~~~~~~~~~~~

Using `memcpy` avoids this by performing a byte-wise copy, which is safe 
to use for packed structures.

Thanks
-- Ankit

Re: [PATCH] selftests/user_events: Avoid taking address of packed member in perf_test

[Andrew Morton]

On Wed, 29 Oct 2025 20:50:38 +0530 Ankit Khushwaha <ankitkhushwaha.linux@gmail.com> wrote:

> > > > >  	/* Ensure write shows up at correct offset */
> > > > > -	ASSERT_NE(-1, write(self->data_fd, &reg.write_index,
> > > > > +	memcpy(&write_index, &reg.write_index, sizeof(reg.write_index));
> > > > > +	ASSERT_NE(-1, write(self->data_fd, &write_index,
> > > > >  	                    sizeof(reg.write_index)));
> > > > 
> > > > Simply casting &write_index to void* would fix this?
> > > 
> > > yes, this hides the type mismatch from the compiler. But i think
> > > casting to void * will not fix the alignment mismatch for packed struct.
> > > It works on x86, but might break on other platform.
> > 
> > It's the second argument to write(2)!  write(2) expects a const char *,
> > but void* will work.
> 
> Hi Andrew,
> Indeed 
> `ASSERT_NE(-1, write(self->data_fd, (void *)&reg.write_index, 
> 		     sizeof(reg.write_index)));`
> 
> would work. However since `reg` is packed struct, directly taking the 
> address of its member  `&reg.write_index` may lead to unaligned access 
> on some architectures. as indicated by the compiler warning
> 
> 	perf_test.c:239:38: warning: taking address of packed member
> 	'write_index' of class or structure 'user_reg' may result in 
> 	an unaligned pointer value [-Waddress-of-packed-member]

Well sure, we might get an unaligned pointer value and it would be an
error to dereference that pointer.

But we don't dereference it!  We pass that pointer to write(2), which
is happy with any alignment.

The warning is accurate.  It "may" indeed "result in an unaligned
pointer value".  But there is nothing at all wrong with this code.  OK,
let's find some way to suppress the warning (preferably without adding
a pointless memcpy) and let's make the changelog and code comments be
clear about what's going on here.