netdev - Re: [PATCH net] selftests/net: toeplitz: fix race on tpacket

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] [day] [month] [year] [list]

Message-ID: <CAF=yD-JaJUYEEGp7H41FeEr8U-cLKx6Ki5-8VchHRVmj6av2QQ@mail.gmail.com>
Date:   Wed, 18 Jan 2023 10:26:12 -0500
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Joe Perches <joe@...ches.com>
Cc:     Jacob Keller <jacob.e.keller@...el.com>, netdev@...r.kernel.org,
        davem@...emloft.net, kuba@...nel.org, edumazet@...gle.com,
        pabeni@...hat.com, Willem de Bruijn <willemb@...gle.com>,
        Andy Whitcroft <apw@...onical.com>
Subject: Re: [PATCH net] selftests/net: toeplitz: fix race on tpacket_v3 block close

On Tue, Jan 17, 2023 at 7:54 PM Joe Perches <joe@...ches.com> wrote:
>
> On Tue, 2023-01-17 at 14:15 -0500, Willem de Bruijn wrote:
> > > On 1/16/2023 9:40 AM, Willem de Bruijn wrote:
> > > > From: Willem de Bruijn <willemb@...gle.com>
> > > >
> > > > Avoid race between process wakeup and tpacket_v3 block timeout.
> > > >
> > > > The test waits for cfg_timeout_msec for packets to arrive. Packets
> > > > arrive in tpacket_v3 rings, which pass packets ("frames") to the
> > > > process in batches ("blocks"). The sk waits for
> > > > req3.tp_retire_blk_tov
> > > > msec to release a block.
> > > >
> > > > Set the block timeout lower than the process waiting time, else
> > > > the process may find that no block has been released by the time
> > > > it
> > > > scans the socket list. Convert to a ring of more than one,
> > > > smaller,
> > > > blocks with shorter timeouts. Blocks must be page aligned, so >=
> > > > 64KB.
> > > >
> > > > Somewhat awkward while () notation dictated by checkpatch: no
> > > > empty
> > > > braces allowed, nor statement on the same line as the condition.
> > > >
> > > > Fixes: 5ebfb4cc3048 ("selftests/net: toeplitz test")
> > > > Signed-off-by: Willem de Bruijn <willemb@...gle.com>
> > > > ---
> > > >  tools/testing/selftests/net/toeplitz.c | 13 ++++++++-----
> > > >  1 file changed, 8 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/tools/testing/selftests/net/toeplitz.c
> > > > b/tools/testing/selftests/net/toeplitz.c
> > > > index 90026a27eac0c..66f7f6568643a 100644
> > > > --- a/tools/testing/selftests/net/toeplitz.c
> > > > +++ b/tools/testing/selftests/net/toeplitz.c
> > > > @@ -215,7 +215,7 @@ static char *recv_frame(const struct
> > > > ring_state *ring, char *frame)
> > > >  }
> > > >
> > > >  /* A single TPACKET_V3 block can hold multiple frames */
> > > > -static void recv_block(struct ring_state *ring)
> > > > +static bool recv_block(struct ring_state *ring)
> > > >  {
> > > >       struct tpacket_block_desc *block;
> > > >       char *frame;
> > > > @@ -223,7 +223,7 @@ static void recv_block(struct ring_state
> > > > *ring)
> > > >
> > > >       block = (void *)(ring->mmap + ring->idx * ring_block_sz);
> > > >       if (!(block->hdr.bh1.block_status & TP_STATUS_USER))
> > > > -             return;
> > > > +             return false;
> > > >
> > > >       frame = (char *)block;
> > > >       frame += block->hdr.bh1.offset_to_first_pkt;
> > > > @@ -235,6 +235,8 @@ static void recv_block(struct ring_state
> > > > *ring)
> > > >
> > > >       block->hdr.bh1.block_status = TP_STATUS_KERNEL;
> > > >       ring->idx = (ring->idx + 1) % ring_block_nr;
> > > > +
> > > > +     return true;
> > > >  }
> > > >
> > > >  /* simple test: sleep once unconditionally and then process all
> > > > rings */
> > > > @@ -245,7 +247,8 @@ static void process_rings(void)
> > > >       usleep(1000 * cfg_timeout_msec);
> > > >
> > > >       for (i = 0; i < num_cpus; i++)
> > > > -             recv_block(&rings[i]);
> > > > +             while (recv_block(&rings[i]))
> > > > +                     ;
> > >
> > > I'd rather have one of
> > >
> > >   while (recv_block(&rings[i]));
> > >
> > > or
> > >
> > >   while (recv_block(&rings[i])) {}
> > >
> > > or even (but less preferred:
> > >
> > >   do {} (while (recv_block(&rings[i]));
> > >
> > > instead of  this ; on its own line.
> > >
> > > Even if this violates checkpatch attempts to catch other bad style
> > > this
> > > is preferable to the lone ';' on its own line.
> > >
> > > If necessary we can/should change checkpatch to allow the idiomatic
> > > approach.
>
> To me it's a 'Don't care'.
>
> There are many hundreds of these in the kernel and there is no
> valid reason to require a particular style.
>
> $ git grep -P  '^\t+;\s*$' -- '*.c' | wc -l
> 871

Interesting, thanks.

I had to send a v2 by now anyway, so changed to a "do {} while" as
evidently people found that more palatable.