Re: [PATCH net-next v4] selftests: net: add test for ipv6 fragmentation

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

Brett A C Sheffield wrote:
> Add selftest for the IPv6 fragmentation regression which affected
> several stable kernels.
> 
> Commit a18dfa9925b9 ("ipv6: save dontfrag in cork") was backported to
> stable without some prerequisite commits.  This caused a regression when
> sending IPv6 UDP packets by preventing fragmentation and instead
> returning -1 (EMSGSIZE).
> 
> Add selftest to check for this issue by attempting to send a packet
> larger than the interface MTU. The packet will be fragmented on a
> working kernel, with sendmsg(2) correctly returning the expected number
> of bytes sent.  When the regression is present, sendmsg returns -1 and
> sets errno to EMSGSIZE.
> 
> Link: https://lore.kernel.org/stable/aElivdUXqd1OqgMY@karahi.gladserv.com
> Signed-off-by: Brett A C Sheffield <bacs@librecast.net>

Reviewed-by: Willem de Bruijn <willemb@google.com>

> ---
> v4 changes:
>  - fix "else should follow close brace" (checkpatch ERROR)

Reminder to send only only iteration to netdev per 24 hrs.
 
> v3 changes:
>  - add usleep instead of busy polling on sendmsg
>  - simplify error handling by using error() and leaving cleanup to O/S
>  - use loopback interface - don't bother creating TAP
>  - send to localhost (::1)
 

> +/* no need to wait for DAD in our namespace */
> +static int disable_dad(char *ifname)
> +{
> +	char sysvar[] = "/proc/sys/net/ipv6/conf/%s/accept_dad";
> +	char fname[IFNAMSIZ + sizeof(sysvar)];
> +	int fd;
> +
> +	snprintf(fname, sizeof(fname), sysvar, ifname);
> +	fd = open(fname, O_WRONLY);
> +	if (fd == -1)
> +		error(KSFT_FAIL, errno, "open accept_dad");
> +	if (write(fd, "0", 1) != 1)
> +		error(KSFT_FAIL, errno, "write accept_dad");
> +
> +	return close(fd);
> +}

Is this needed with loopback?

> +int main(void)
> +{
> +	struct in6_addr addr = {
> +		.s6_addr[15] = 0x01, /* ::1 */
> +	};
> +	struct sockaddr_in6 sa = {
> +		.sin6_family = AF_INET6,
> +		.sin6_addr = addr,
> +		.sin6_port = 9      /* port 9/udp (DISCARD) */

htons

> +	};
> +	char buf[LARGER_THAN_MTU] = {0};

That's a large stack allocation. static char?

> +	ns_fd = setup();
> +	s = socket(AF_INET6, SOCK_DGRAM, 0);
> +send_again:
> +	rc = sendmsg(s, &msg, 0);
> +	if (rc == -1) {
> +		/* if interface wasn't ready, try again */
> +		if (errno == EADDRNOTAVAIL) {
> +			usleep(1000);
> +			goto send_again;
> +		}
> +		printf("[FAIL] sendmsg: %s\n", strerror(errno));
> +	} else if (rc != LARGER_THAN_MTU) {
> +		printf("[FAIL] sendmsg() returned %zi, expected %i\n", rc, LARGER_THAN_MTU);
> +	} else {
> +		printf("[PASS] sendmsg() returned %zi\n", rc);
> +		err = KSFT_PASS;

slight preference to just fail with error() in the two error cases and
let the expected common path be linear and succeed:

    if (rc == -1) {
            if (..)
                    goto send_again;
            error(KSFT_FAIL, ..);
    }
    if (rc != LARGER_THAN_MTU) {
            error(KSFT_FAIL, ..);
    }

    printf(..)
    return KSFT_PASS;

> +	}
> +	close(s);
> +	close(ns_fd);

ns_fd is always -1. Check all system calls return values. Now that
setup internally can fail with error() no need for return values at
all.

> +	return err;
> +}
> 
> base-commit: 864ecc4a6dade82d3f70eab43dad0e277aa6fc78
> -- 
> 2.49.1
>