[PATCH net] net: pktgen: use a consistent flow count

[PATCH net] net: pktgen: use a consistent flow count

[Qi Zhang]

From: Chengfeng Ye <nicoyip.dev@gmail.com>

pktgen_if_write() can update cflows while the packet generator thread is
inside mod_cur_headers(). The latter first tests cflows, but f_pick() then
reloads it when selecting a random flow.

This allows the following interleaving:

  CPU 0 (kpktgend)                 CPU 1 (proc write)
  if (pkt_dev->cflows) // 10
                                   pkt_dev->cflows = 0
  get_random_u32_below(pkt_dev->cflows)

get_random_u32_below(0) returns a full-width random value. Using that
value as an index into the fixed-size flows array causes an out-of-bounds
access. The kernel reported:

  BUG: unable to handle page fault for address: ffffc8fe2d2674bc
  #PF: supervisor read access in kernel mode
  Oops: Oops: 0000 [#1] SMP KASAN NOPTI
  CPU: 0 UID: 0 PID: 65 Comm: kpktgend_0
  RIP: 0010:mod_cur_headers+0x16f8/0x2840
  Call Trace:
   <TASK>
   pktgen_thread_worker+0x305a/0x6bc0
   kthread+0x2c6/0x3b0
   ret_from_fork+0x36e/0x5a0
   ret_from_fork_asm+0x1a/0x30
   </TASK>

Read cflows once at the start of mod_cur_headers(), pass the snapshot to
f_pick(), and use it for later flow-state decisions in the same packet.
Publish proc updates with WRITE_ONCE(). Flow selection then always uses a
nonzero count bounded by MAX_CFLOWS, while a concurrent update takes
effect on a later packet.

Fixes: 007a531b0a0c ("[PKTGEN]: Introduce sequential flows")
Cc: stable@vger.kernel.org
Signed-off-by: Chengfeng Ye <nicoyip.dev@gmail.com>
Signed-off-by: Qi Zhang <marsy12010123@gmail.com>
---
 net/core/pktgen.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/net/core/pktgen.c b/net/core/pktgen.c
index ee64f3012321..631cb1f55f6d 100644
--- a/net/core/pktgen.c
+++ b/net/core/pktgen.c
@@ -1632,7 +1632,7 @@ static ssize_t pktgen_if_write(struct file *file,
 		if (value > MAX_CFLOWS)
 			value = MAX_CFLOWS;
 
-		pkt_dev->cflows = value;
+		WRITE_ONCE(pkt_dev->cflows, value);
 		sprintf(pg_result, "OK: flows=%u", pkt_dev->cflows);
 		return count;
 	}
@@ -2373,7 +2373,7 @@ static inline int f_seen(const struct pktgen_dev *pkt_dev, int flow)
 	return !!(pkt_dev->flows[flow].flags & F_INIT);
 }
 
