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

Date:   Tue, 17 Jan 2023 15:47:09 -0800
From:   Joe Perches <joe@...ches.com>
To:     Willem de Bruijn <willemdebruijn.kernel@...il.com>,
        Jacob Keller <jacob.e.keller@...el.com>
Cc:     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, 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