netdev - Re: [PATCH net-next] selftests: add selftest for UDP SO_PEEK

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAL+tcoDy4XNBTZK3_MwzS1Js0NPg1e46xTtHmpJNckLywJ6NQQ@mail.gmail.com>
Date: Mon, 2 Sep 2024 23:33:47 +0800
From: Jason Xing <kerneljasonxing@...il.com>
To: Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org, 
	pabeni@...hat.com, shuah@...nel.org, jmaloy@...hat.com, 
	linux-kselftest@...r.kernel.org, netdev@...r.kernel.org, 
	Jason Xing <kernelxing@...cent.com>
Subject: Re: [PATCH net-next] selftests: add selftest for UDP SO_PEEK_OFF support

On Mon, Sep 2, 2024 at 11:02 PM Willem de Bruijn
<willemdebruijn.kernel@...il.com> wrote:
>
> Jason Xing wrote:
> > From: Jason Xing <kernelxing@...cent.com>
> >
> > Add the SO_PEEK_OFF selftest for UDP. In this patch, I mainly do
> > three things:
> > 1. rename tcp_so_peek_off.c
> > 2. adjust for UDP protocol
> > 3. add selftests into it
> >
> > Suggested-by: Jon Maloy <jmaloy@...hat.com>
> > Signed-off-by: Jason Xing <kernelxing@...cent.com>
>
> A few minor comments. Nothing important.
>
> Subject to Stan's point about .gitignore:
>
> Reviewed-by: Willem de Bruijn <willemb@...gle.com>

Thanks for your review!

>
> > -int tcp_peek_offset_probe(sa_family_t af)
> > +int sk_peek_offset_probe(sa_family_t af, int proto)
> >  {
> > +     int type = (proto == IPPROTO_TCP ? SOCK_STREAM : SOCK_DGRAM);
> >       int optv = 0;
> >       int ret = 0;
> >       int s;
> >
> > -     s = socket(af, SOCK_STREAM | SOCK_CLOEXEC, IPPROTO_TCP);
> > +     s = socket(af, type, proto);
>
> Removing the SOCK_CLOEXEC because not relevant to this single thread
> process, I suppose?

Yep. We don't need this one.

>
> Not important, but no need for proto, can just be 0.

You're right. I wonder if it is better if we explicitly pass the proto
here? I would like not to touch it here.

>
> >       if (s < 0) {
> >               ksft_perror("Temporary TCP socket creation failed");
> >       } else {
> >               if (!setsockopt(s, SOL_SOCKET, SO_PEEK_OFF, &optv, sizeof(int)))
> >                       ret = 1;
> >               else
> > -                     printf("%s does not support SO_PEEK_OFF\n", afstr(af));
> > +                     printf("%s does not support SO_PEEK_OFF\n", afstr(af, proto));
> >               close(s);
> >       }
> >       return ret;
> >  }
> >
> > -static void tcp_peek_offset_set(int s, int offset)
> > +static void sk_peek_offset_set(int s, int offset)
> >  {
> >       if (setsockopt(s, SOL_SOCKET, SO_PEEK_OFF, &offset, sizeof(offset)))
> >               ksft_perror("Failed to set SO_PEEK_OFF value\n");
> >  }
> >
> > -static int tcp_peek_offset_get(int s)
> > +static int sk_peek_offset_get(int s)
> >  {
> >       int offset;
> >       socklen_t len = sizeof(offset);
> > @@ -50,8 +54,9 @@ static int tcp_peek_offset_get(int s)
> >       return offset;
> >  }
> >
> > -static int tcp_peek_offset_test(sa_family_t af)
> > +static int sk_peek_offset_test(sa_family_t af, int proto)
> >  {
> > +     int type = (proto == IPPROTO_TCP ? SOCK_STREAM : SOCK_DGRAM);
> >       union {
> >               struct sockaddr sa;
> >               struct sockaddr_in a4;
> > @@ -62,13 +67,13 @@ static int tcp_peek_offset_test(sa_family_t af)
> >       int recv_sock = 0;
> >       int offset = 0;
> >       ssize_t len;
> > -     char buf;
> > +     char buf[2];
> >
> >       memset(&a, 0, sizeof(a));
> >       a.sa.sa_family = af;
> >
> > -     s[0] = socket(af, SOCK_STREAM, IPPROTO_TCP);
> > -     s[1] = socket(af, SOCK_STREAM | SOCK_NONBLOCK, IPPROTO_TCP);
> > +     s[0] = recv_sock = socket(af, type, proto);
> > +     s[1] = socket(af, type, proto);
>
> Same

I think we don't need this one, either.
As we can see, there are already some existing test files without the
SOCK_NONBLOCK flag.

>
> >
> >       if (s[0] < 0 || s[1] < 0) {
> >               ksft_perror("Temporary socket creation failed\n");
> > @@ -82,76 +87,78 @@ static int tcp_peek_offset_test(sa_family_t af)
> >               ksft_perror("Temporary socket getsockname() failed\n");
> >               goto out;
> >       }
> > -     if (listen(s[0], 0) < 0) {
> > +     if (proto == IPPROTO_TCP && listen(s[0], 0) < 0) {
> >               ksft_perror("Temporary socket listen() failed\n");
> >               goto out;
> >       }
> > -     if (connect(s[1], &a.sa, sizeof(a)) >= 0 || errno != EINPROGRESS) {
> > +     if (connect(s[1], &a.sa, sizeof(a))) {
> >               ksft_perror("Temporary socket connect() failed\n");
> >               goto out;
> >       }
>
> Changed due to the removal of SOCK_NONBLOCK above. Definitely
> simplifies the test.

Yep.

>
> Just note that error test is == -1 or < 0, also for consistency with
> the rest of the file.

I will add "< 0" here as you said.

Thanks,
Jason