-static inline int f_pick(struct pktgen_dev *pkt_dev)
+static inline int f_pick(struct pktgen_dev *pkt_dev, unsigned int cflows)
 {
 	int flow = pkt_dev->curfl;
 
@@ -2383,11 +2383,11 @@ static inline int f_pick(struct pktgen_dev *pkt_dev)
 			pkt_dev->flows[flow].count = 0;
 			pkt_dev->flows[flow].flags = 0;
 			pkt_dev->curfl += 1;
-			if (pkt_dev->curfl >= pkt_dev->cflows)
+			if (pkt_dev->curfl >= cflows)
 				pkt_dev->curfl = 0; /*reset */
 		}
 	} else {
-		flow = get_random_u32_below(pkt_dev->cflows);
+		flow = get_random_u32_below(cflows);
 		pkt_dev->curfl = flow;
 
 		if (pkt_dev->flows[flow].count > pkt_dev->lflow) {
@@ -2461,12 +2461,14 @@ static void set_cur_queue_map(struct pktgen_dev *pkt_dev)
  */
 static void mod_cur_headers(struct pktgen_dev *pkt_dev)
 {
+	unsigned int cflows;
 	__u32 imn;
 	__u32 imx;
 	int flow = 0;
 
-	if (pkt_dev->cflows)
-		flow = f_pick(pkt_dev);
+	cflows = READ_ONCE(pkt_dev->cflows);
+	if (cflows)
+		flow = f_pick(pkt_dev, cflows);
 
 	/*  Deal with source MAC */
 	if (pkt_dev->src_mac_count > 1) {
@@ -2582,7 +2584,7 @@ static void mod_cur_headers(struct pktgen_dev *pkt_dev)
 			pkt_dev->cur_saddr = htonl(t);
 		}
 
-		if (pkt_dev->cflows && f_seen(pkt_dev, flow)) {
+		if (cflows && f_seen(pkt_dev, flow)) {
 			pkt_dev->cur_daddr = pkt_dev->flows[flow].cur_daddr;
 		} else {
 			imn = ntohl(pkt_dev->daddr_min);
@@ -2611,7 +2613,7 @@ static void mod_cur_headers(struct pktgen_dev *pkt_dev)
 					pkt_dev->cur_daddr = htonl(t);
 				}
 			}
-			if (pkt_dev->cflows) {
+			if (cflows) {
 				pkt_dev->flows[flow].flags |= F_INIT;
 				pkt_dev->flows[flow].cur_daddr =
 				    pkt_dev->cur_daddr;
-- 
2.43.0

Re: [PATCH net] net: pktgen: use a consistent flow count

[Paolo Abeni]

On 8/2/26 5:23 PM, Qi Zhang wrote:
> From: Chengfeng Ye <nicoyip.dev@gmail.com>
> 
> pktgen_if_write() can update cflows while the packet generator thread is
> inside mod_cur_headers(). The latter first tests cflows, but f_pick() then
> reloads it when selecting a random flow.
> 
> This allows the following interleaving:
> 
>   CPU 0 (kpktgend)                 CPU 1 (proc write)
>   if (pkt_dev->cflows) // 10
>                                    pkt_dev->cflows = 0
>   get_random_u32_below(pkt_dev->cflows)
> 
> get_random_u32_below(0) returns a full-width random value. Using that
> value as an index into the fixed-size flows array causes an out-of-bounds
> access. The kernel reported:
> 
>   BUG: unable to handle page fault for address: ffffc8fe2d2674bc
>   #PF: supervisor read access in kernel mode
>   Oops: Oops: 0000 [#1] SMP KASAN NOPTI
>   CPU: 0 UID: 0 PID: 65 Comm: kpktgend_0
>   RIP: 0010:mod_cur_headers+0x16f8/0x2840
>   Call Trace:
>    <TASK>
>    pktgen_thread_worker+0x305a/0x6bc0
>    kthread+0x2c6/0x3b0
>    ret_from_fork+0x36e/0x5a0
>    ret_from_fork_asm+0x1a/0x30
>    </TASK>
> 
> Read cflows once at the start of mod_cur_headers(), pass the snapshot to
> f_pick(), and use it for later flow-state decisions in the same packet.
> Publish proc updates with WRITE_ONCE(). Flow selection then always uses a
> nonzero count bounded by MAX_CFLOWS, while a concurrent update takes
> effect on a later packet.
> 
> Fixes: 007a531b0a0c ("[PKTGEN]: Introduce sequential flows")
> Cc: stable@vger.kernel.org
> Signed-off-by: Chengfeng Ye <nicoyip.dev@gmail.com>
> Signed-off-by: Qi Zhang <marsy12010123@gmail.com>
> ---
>  net/core/pktgen.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/net/core/pktgen.c b/net/core/pktgen.c
> index ee64f3012321..631cb1f55f6d 100644
> --- a/net/core/pktgen.c
> +++ b/net/core/pktgen.c
> @@ -1632,7 +1632,7 @@ static ssize_t pktgen_if_write(struct file *file,
>  		if (value > MAX_CFLOWS)
>  			value = MAX_CFLOWS;
>  
> -		pkt_dev->cflows = value;
> +		WRITE_ONCE(pkt_dev->cflows, value);
>  		sprintf(pg_result, "OK: flows=%u", pkt_dev->cflows);

Please use:
		sprintf(pg_result, "OK: flows=%u", value);

so that the user get consistent results across racing writes.

Also you need to add READ_ONCE() annotation to all `->cflows` accesses -
a few missed ones in pktgen_if_show().

/P