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

Willem de Bruijn
2026-04-05T03:30:11Z

Willem de Bruijn wrote:
> Joe Damato wrote:
> > Extend the packet socket selftest, adding a recvmsg path, to test
> > PACKET_AUXDATA. Check basic attributes of tpacket_auxdata.
> >
> > Signed-off-by: Joe Damato <joe@dama.to>
> > ---
> > tools/testing/selftests/net/psock_snd.c | 67 ++++++++++++++++++++++--
> > tools/testing/selftests/net/psock_snd.sh | 5 ++
> > 2 files changed, 67 insertions(+), 5 deletions(-)
> >
> > v2:
> > - Add is_psock bool argument to do_rx.
> > - Factor out aux data check into its own function for readability.
> >
> > diff --git a/tools/testing/selftests/net/psock_snd.c b/tools/testing/selftests/net/psock_snd.c
> > index 81096df5cffc..5464317c1764 100644
> > --- a/tools/testing/selftests/net/psock_snd.c
> > +++ b/tools/testing/selftests/net/psock_snd.c
> > @@ -40,6 +40,7 @@ static bool cfg_use_qdisc_bypass;
> > static bool cfg_use_vlan;
> > static bool cfg_use_vnet;
> > static bool cfg_drop;
> > +static bool cfg_aux_data;
> >
> > static char *cfg_ifname = "lo";
> > static int cfg_mtu = 1500;
> > @@ -279,11 +280,54 @@ static int setup_rx(void)
> > return fd;
> > }
> >
> > -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.

> Indeed I can trigger this, e.g., with ipv6_flowlabel.c with
>
> - char control[CMSG_SPACE(sizeof(flowlabel))] = {0};
> + char control[1 + CMSG_SPACE(sizeof(flowlabel))] = {0};
>
> - cm = (void *)control;
> + cm = (void *)control + 1;
>
> and compiling with -fsanitize=alignment. That triggers warnings for
> all fields, starting from cmsg_len on line 78.
>
> In practice this does not cause issues, because the compiler appears
> to align char[] to 16B, even though __alignof__(control) shows 1. This
> seems true for x86_64, but I am not aware that it is true across all
> archs, especially those that cannot handle unaligned access.
>
> I think the x86_64 source is the AMD64 ABI Draft, e.g., v 0.99.6
>
> An array uses the same alignment as its elements, except that
> a local or global array variable of length at least 16 bytes
> or a C99 variable-length array variable always has alignment
> of at least 16 bytes(4)
>
> (4) The alignment requirement allows the use of SSE instructions
> when operating on the array. [..]
>
> Makes sense as sizeof struct cmsghdr == 16.
>
> cmsg_len has length 8 (size_t). We'll be hardpressed to find a
> CMSG_DATA example with a larger alignment requirement. Indeed I did
> not in this directory. So satisfying 8-byte alignment for msg_control
> will suffice for all tests in this directory.
>
> Unless we're certain that 8B alignment for stack aligned char[] is
> guaranteed across platforms, one safe approach it to add explicit
> alignment:
>
> - char control[CMSG_SPACE(sizeof(flowlabel))] = {0};
> + char control[CMSG_SPACE(sizeof(flowlabel))] __attribute__((aligned(8))) = {0};
>
> I can update the (other) tests. Unless someone knows that this is
> indeed not needed in practice on any platform.
>
> > +
> > + if (adata->tp_net != ETH_HLEN)
> > + error(1, 0, "cmsg tp_net != ETH_HLEN");
> > +
> > + if (adata->tp_len != expected_len)
> > + error(1, 0, "cmsg tp_len != %u", expected_len);
> > +
> > + if (adata->tp_snaplen != expected_len)
> > + error(1, 0, "cmsg tp_snaplen != %u", expected_len);
> > +}
> > +
> > +static void do_rx(int fd, int expected_len, char *expected, bool is_psock)
> > +{
> > + bool aux = is_psock && cfg_aux_data;
> > + char cmsg_buf[1024] = {};
> > + struct msghdr msg = {};
> > + struct iovec iov[1];
> > int ret;
> >
> > - ret = recv(fd, rbuf, sizeof(rbuf), 0);
> > + if (aux) {
> > + iov[0].iov_base = rbuf;
> > + iov[0].iov_len = sizeof(rbuf);
> > +
> > + msg.msg_iov = iov;
> > + msg.msg_iovlen = 1;
> > +
> > + msg.msg_control = cmsg_buf;
> > + msg.msg_controllen = sizeof(cmsg_buf);
> > +
> > + ret = recvmsg(fd, &msg, 0);
> > + } else {
> > + ret = recv(fd, rbuf, sizeof(rbuf), 0);
> > + }
> > +