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 <[email protected]> > > --- > > 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); > > + } > > +
