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.
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);
> + }
> +
