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: <5f494ec3-9435-b9dc-6dd8-9e1b7354430d@intel.com>
Date:   Tue, 17 Jan 2023 11:03:21 -0800
From:   Jacob Keller <jacob.e.keller@...el.com>
To:     Willem de Bruijn <willemdebruijn.kernel@...il.com>,
        <netdev@...r.kernel.org>
CC:     <davem@...emloft.net>, <kuba@...nel.org>, <edumazet@...gle.com>,
        <pabeni@...hat.com>, Willem de Bruijn <willemb@...gle.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.