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

[Willem de Bruijn]

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