Re: [net-next v2 3/3] selftests/net: Test PACKET_AUXDATA

[Willem de Bruijn]

Joe Damato wrote:
> On Sat, Apr 04, 2026 at 11:30:09PM -0400, Willem de Bruijn wrote:
> > Willem de Bruijn wrote:
> > > Joe Damato wrote:
> 
> [...]
> 
> > > > -static void do_rx(int fd, int expected_len, char *expected)
> > > > +static void check_aux_data(struct cmsghdr *cmsg, int expected_len)
> > > >  {
> > > > +	struct tpacket_auxdata *adata;
> > > > +
> > > > +	if (!cmsg)
> > > > +		error(1, 0, "auxdata null");
> > > > +
> > > > +	if (cmsg->cmsg_level != SOL_PACKET)
> > > > +		error(1, 0, "cmsg_level != SOL_PACKET");
> > > > +
> > > > +	if (cmsg->cmsg_type != PACKET_AUXDATA)
> > > > +		error(1, 0, "cmsg_type != PACKET_AUXDATA");
> > > > +
> > > > +	adata = (struct tpacket_auxdata *)CMSG_DATA(cmsg);
> > > 
> > > Sashiko had another interesting observation that this access may be
> > > unaligned, as cmsg_buf[1024] has 1-byte alignment.
> > > 
> > > That is not new in this patch. Indeed most tests in this dir just
> > > deference msg_control as struct cmsghdr * and CMSG_DATA as whatever
> > > domain specific type.
> > > 
> > > The man page also warns about this and suggests using memcpy to
> > > access CMSG_DATA. Not sure why it does not warn about the other
> > > cmsg_.. fields.
> > 
> > The commit that introduced that has more context
> > 
> > https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git/commit/man/man3/cmsg.3?id=36d25246b4333513fefdbec7f78f29d193cf5d9a
> > 
> > It points out 32-bit platforms where cmsghdr is 12 bytes.
> > 
> > At least one example given, ptpd, uses a union to ensure alignment
> > 
> > 	union {
> > 		struct cmsghdr cm;
> > 		char	control[256];
> > 	}     cmsg_un;
> > 
> > But at least one other example, ssmping, does not. So I think not even
> > the 4B (on 32-bit archs) of cmsghdr fields can be depended on.
> 
> I'm fine with adding an "__attribute__((aligned(8)))" to be safe, but FWIW, I
> think the key point from the commit message linked above is:
>  
>   shows access to int payload via *(int *)CMSG_DATA(cmsg) (of course
>   int is safe because its alignment is <= header alignment, but this
>   is not mentioned).

But that conditional on having a union as showed to make sure that
the structure has the struct cmsghdr alignment.

A bare char[] won't have that.

Anyway, in practice on the platforms we test this hasn't been an issue.
Fine to copy the example from other tests, leaving out the alignment.

I will follow-up with one patch to update all of them at once, once
I'm certain that I did not overlook some other mechanism that
implicitly does guarantee this alignment on all platforms.
 
> struct tpacket_auxdata only has u32 and u16, so 4 byte alignment seems fine
> and I don't think the issue applies in this particular case. But, as I said
> above, I am fine with adding the attribute to be defensive.