* [PATCH net-next v2] pktgen: Introducing 'SHARED' flag for testing with non-shared skb
@ 2023-09-13 5:10 Liang Chen
2023-09-13 17:02 ` Benjamin Poirier
0 siblings, 1 reply; 3+ messages in thread
From: Liang Chen @ 2023-09-13 5:10 UTC (permalink / raw)
To: davem, edumazet, kuba, pabeni, benjamin.poirier; +Cc: netdev, liangchen.linux
Currently, skbs generated by pktgen always have their reference count
incremented before transmission, causing their reference count to be
always greater than 1, leading to two issues:
1. Only the code paths for shared skbs can be tested.
2. In certain situations, skbs can only be released by pktgen.
To enhance testing comprehensiveness, we are introducing the "SHARED"
flag to indicate whether an SKB is shared. This flag is enabled by
default, aligning with the current behavior. However, disabling this
flag allows skbs with a reference count of 1 to be transmitted.
So we can test non-shared skbs and code paths where skbs are released
within the stack.
Signed-off-by: Liang Chen <liangchen.linux@gmail.com>
---
Documentation/networking/pktgen.rst | 13 ++++++++
net/core/pktgen.c | 50 +++++++++++++++++++++++++----
2 files changed, 56 insertions(+), 7 deletions(-)
diff --git a/Documentation/networking/pktgen.rst b/Documentation/networking/pktgen.rst
index 1225f0f63ff0..ffd976c0cbf0 100644
--- a/Documentation/networking/pktgen.rst
+++ b/Documentation/networking/pktgen.rst
@@ -178,6 +178,7 @@ Examples::
IPSEC # IPsec encapsulation (needs CONFIG_XFRM)
NODE_ALLOC # node specific memory allocation
NO_TIMESTAMP # disable timestamping
+ SHARED # enable shared SKB
pgset 'flag ![name]' Clear a flag to determine behaviour.
Note that you might need to use single quote in
interactive mode, so that your shell wouldn't expand
@@ -288,6 +289,17 @@ To avoid breaking existing testbed scripts for using AH type and tunnel mode,
you can use "pgset spi SPI_VALUE" to specify which transformation mode
to employ.
+Disable shared SKB
+==================
+By default, SKBs sent by pktgen are shared (user count > 1).
+If you need to test with non-shared SKBs, you can remove the "SHARED" flag
+by simply setting::
+
+ pg_set "flag !SHARED"
+
+However, if the "clone_skb," "burst," or "count" parameters are configured,
+the skb still needs to be held by pktgen for further access. Hence the skb
+must be shared.
Current commands and configuration options
==========================================
@@ -357,6 +369,7 @@ Current commands and configuration options
IPSEC
NODE_ALLOC
NO_TIMESTAMP
+ SHARED
spi (ipsec)
diff --git a/net/core/pktgen.c b/net/core/pktgen.c
index f56b8d697014..3cf00090cf09 100644
--- a/net/core/pktgen.c
+++ b/net/core/pktgen.c
@@ -200,6 +200,7 @@
pf(VID_RND) /* Random VLAN ID */ \
pf(SVID_RND) /* Random SVLAN ID */ \
pf(NODE) /* Node memory alloc*/ \
+ pf(SHARED) /* Shared SKB */ \
#define pf(flag) flag##_SHIFT,
enum pkt_flags {
@@ -1198,7 +1199,8 @@ static ssize_t pktgen_if_write(struct file *file,
((pkt_dev->xmit_mode == M_NETIF_RECEIVE) ||
!(pkt_dev->odev->priv_flags & IFF_TX_SKB_SHARING)))
return -ENOTSUPP;
- if (value > 0 && pkt_dev->n_imix_entries > 0)
+ if (value > 0 && (pkt_dev->n_imix_entries > 0 ||
+ !(pkt_dev->flags & F_SHARED)))
return -EINVAL;
i += len;
@@ -1212,6 +1214,9 @@ static ssize_t pktgen_if_write(struct file *file,
if (len < 0)
return len;
+ if ((value > 0) && !(pkt_dev->flags & F_SHARED))
+ return -EINVAL;
+
i += len;
pkt_dev->count = value;
sprintf(pg_result, "OK: count=%llu",
@@ -1257,6 +1262,10 @@ static ssize_t pktgen_if_write(struct file *file,
((pkt_dev->xmit_mode == M_START_XMIT) &&
(!(pkt_dev->odev->priv_flags & IFF_TX_SKB_SHARING)))))
return -ENOTSUPP;
+
+ if ((value > 1) && !(pkt_dev->flags & F_SHARED))
+ return -EINVAL;
+
pkt_dev->burst = value < 1 ? 1 : value;
sprintf(pg_result, "OK: burst=%u", pkt_dev->burst);
return count;
@@ -1334,10 +1343,18 @@ static ssize_t pktgen_if_write(struct file *file,
flag = pktgen_read_flag(f, &disable);
if (flag) {
- if (disable)
+ if (disable) {
+ /* If "clone_skb", "burst", or "count" parameters are
+ * configured, it means that the skb still needs to be
+ * referenced by the pktgen, so the skb must be shared.
+ */
+ if (flag == F_SHARED && (pkt_dev->clone_skb ||
+ pkt_dev->burst > 1 || pkt_dev->count))
+ return -EINVAL;
pkt_dev->flags &= ~flag;
- else
+ } else {
pkt_dev->flags |= flag;
+ }
} else {
sprintf(pg_result,
"Flag -:%s:- unknown\nAvailable flags, (prepend ! to un-set flag):\n%s",
@@ -1350,7 +1367,8 @@ static ssize_t pktgen_if_write(struct file *file,
#ifdef CONFIG_XFRM
"IPSEC, "
#endif
- "NODE_ALLOC\n");
+ "NODE_ALLOC, "
+ "SHARED\n");
return count;
}
sprintf(pg_result, "OK: flags=0x%x", pkt_dev->flags);
@@ -3483,7 +3501,8 @@ static void pktgen_xmit(struct pktgen_dev *pkt_dev)
if (pkt_dev->xmit_mode == M_NETIF_RECEIVE) {
skb = pkt_dev->skb;
skb->protocol = eth_type_trans(skb, skb->dev);
- refcount_add(burst, &skb->users);
+ if (pkt_dev->flags & F_SHARED)
+ refcount_add(burst, &skb->users);
local_bh_disable();
do {
ret = netif_receive_skb(skb);
@@ -3491,6 +3510,10 @@ static void pktgen_xmit(struct pktgen_dev *pkt_dev)
pkt_dev->errors++;
pkt_dev->sofar++;
pkt_dev->seq_num++;
+ if (unlikely(!(pkt_dev->flags & F_SHARED))) {
+ pkt_dev->skb = NULL;
+ break;
+ }
if (refcount_read(&skb->users) != burst) {
/* skb was queued by rps/rfs or taps,
* so cannot reuse this skb
@@ -3509,7 +3532,8 @@ static void pktgen_xmit(struct pktgen_dev *pkt_dev)
goto out; /* Skips xmit_mode M_START_XMIT */
} else if (pkt_dev->xmit_mode == M_QUEUE_XMIT) {
local_bh_disable();
- refcount_inc(&pkt_dev->skb->users);
+ if (pkt_dev->flags & F_SHARED)
+ refcount_inc(&pkt_dev->skb->users);
ret = dev_queue_xmit(pkt_dev->skb);
switch (ret) {
@@ -3517,8 +3541,13 @@ static void pktgen_xmit(struct pktgen_dev *pkt_dev)
pkt_dev->sofar++;
pkt_dev->seq_num++;
pkt_dev->tx_bytes += pkt_dev->last_pkt_size;
+ if (!(pkt_dev->flags & F_SHARED))
+ pkt_dev->skb = NULL;
break;
case NET_XMIT_DROP:
+ if (!(pkt_dev->flags & F_SHARED))
+ pkt_dev->skb = NULL;
+ fallthrough;
case NET_XMIT_CN:
/* These are all valid return codes for a qdisc but
* indicate packets are being dropped or will likely
@@ -3549,7 +3578,8 @@ static void pktgen_xmit(struct pktgen_dev *pkt_dev)
pkt_dev->last_ok = 0;
goto unlock;
}
- refcount_add(burst, &pkt_dev->skb->users);
+ if (pkt_dev->flags & F_SHARED)
+ refcount_add(burst, &pkt_dev->skb->users);
xmit_more:
ret = netdev_start_xmit(pkt_dev->skb, odev, txq, --burst > 0);
@@ -3560,10 +3590,15 @@ static void pktgen_xmit(struct pktgen_dev *pkt_dev)
pkt_dev->sofar++;
pkt_dev->seq_num++;
pkt_dev->tx_bytes += pkt_dev->last_pkt_size;
+ if (!(pkt_dev->flags & F_SHARED))
+ pkt_dev->skb = NULL;
if (burst > 0 && !netif_xmit_frozen_or_drv_stopped(txq))
goto xmit_more;
break;
case NET_XMIT_DROP:
+ if (!(pkt_dev->flags & F_SHARED))
+ pkt_dev->skb = NULL;
+ fallthrough;
case NET_XMIT_CN:
/* skb has been consumed */
pkt_dev->errors++;
@@ -3771,6 +3806,7 @@ static int pktgen_add_device(struct pktgen_thread *t, const char *ifname)
pkt_dev->svlan_id = 0xffff;
pkt_dev->burst = 1;
pkt_dev->node = NUMA_NO_NODE;
+ pkt_dev->flags = F_SHARED; /* SKB shared by default */
err = pktgen_setup_dev(t->net, pkt_dev, ifname);
if (err)
--
2.40.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH net-next v2] pktgen: Introducing 'SHARED' flag for testing with non-shared skb
2023-09-13 5:10 [PATCH net-next v2] pktgen: Introducing 'SHARED' flag for testing with non-shared skb Liang Chen
@ 2023-09-13 17:02 ` Benjamin Poirier
2023-09-14 3:32 ` Liang Chen
0 siblings, 1 reply; 3+ messages in thread
From: Benjamin Poirier @ 2023-09-13 17:02 UTC (permalink / raw)
To: Liang Chen; +Cc: davem, edumazet, kuba, pabeni, netdev
On 2023-09-13 13:10 +0800, Liang Chen wrote:
> Currently, skbs generated by pktgen always have their reference count
> incremented before transmission, causing their reference count to be
> always greater than 1, leading to two issues:
> 1. Only the code paths for shared skbs can be tested.
> 2. In certain situations, skbs can only be released by pktgen.
> To enhance testing comprehensiveness, we are introducing the "SHARED"
> flag to indicate whether an SKB is shared. This flag is enabled by
> default, aligning with the current behavior. However, disabling this
> flag allows skbs with a reference count of 1 to be transmitted.
> So we can test non-shared skbs and code paths where skbs are released
> within the stack.
>
> Signed-off-by: Liang Chen <liangchen.linux@gmail.com>
> ---
> Documentation/networking/pktgen.rst | 13 ++++++++
> net/core/pktgen.c | 50 +++++++++++++++++++++++++----
> 2 files changed, 56 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/networking/pktgen.rst b/Documentation/networking/pktgen.rst
> index 1225f0f63ff0..ffd976c0cbf0 100644
> --- a/Documentation/networking/pktgen.rst
> +++ b/Documentation/networking/pktgen.rst
> @@ -178,6 +178,7 @@ Examples::
> IPSEC # IPsec encapsulation (needs CONFIG_XFRM)
> NODE_ALLOC # node specific memory allocation
> NO_TIMESTAMP # disable timestamping
> + SHARED # enable shared SKB
> pgset 'flag ![name]' Clear a flag to determine behaviour.
> Note that you might need to use single quote in
> interactive mode, so that your shell wouldn't expand
> @@ -288,6 +289,17 @@ To avoid breaking existing testbed scripts for using AH type and tunnel mode,
> you can use "pgset spi SPI_VALUE" to specify which transformation mode
> to employ.
>
> +Disable shared SKB
> +==================
> +By default, SKBs sent by pktgen are shared (user count > 1).
> +If you need to test with non-shared SKBs, you can remove the "SHARED" flag
> +by simply setting::
> +
> + pg_set "flag !SHARED"
> +
> +However, if the "clone_skb," "burst," or "count" parameters are configured,
> +the skb still needs to be held by pktgen for further access. Hence the skb
> +must be shared.
>
> Current commands and configuration options
> ==========================================
> @@ -357,6 +369,7 @@ Current commands and configuration options
> IPSEC
> NODE_ALLOC
> NO_TIMESTAMP
> + SHARED
>
> spi (ipsec)
>
> diff --git a/net/core/pktgen.c b/net/core/pktgen.c
> index f56b8d697014..3cf00090cf09 100644
> --- a/net/core/pktgen.c
> +++ b/net/core/pktgen.c
> @@ -200,6 +200,7 @@
> pf(VID_RND) /* Random VLAN ID */ \
> pf(SVID_RND) /* Random SVLAN ID */ \
> pf(NODE) /* Node memory alloc*/ \
> + pf(SHARED) /* Shared SKB */ \
>
> #define pf(flag) flag##_SHIFT,
> enum pkt_flags {
> @@ -1198,7 +1199,8 @@ static ssize_t pktgen_if_write(struct file *file,
> ((pkt_dev->xmit_mode == M_NETIF_RECEIVE) ||
> !(pkt_dev->odev->priv_flags & IFF_TX_SKB_SHARING)))
> return -ENOTSUPP;
> - if (value > 0 && pkt_dev->n_imix_entries > 0)
> + if (value > 0 && (pkt_dev->n_imix_entries > 0 ||
> + !(pkt_dev->flags & F_SHARED)))
> return -EINVAL;
I see that imix uses EINVAL but I would suggest to add the new check to
the previous condition which returns -ENOTSUPP. A value like "clone_skb
1" is not invalid by itself.
>
> i += len;
> @@ -1212,6 +1214,9 @@ static ssize_t pktgen_if_write(struct file *file,
> if (len < 0)
> return len;
>
> + if ((value > 0) && !(pkt_dev->flags & F_SHARED))
> + return -EINVAL;
> +
I think the restriction on count == 0 can be dropped by adding the
following changes, do you agree?
diff --git a/net/core/pktgen.c b/net/core/pktgen.c
index 3cf00090cf09..6fe19783b060 100644
--- a/net/core/pktgen.c
+++ b/net/core/pktgen.c
@@ -1214,9 +1214,6 @@ static ssize_t pktgen_if_write(struct file *file,
if (len < 0)
return len;
- if ((value > 0) && !(pkt_dev->flags & F_SHARED))
- return -EINVAL;
-
i += len;
pkt_dev->count = value;
sprintf(pg_result, "OK: count=%llu",
@@ -1344,12 +1341,12 @@ static ssize_t pktgen_if_write(struct file *file,
if (flag) {
if (disable) {
- /* If "clone_skb", "burst", or "count" parameters are
+ /* If "clone_skb" or "burst" parameters are
* configured, it means that the skb still needs to be
* referenced by the pktgen, so the skb must be shared.
*/
if (flag == F_SHARED && (pkt_dev->clone_skb ||
- pkt_dev->burst > 1 || pkt_dev->count))
+ pkt_dev->burst > 1))
return -EINVAL;
pkt_dev->flags &= ~flag;
} else {
@@ -3623,7 +3620,8 @@ static void pktgen_xmit(struct pktgen_dev *pkt_dev)
/* If pkt_dev->count is zero, then run forever */
if ((pkt_dev->count != 0) && (pkt_dev->sofar >= pkt_dev->count)) {
- pktgen_wait_for_skb(pkt_dev);
+ if (pkt_dev->skb)
+ pktgen_wait_for_skb(pkt_dev);
/* Done with this */
pktgen_stop_device(pkt_dev);
> i += len;
> pkt_dev->count = value;
> sprintf(pg_result, "OK: count=%llu",
> @@ -1257,6 +1262,10 @@ static ssize_t pktgen_if_write(struct file *file,
> ((pkt_dev->xmit_mode == M_START_XMIT) &&
> (!(pkt_dev->odev->priv_flags & IFF_TX_SKB_SHARING)))))
> return -ENOTSUPP;
> +
> + if ((value > 1) && !(pkt_dev->flags & F_SHARED))
> + return -EINVAL;
> +
Please integrate the new check with the existing ones in the 'if' just
above.
> pkt_dev->burst = value < 1 ? 1 : value;
> sprintf(pg_result, "OK: burst=%u", pkt_dev->burst);
> return count;
> @@ -1334,10 +1343,18 @@ static ssize_t pktgen_if_write(struct file *file,
> flag = pktgen_read_flag(f, &disable);
>
> if (flag) {
> - if (disable)
> + if (disable) {
> + /* If "clone_skb", "burst", or "count" parameters are
> + * configured, it means that the skb still needs to be
> + * referenced by the pktgen, so the skb must be shared.
> + */
> + if (flag == F_SHARED && (pkt_dev->clone_skb ||
> + pkt_dev->burst > 1 || pkt_dev->count))
> + return -EINVAL;
netdev uses an 80 column limit
https://github.com/kuba-moo/nipa/blob/853db1f2a67758d839324920f7319b94630efd17/tests/patch/checkpatch/checkpatch.sh#L16
> pkt_dev->flags &= ~flag;
> - else
> + } else {
> pkt_dev->flags |= flag;
> + }
> } else {
> sprintf(pg_result,
> "Flag -:%s:- unknown\nAvailable flags, (prepend ! to un-set flag):\n%s",
> @@ -1350,7 +1367,8 @@ static ssize_t pktgen_if_write(struct file *file,
> #ifdef CONFIG_XFRM
> "IPSEC, "
> #endif
> - "NODE_ALLOC\n");
> + "NODE_ALLOC, "
> + "SHARED\n");
This triggers a checkpatch warning. How about cleaning that code up in a
preceding patch to use pkt_flag_names[] instead?
> return count;
> }
> sprintf(pg_result, "OK: flags=0x%x", pkt_dev->flags);
> @@ -3483,7 +3501,8 @@ static void pktgen_xmit(struct pktgen_dev *pkt_dev)
> if (pkt_dev->xmit_mode == M_NETIF_RECEIVE) {
> skb = pkt_dev->skb;
> skb->protocol = eth_type_trans(skb, skb->dev);
> - refcount_add(burst, &skb->users);
> + if (pkt_dev->flags & F_SHARED)
> + refcount_add(burst, &skb->users);
> local_bh_disable();
> do {
> ret = netif_receive_skb(skb);
> @@ -3491,6 +3510,10 @@ static void pktgen_xmit(struct pktgen_dev *pkt_dev)
> pkt_dev->errors++;
> pkt_dev->sofar++;
> pkt_dev->seq_num++;
> + if (unlikely(!(pkt_dev->flags & F_SHARED))) {
> + pkt_dev->skb = NULL;
> + break;
> + }
> if (refcount_read(&skb->users) != burst) {
> /* skb was queued by rps/rfs or taps,
> * so cannot reuse this skb
> @@ -3509,7 +3532,8 @@ static void pktgen_xmit(struct pktgen_dev *pkt_dev)
> goto out; /* Skips xmit_mode M_START_XMIT */
> } else if (pkt_dev->xmit_mode == M_QUEUE_XMIT) {
> local_bh_disable();
> - refcount_inc(&pkt_dev->skb->users);
> + if (pkt_dev->flags & F_SHARED)
> + refcount_inc(&pkt_dev->skb->users);
>
> ret = dev_queue_xmit(pkt_dev->skb);
> switch (ret) {
> @@ -3517,8 +3541,13 @@ static void pktgen_xmit(struct pktgen_dev *pkt_dev)
> pkt_dev->sofar++;
> pkt_dev->seq_num++;
> pkt_dev->tx_bytes += pkt_dev->last_pkt_size;
> + if (!(pkt_dev->flags & F_SHARED))
> + pkt_dev->skb = NULL;
> break;
> case NET_XMIT_DROP:
> + if (!(pkt_dev->flags & F_SHARED))
> + pkt_dev->skb = NULL;
> + fallthrough;
> case NET_XMIT_CN:
> /* These are all valid return codes for a qdisc but
> * indicate packets are being dropped or will likely
This patch introduces almost the same use after free problem as v1.
Please take the time to look at the existing code to understand the
cases when a skb refcount is dropped by the stack.
This time the problem can be triggered with:
ip link add dummy0 up type dummy
tc qdisc add dev dummy0 root pfifo_head_drop limit 0
And then run pktgen on dummy0 with "flag !SHARED" and "xmit_mode
queue_xmit"
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH net-next v2] pktgen: Introducing 'SHARED' flag for testing with non-shared skb
2023-09-13 17:02 ` Benjamin Poirier
@ 2023-09-14 3:32 ` Liang Chen
0 siblings, 0 replies; 3+ messages in thread
From: Liang Chen @ 2023-09-14 3:32 UTC (permalink / raw)
To: Benjamin Poirier; +Cc: davem, edumazet, kuba, pabeni, netdev
On Thu, Sep 14, 2023 at 1:02 AM Benjamin Poirier
<benjamin.poirier@gmail.com> wrote:
>
> On 2023-09-13 13:10 +0800, Liang Chen wrote:
> > Currently, skbs generated by pktgen always have their reference count
> > incremented before transmission, causing their reference count to be
> > always greater than 1, leading to two issues:
> > 1. Only the code paths for shared skbs can be tested.
> > 2. In certain situations, skbs can only be released by pktgen.
> > To enhance testing comprehensiveness, we are introducing the "SHARED"
> > flag to indicate whether an SKB is shared. This flag is enabled by
> > default, aligning with the current behavior. However, disabling this
> > flag allows skbs with a reference count of 1 to be transmitted.
> > So we can test non-shared skbs and code paths where skbs are released
> > within the stack.
> >
> > Signed-off-by: Liang Chen <liangchen.linux@gmail.com>
> > ---
> > Documentation/networking/pktgen.rst | 13 ++++++++
> > net/core/pktgen.c | 50 +++++++++++++++++++++++++----
> > 2 files changed, 56 insertions(+), 7 deletions(-)
> >
> > diff --git a/Documentation/networking/pktgen.rst b/Documentation/networking/pktgen.rst
> > index 1225f0f63ff0..ffd976c0cbf0 100644
> > --- a/Documentation/networking/pktgen.rst
> > +++ b/Documentation/networking/pktgen.rst
> > @@ -178,6 +178,7 @@ Examples::
> > IPSEC # IPsec encapsulation (needs CONFIG_XFRM)
> > NODE_ALLOC # node specific memory allocation
> > NO_TIMESTAMP # disable timestamping
> > + SHARED # enable shared SKB
> > pgset 'flag ![name]' Clear a flag to determine behaviour.
> > Note that you might need to use single quote in
> > interactive mode, so that your shell wouldn't expand
> > @@ -288,6 +289,17 @@ To avoid breaking existing testbed scripts for using AH type and tunnel mode,
> > you can use "pgset spi SPI_VALUE" to specify which transformation mode
> > to employ.
> >
> > +Disable shared SKB
> > +==================
> > +By default, SKBs sent by pktgen are shared (user count > 1).
> > +If you need to test with non-shared SKBs, you can remove the "SHARED" flag
> > +by simply setting::
> > +
> > + pg_set "flag !SHARED"
> > +
> > +However, if the "clone_skb," "burst," or "count" parameters are configured,
> > +the skb still needs to be held by pktgen for further access. Hence the skb
> > +must be shared.
> >
> > Current commands and configuration options
> > ==========================================
> > @@ -357,6 +369,7 @@ Current commands and configuration options
> > IPSEC
> > NODE_ALLOC
> > NO_TIMESTAMP
> > + SHARED
> >
> > spi (ipsec)
> >
> > diff --git a/net/core/pktgen.c b/net/core/pktgen.c
> > index f56b8d697014..3cf00090cf09 100644
> > --- a/net/core/pktgen.c
> > +++ b/net/core/pktgen.c
> > @@ -200,6 +200,7 @@
> > pf(VID_RND) /* Random VLAN ID */ \
> > pf(SVID_RND) /* Random SVLAN ID */ \
> > pf(NODE) /* Node memory alloc*/ \
> > + pf(SHARED) /* Shared SKB */ \
> >
> > #define pf(flag) flag##_SHIFT,
> > enum pkt_flags {
> > @@ -1198,7 +1199,8 @@ static ssize_t pktgen_if_write(struct file *file,
> > ((pkt_dev->xmit_mode == M_NETIF_RECEIVE) ||
> > !(pkt_dev->odev->priv_flags & IFF_TX_SKB_SHARING)))
> > return -ENOTSUPP;
> > - if (value > 0 && pkt_dev->n_imix_entries > 0)
> > + if (value > 0 && (pkt_dev->n_imix_entries > 0 ||
> > + !(pkt_dev->flags & F_SHARED)))
> > return -EINVAL;
>
> I see that imix uses EINVAL but I would suggest to add the new check to
> the previous condition which returns -ENOTSUPP. A value like "clone_skb
> 1" is not invalid by itself.
>
Returning 'ENOTSUPP' seems to imply that "such a option is not
supported by the NIC", while we are trying to convey to users "such a
combination is invalid'. So it is a bit confusing as to which one to
return.
> >
> > i += len;
> > @@ -1212,6 +1214,9 @@ static ssize_t pktgen_if_write(struct file *file,
> > if (len < 0)
> > return len;
> >
> > + if ((value > 0) && !(pkt_dev->flags & F_SHARED))
> > + return -EINVAL;
> > +
>
> I think the restriction on count == 0 can be dropped by adding the
> following changes, do you agree?
>
Yes, I agree. At least, this doesn't change the existing behavior
without using the 'SHARED' flag. Only in that case, the code doesn't
wait for the last packet to be transmitted.
> diff --git a/net/core/pktgen.c b/net/core/pktgen.c
> index 3cf00090cf09..6fe19783b060 100644
> --- a/net/core/pktgen.c
> +++ b/net/core/pktgen.c
> @@ -1214,9 +1214,6 @@ static ssize_t pktgen_if_write(struct file *file,
> if (len < 0)
> return len;
>
> - if ((value > 0) && !(pkt_dev->flags & F_SHARED))
> - return -EINVAL;
> -
> i += len;
> pkt_dev->count = value;
> sprintf(pg_result, "OK: count=%llu",
> @@ -1344,12 +1341,12 @@ static ssize_t pktgen_if_write(struct file *file,
>
> if (flag) {
> if (disable) {
> - /* If "clone_skb", "burst", or "count" parameters are
> + /* If "clone_skb" or "burst" parameters are
> * configured, it means that the skb still needs to be
> * referenced by the pktgen, so the skb must be shared.
> */
> if (flag == F_SHARED && (pkt_dev->clone_skb ||
> - pkt_dev->burst > 1 || pkt_dev->count))
> + pkt_dev->burst > 1))
> return -EINVAL;
> pkt_dev->flags &= ~flag;
> } else {
> @@ -3623,7 +3620,8 @@ static void pktgen_xmit(struct pktgen_dev *pkt_dev)
>
> /* If pkt_dev->count is zero, then run forever */
> if ((pkt_dev->count != 0) && (pkt_dev->sofar >= pkt_dev->count)) {
> - pktgen_wait_for_skb(pkt_dev);
> + if (pkt_dev->skb)
> + pktgen_wait_for_skb(pkt_dev);
>
> /* Done with this */
> pktgen_stop_device(pkt_dev);
>
>
> > i += len;
> > pkt_dev->count = value;
> > sprintf(pg_result, "OK: count=%llu",
> > @@ -1257,6 +1262,10 @@ static ssize_t pktgen_if_write(struct file *file,
> > ((pkt_dev->xmit_mode == M_START_XMIT) &&
> > (!(pkt_dev->odev->priv_flags & IFF_TX_SKB_SHARING)))))
> > return -ENOTSUPP;
> > +
> > + if ((value > 1) && !(pkt_dev->flags & F_SHARED))
> > + return -EINVAL;
> > +
>
> Please integrate the new check with the existing ones in the 'if' just
> above.
>
> > pkt_dev->burst = value < 1 ? 1 : value;
> > sprintf(pg_result, "OK: burst=%u", pkt_dev->burst);
> > return count;
> > @@ -1334,10 +1343,18 @@ static ssize_t pktgen_if_write(struct file *file,
> > flag = pktgen_read_flag(f, &disable);
> >
> > if (flag) {
> > - if (disable)
> > + if (disable) {
> > + /* If "clone_skb", "burst", or "count" parameters are
> > + * configured, it means that the skb still needs to be
> > + * referenced by the pktgen, so the skb must be shared.
> > + */
> > + if (flag == F_SHARED && (pkt_dev->clone_skb ||
> > + pkt_dev->burst > 1 || pkt_dev->count))
> > + return -EINVAL;
>
> netdev uses an 80 column limit
> https://github.com/kuba-moo/nipa/blob/853db1f2a67758d839324920f7319b94630efd17/tests/patch/checkpatch/checkpatch.sh#L16
>
Sure.
> > pkt_dev->flags &= ~flag;
> > - else
> > + } else {
> > pkt_dev->flags |= flag;
> > + }
> > } else {
> > sprintf(pg_result,
> > "Flag -:%s:- unknown\nAvailable flags, (prepend ! to un-set flag):\n%s",
> > @@ -1350,7 +1367,8 @@ static ssize_t pktgen_if_write(struct file *file,
> > #ifdef CONFIG_XFRM
> > "IPSEC, "
> > #endif
> > - "NODE_ALLOC\n");
> > + "NODE_ALLOC, "
> > + "SHARED\n");
>
> This triggers a checkpatch warning. How about cleaning that code up in a
> preceding patch to use pkt_flag_names[] instead?
>
Sure.
> > return count;
> > }
> > sprintf(pg_result, "OK: flags=0x%x", pkt_dev->flags);
> > @@ -3483,7 +3501,8 @@ static void pktgen_xmit(struct pktgen_dev *pkt_dev)
> > if (pkt_dev->xmit_mode == M_NETIF_RECEIVE) {
> > skb = pkt_dev->skb;
> > skb->protocol = eth_type_trans(skb, skb->dev);
> > - refcount_add(burst, &skb->users);
> > + if (pkt_dev->flags & F_SHARED)
> > + refcount_add(burst, &skb->users);
> > local_bh_disable();
> > do {
> > ret = netif_receive_skb(skb);
> > @@ -3491,6 +3510,10 @@ static void pktgen_xmit(struct pktgen_dev *pkt_dev)
> > pkt_dev->errors++;
> > pkt_dev->sofar++;
> > pkt_dev->seq_num++;
> > + if (unlikely(!(pkt_dev->flags & F_SHARED))) {
> > + pkt_dev->skb = NULL;
> > + break;
> > + }
> > if (refcount_read(&skb->users) != burst) {
> > /* skb was queued by rps/rfs or taps,
> > * so cannot reuse this skb
> > @@ -3509,7 +3532,8 @@ static void pktgen_xmit(struct pktgen_dev *pkt_dev)
> > goto out; /* Skips xmit_mode M_START_XMIT */
> > } else if (pkt_dev->xmit_mode == M_QUEUE_XMIT) {
> > local_bh_disable();
> > - refcount_inc(&pkt_dev->skb->users);
> > + if (pkt_dev->flags & F_SHARED)
> > + refcount_inc(&pkt_dev->skb->users);
> >
> > ret = dev_queue_xmit(pkt_dev->skb);
> > switch (ret) {
> > @@ -3517,8 +3541,13 @@ static void pktgen_xmit(struct pktgen_dev *pkt_dev)
> > pkt_dev->sofar++;
> > pkt_dev->seq_num++;
> > pkt_dev->tx_bytes += pkt_dev->last_pkt_size;
> > + if (!(pkt_dev->flags & F_SHARED))
> > + pkt_dev->skb = NULL;
> > break;
> > case NET_XMIT_DROP:
> > + if (!(pkt_dev->flags & F_SHARED))
> > + pkt_dev->skb = NULL;
> > + fallthrough;
> > case NET_XMIT_CN:
> > /* These are all valid return codes for a qdisc but
> > * indicate packets are being dropped or will likely
>
> This patch introduces almost the same use after free problem as v1.
> Please take the time to look at the existing code to understand the
> cases when a skb refcount is dropped by the stack.
>
> This time the problem can be triggered with:
> ip link add dummy0 up type dummy
> tc qdisc add dev dummy0 root pfifo_head_drop limit 0
> And then run pktgen on dummy0 with "flag !SHARED" and "xmit_mode
> queue_xmit"
Thank you for pointing it out. We will take a closer look into this problem.
Thanks,
Liang
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-09-14 3:33 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-09-13 5:10 [PATCH net-next v2] pktgen: Introducing 'SHARED' flag for testing with non-shared skb Liang Chen
2023-09-13 17:02 ` Benjamin Poirier
2023-09-14 3:32 ` Liang Chen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).