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]

Message-ID: <CAF=yD-LMO5Y1Uith1jsbh1kOO3t4oagTnKSdKoM=gQkfd61oAA@mail.gmail.com>
Date:   Tue, 17 Jan 2023 14:15:58 -0500
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     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>,
        Joe Perches <joe@...ches.com>,
        Andy Whitcroft <apw@...onical.com>
Subject: Re: [PATCH net] selftests/net: toeplitz: fix race on tpacket_v3 block close

> 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.

Let me send a v2 with the do {} while construct.

Cc:ed the checkpatch maintainers for visibility.