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

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

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