Thanks for the review, Willem.
On 2025-08-25 12:16, Willem de Bruijn wrote:
> 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.
> >
> > Signed-off-by: Brett A C Sheffield <b...@librecast.net>
> > Link: https://lore.kernel.org/stable/aelivduxqd1oq...@karahi.gladserv.com
>
> Thanks for adding a regression test for this.
No problem. I wrote a test for myself when bisecting the problem back in June -
makes sense to convert it to a selftest.
> > +/* we need to set MTU, so do this in a namespace to play nicely */
> > +static int create_namespace(void)
> > +{
> > + const char *netns_path = "/proc/self/ns/net";
> > + int fd;
> > +
> > + if (unshare(CLONE_NEWNET) != 0) {
> > + perror("unshare");
> > + return -1;
> > + }
>
> Is this not sufficient to move the current process in its own netns?
Yes. Yes it is. Apparently I did not read the man page properly.
> > + fd = open(netns_path, O_RDONLY);
> > + if (fd == -1) {
> > + perror("open");
> > + return -1;
> > + }
> > +
> > + if (setns(fd, CLONE_NEWNET)) {
> > + perror("setns");
> > + return -1;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int setup(void)
> > +{
> > + struct ifreq ifr = {0};
> > + char ifname[IFNAMSIZ];
> > + int fd = -1;
> > + int ctl;
> > +
> > + if (create_namespace() == -1)
> > + return -1;
> > +
> > + ctl = socket(AF_LOCAL, SOCK_STREAM, 0);
> > + if (ctl == -1)
> > + return -1;
> > +
> > + memset(ifname, 0, sizeof(ifname));
> > + fd = create_interface(ctl, ifname, &ifr);
> > + if (fd == -1)
> > + goto err_close_ctl;
> > + if (disable_dad(ifname) == -1)
> > + goto err_close_fd;
> > + if (interface_up(ctl, ifname, &ifr) == -1)
> > + goto err_close_fd;
> > + if (set_mtu(ctl, ifname, &ifr) == -1)
> > + goto err_close_fd;
> > + usleep(10000); /* give interface a moment to wake up */
>
> This may be racy. Wait on a more explicit signal? E.g.,
> /sys/class/net/$DEV/operstate.
Good thinking. I'll try that.
> > + struct msghdr msg = {
> > + .msg_iov = &iov,
> > + .msg_iovlen = 1,
> > + .msg_name = (struct sockaddr *)&sa,
> > + .msg_namelen = sizeof(sa),
> > + };
> > + ssize_t rc;
> > + int ns_fd;
> > + int s;
> > +
> > + printf("Testing IPv6 fragmentation\n");
> > + ns_fd = setup();
> > + if (ns_fd == -1)
> > + return 1;
> > + s = socket(AF_INET6, SOCK_DGRAM, 0);
> > + msg.msg_name = (struct sockaddr *)&sa;
> > + msg.msg_namelen = sizeof(sa);
>
> nit: duplicate?
Well spotted. Will fix.
> Also, no local address is set. This uses the IPv6 auto assigned
> address?
Correct. The test sends to a link-local scope multicast group from the autoconf
link-local address. I'll clarify that in the comments at the top of the test.
> > + rc = sendmsg(s, &msg, 0);
> > + if (rc == -1) {
> > + perror("send");
> > + return 1;
>
> Probably want to cleanup state both on success and failure.
Ack.
> Could use KSFT_.. exit codes, though 0/1 works just as well for
> kselftests in practice.
Ok.
> > + } else if (rc != LARGER_THAN_MTU) {
> > + fprintf(stderr, "send() returned %zi\n", rc);
> > + return 1;
> > + }
> > + close(s);
> > + close(ns_fd);
> > +
> > + return 0;
> > +}
> > --
> > 2.49.1
> >
Thanks again - expect a v2 when I have that cleaned up and re-tested.
Cheers,
Brett
--
