* [RFC PATCH net-next] pktgen: Introducing a parameter for non-shared skb testing
@ 2023-09-06 10:35 Liang Chen
2023-09-06 22:32 ` Benjamin Poirier
0 siblings, 1 reply; 6+ messages in thread
From: Liang Chen @ 2023-09-06 10:35 UTC (permalink / raw)
To: davem, edumazet, kuba, pabeni; +Cc: netdev, liangchen.linux
Currently, skbs generated by pktgen always have their reference count
incremented before transmission, leading to two issues:
1. Only the code paths for shared skbs can be tested.
2. Skbs can only be released by pktgen.
To enhance testing comprehensiveness, introducing the "skb_single_user"
parameter, which 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 network stack.
Signed-off-by: Liang Chen <liangchen.linux@gmail.com>
---
net/core/pktgen.c | 39 ++++++++++++++++++++++++++++++++++++---
1 file changed, 36 insertions(+), 3 deletions(-)
diff --git a/net/core/pktgen.c b/net/core/pktgen.c
index f56b8d697014..8f48272b9d4b 100644
--- a/net/core/pktgen.c
+++ b/net/core/pktgen.c
@@ -423,6 +423,7 @@ struct pktgen_dev {
__u32 skb_priority; /* skb priority field */
unsigned int burst; /* number of duplicated packets to burst */
int node; /* Memory node */
+ int skb_single_user; /* allow single user skb for transmission */
#ifdef CONFIG_XFRM
__u8 ipsmode; /* IPSEC mode (config) */
@@ -1805,6 +1806,17 @@ static ssize_t pktgen_if_write(struct file *file,
return count;
}
+ if (!strcmp(name, "skb_single_user")) {
+ len = num_arg(&user_buffer[i], 1, &value);
+ if (len < 0)
+ return len;
+
+ i += len;
+ pkt_dev->skb_single_user = value;
+ sprintf(pg_result, "OK: skb_single_user=%u", pkt_dev->skb_single_user);
+ return count;
+ }
+
sprintf(pkt_dev->result, "No such parameter \"%s\"", name);
return -EINVAL;
}
@@ -3460,6 +3472,14 @@ static void pktgen_xmit(struct pktgen_dev *pkt_dev)
return;
}
+ /* If clone_skb, burst, or count parameters are configured,
+ * it implies the need for skb reuse, hence single user skb
+ * transmission is not allowed.
+ */
+ if (pkt_dev->skb_single_user && (pkt_dev->clone_skb ||
+ burst > 1 || pkt_dev->count))
+ pkt_dev->skb_single_user = 0;
+
/* If no skb or clone count exhausted then get new one */
if (!pkt_dev->skb || (pkt_dev->last_ok &&
++pkt_dev->clone_count >= pkt_dev->clone_skb)) {
@@ -3483,7 +3503,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->skb_single_user)
+ refcount_add(burst, &skb->users);
local_bh_disable();
do {
ret = netif_receive_skb(skb);
@@ -3491,6 +3512,12 @@ static void pktgen_xmit(struct pktgen_dev *pkt_dev)
pkt_dev->errors++;
pkt_dev->sofar++;
pkt_dev->seq_num++;
+
+ if (pkt_dev->skb_single_user) {
+ 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 +3536,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->skb_single_user)
+ refcount_inc(&pkt_dev->skb->users);
ret = dev_queue_xmit(pkt_dev->skb);
switch (ret) {
@@ -3517,6 +3545,8 @@ 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->skb_single_user)
+ pkt_dev->skb = NULL;
break;
case NET_XMIT_DROP:
case NET_XMIT_CN:
@@ -3549,7 +3579,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->skb_single_user)
+ refcount_add(burst, &pkt_dev->skb->users);
xmit_more:
ret = netdev_start_xmit(pkt_dev->skb, odev, txq, --burst > 0);
@@ -3560,6 +3591,8 @@ 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->skb_single_user)
+ pkt_dev->skb = NULL;
if (burst > 0 && !netif_xmit_frozen_or_drv_stopped(txq))
goto xmit_more;
break;
--
2.31.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RFC PATCH net-next] pktgen: Introducing a parameter for non-shared skb testing
2023-09-06 10:35 [RFC PATCH net-next] pktgen: Introducing a parameter for non-shared skb testing Liang Chen
@ 2023-09-06 22:32 ` Benjamin Poirier
2023-09-07 3:54 ` Liang Chen
0 siblings, 1 reply; 6+ messages in thread
From: Benjamin Poirier @ 2023-09-06 22:32 UTC (permalink / raw)
To: Liang Chen; +Cc: davem, edumazet, kuba, pabeni, netdev
On 2023-09-06 18:35 +0800, Liang Chen wrote:
> Currently, skbs generated by pktgen always have their reference count
> incremented before transmission, leading to two issues:
> 1. Only the code paths for shared skbs can be tested.
> 2. Skbs can only be released by pktgen.
> To enhance testing comprehensiveness, introducing the "skb_single_user"
> parameter, which 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 network stack.
If my understanding of the code is correct, pktgen operates in the same
way with parameter clone_skb = 0 and clone_skb = 1.
clone_skb = 0 is already meant to work on devices that don't support
shared skbs (see IFF_TX_SKB_SHARING check in pktgen_if_write()). Instead
of introducing a new option for your purpose, how about changing
pktgen_xmit() to send "not shared" skbs when clone_skb == 0?
Note that for devices without IFF_TX_SKB_SHARING, it would no longer be
possible to have pktgen free skbs. Is that important?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH net-next] pktgen: Introducing a parameter for non-shared skb testing
2023-09-06 22:32 ` Benjamin Poirier
@ 2023-09-07 3:54 ` Liang Chen
2023-09-07 20:19 ` Benjamin Poirier
0 siblings, 1 reply; 6+ messages in thread
From: Liang Chen @ 2023-09-07 3:54 UTC (permalink / raw)
To: Benjamin Poirier; +Cc: davem, edumazet, kuba, pabeni, netdev
On Thu, Sep 7, 2023 at 6:32 AM Benjamin Poirier
<benjamin.poirier@gmail.com> wrote:
>
> On 2023-09-06 18:35 +0800, Liang Chen wrote:
> > Currently, skbs generated by pktgen always have their reference count
> > incremented before transmission, leading to two issues:
> > 1. Only the code paths for shared skbs can be tested.
> > 2. Skbs can only be released by pktgen.
> > To enhance testing comprehensiveness, introducing the "skb_single_user"
> > parameter, which 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 network stack.
>
> If my understanding of the code is correct, pktgen operates in the same
> way with parameter clone_skb = 0 and clone_skb = 1.
>
Yeah. pktgen seems to treat user count of 2 as not shared, as long as
the skb is not reused for burst or clone_skb. In that case the only
thing left to do with the skb is to check if user count is
decremented.
> clone_skb = 0 is already meant to work on devices that don't support
> shared skbs (see IFF_TX_SKB_SHARING check in pktgen_if_write()). Instead
> of introducing a new option for your purpose, how about changing
> pktgen_xmit() to send "not shared" skbs when clone_skb == 0?
>
Using clone_skb = 0 to enforce non-sharing makes sense to me. However,
we are a bit concerned that such a change would affect existing users
who have been assuming the current behavior.
> Note that for devices without IFF_TX_SKB_SHARING, it would no longer be
> possible to have pktgen free skbs. Is that important?
It seems that only the "count" capability depends on that. In fact,
this patch is attempting to allow skb release within the network stack
when possible. BTW, to strictly obey the IFF_TX_SKB_SHARING flag,
perhaps the "count" capability can be implemented by supplying a
destructor to skbs.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH net-next] pktgen: Introducing a parameter for non-shared skb testing
2023-09-07 3:54 ` Liang Chen
@ 2023-09-07 20:19 ` Benjamin Poirier
2023-09-11 6:25 ` Liang Chen
0 siblings, 1 reply; 6+ messages in thread
From: Benjamin Poirier @ 2023-09-07 20:19 UTC (permalink / raw)
To: Liang Chen; +Cc: davem, edumazet, kuba, pabeni, netdev
On 2023-09-07 11:54 +0800, Liang Chen wrote:
> On Thu, Sep 7, 2023 at 6:32 AM Benjamin Poirier
> <benjamin.poirier@gmail.com> wrote:
> >
> > On 2023-09-06 18:35 +0800, Liang Chen wrote:
> > > Currently, skbs generated by pktgen always have their reference count
> > > incremented before transmission, leading to two issues:
> > > 1. Only the code paths for shared skbs can be tested.
> > > 2. Skbs can only be released by pktgen.
> > > To enhance testing comprehensiveness, introducing the "skb_single_user"
> > > parameter, which 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 network stack.
> >
> > If my understanding of the code is correct, pktgen operates in the same
> > way with parameter clone_skb = 0 and clone_skb = 1.
> >
>
> Yeah. pktgen seems to treat user count of 2 as not shared, as long as
> the skb is not reused for burst or clone_skb. In that case the only
> thing left to do with the skb is to check if user count is
> decremented.
>
> > clone_skb = 0 is already meant to work on devices that don't support
> > shared skbs (see IFF_TX_SKB_SHARING check in pktgen_if_write()). Instead
> > of introducing a new option for your purpose, how about changing
> > pktgen_xmit() to send "not shared" skbs when clone_skb == 0?
> >
>
> Using clone_skb = 0 to enforce non-sharing makes sense to me. However,
> we are a bit concerned that such a change would affect existing users
> who have been assuming the current behavior.
I looked into it more and mode netif_receive only supports clone_skb = 0
and normally reuses the same skb all the time. In order to support
shared/non-shared, I think a new parameter is needed, indeed.
Here are some comments on the rest of the patch:
> ---
> net/core/pktgen.c | 39 ++++++++++++++++++++++++++++++++++++---
> 1 file changed, 36 insertions(+), 3 deletions(-)
>
> diff --git a/net/core/pktgen.c b/net/core/pktgen.c
> index f56b8d697014..8f48272b9d4b 100644
> --- a/net/core/pktgen.c
> +++ b/net/core/pktgen.c
> @@ -423,6 +423,7 @@ struct pktgen_dev {
> __u32 skb_priority; /* skb priority field */
> unsigned int burst; /* number of duplicated packets to burst */
> int node; /* Memory node */
> + int skb_single_user; /* allow single user skb for transmission */
>
> #ifdef CONFIG_XFRM
> __u8 ipsmode; /* IPSEC mode (config) */
> @@ -1805,6 +1806,17 @@ static ssize_t pktgen_if_write(struct file *file,
> return count;
> }
>
> + if (!strcmp(name, "skb_single_user")) {
> + len = num_arg(&user_buffer[i], 1, &value);
> + if (len < 0)
> + return len;
> +
> + i += len;
> + pkt_dev->skb_single_user = value;
> + sprintf(pg_result, "OK: skb_single_user=%u", pkt_dev->skb_single_user);
> + return count;
> + }
> +
Since skb_single_user is a boolean, it seems that it should be a flag
(pkt_dev->flags), not a parameter.
Since "non shared" skbs don't really have a name, I would suggest to
avoid inventing a new name and instead call the flag "SHARED" and make
it on by default. So the user would unset the flag to enable the new
behavior.
This patch should also document the new option in
Documentation/networking/pktgen.rst
> sprintf(pkt_dev->result, "No such parameter \"%s\"", name);
> return -EINVAL;
> }
> @@ -3460,6 +3472,14 @@ static void pktgen_xmit(struct pktgen_dev *pkt_dev)
> return;
> }
>
> + /* If clone_skb, burst, or count parameters are configured,
> + * it implies the need for skb reuse, hence single user skb
> + * transmission is not allowed.
> + */
> + if (pkt_dev->skb_single_user && (pkt_dev->clone_skb ||
> + burst > 1 || pkt_dev->count))
> + pkt_dev->skb_single_user = 0;
> +
count > 0 does not imply reuse. That restriction can be lifted.
Instead of silently disabling the option, how about adding these checks
to pktgen_if_write()? The "clone_skb" parameter works that way, for
example.
> /* If no skb or clone count exhausted then get new one */
> if (!pkt_dev->skb || (pkt_dev->last_ok &&
> ++pkt_dev->clone_count >= pkt_dev->clone_skb)) {
> @@ -3483,7 +3503,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->skb_single_user)
> + refcount_add(burst, &skb->users);
> local_bh_disable();
> do {
> ret = netif_receive_skb(skb);
> @@ -3491,6 +3512,12 @@ static void pktgen_xmit(struct pktgen_dev *pkt_dev)
> pkt_dev->errors++;
> pkt_dev->sofar++;
> pkt_dev->seq_num++;
> +
> + if (pkt_dev->skb_single_user) {
> + pkt_dev->skb = NULL;
> + break;
> + }
> +
The assignment can be moved out of the loop, with the other 'if' in the
previous hunk.
> if (refcount_read(&skb->users) != burst) {
> /* skb was queued by rps/rfs or taps,
> * so cannot reuse this skb
> @@ -3509,7 +3536,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->skb_single_user)
> + refcount_inc(&pkt_dev->skb->users);
>
> ret = dev_queue_xmit(pkt_dev->skb);
> switch (ret) {
> @@ -3517,6 +3545,8 @@ 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->skb_single_user)
> + pkt_dev->skb = NULL;
> break;
> case NET_XMIT_DROP:
> case NET_XMIT_CN:
This code can lead to a use after free of pkt_dev->skb when
dev_queue_xmit() returns ex. NET_XMIT_DROP. The skb has been freed by
the stack but pkt_dev->skb is still set.
It can be triggered like this:
ip link add dummy0 up type dummy
tc qdisc add dev dummy0 clsact
tc filter add dev dummy0 egress matchall action drop
And then run pktgen on dummy0 with "skb_single_user 1" and "xmit_mode
queue_xmit"
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH net-next] pktgen: Introducing a parameter for non-shared skb testing
2023-09-07 20:19 ` Benjamin Poirier
@ 2023-09-11 6:25 ` Liang Chen
2023-09-13 4:51 ` Liang Chen
0 siblings, 1 reply; 6+ messages in thread
From: Liang Chen @ 2023-09-11 6:25 UTC (permalink / raw)
To: Benjamin Poirier; +Cc: davem, edumazet, kuba, pabeni, netdev
On Fri, Sep 8, 2023 at 4:19 AM Benjamin Poirier
<benjamin.poirier@gmail.com> wrote:
>
> On 2023-09-07 11:54 +0800, Liang Chen wrote:
> > On Thu, Sep 7, 2023 at 6:32 AM Benjamin Poirier
> > <benjamin.poirier@gmail.com> wrote:
> > >
> > > On 2023-09-06 18:35 +0800, Liang Chen wrote:
> > > > Currently, skbs generated by pktgen always have their reference count
> > > > incremented before transmission, leading to two issues:
> > > > 1. Only the code paths for shared skbs can be tested.
> > > > 2. Skbs can only be released by pktgen.
> > > > To enhance testing comprehensiveness, introducing the "skb_single_user"
> > > > parameter, which 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 network stack.
> > >
> > > If my understanding of the code is correct, pktgen operates in the same
> > > way with parameter clone_skb = 0 and clone_skb = 1.
> > >
> >
> > Yeah. pktgen seems to treat user count of 2 as not shared, as long as
> > the skb is not reused for burst or clone_skb. In that case the only
> > thing left to do with the skb is to check if user count is
> > decremented.
> >
> > > clone_skb = 0 is already meant to work on devices that don't support
> > > shared skbs (see IFF_TX_SKB_SHARING check in pktgen_if_write()). Instead
> > > of introducing a new option for your purpose, how about changing
> > > pktgen_xmit() to send "not shared" skbs when clone_skb == 0?
> > >
> >
> > Using clone_skb = 0 to enforce non-sharing makes sense to me. However,
> > we are a bit concerned that such a change would affect existing users
> > who have been assuming the current behavior.
>
> I looked into it more and mode netif_receive only supports clone_skb = 0
> and normally reuses the same skb all the time. In order to support
> shared/non-shared, I think a new parameter is needed, indeed.
>
Sure, we will introduce a new parameter and store the value in the
flag, as you suggested below. Thanks.
> Here are some comments on the rest of the patch:
>
> > ---
> > net/core/pktgen.c | 39 ++++++++++++++++++++++++++++++++++++---
> > 1 file changed, 36 insertions(+), 3 deletions(-)
> >
> > diff --git a/net/core/pktgen.c b/net/core/pktgen.c
> > index f56b8d697014..8f48272b9d4b 100644
> > --- a/net/core/pktgen.c
> > +++ b/net/core/pktgen.c
> > @@ -423,6 +423,7 @@ struct pktgen_dev {
> > __u32 skb_priority; /* skb priority field */
> > unsigned int burst; /* number of duplicated packets to burst */
> > int node; /* Memory node */
> > + int skb_single_user; /* allow single user skb for transmission */
> >
> > #ifdef CONFIG_XFRM
> > __u8 ipsmode; /* IPSEC mode (config) */
> > @@ -1805,6 +1806,17 @@ static ssize_t pktgen_if_write(struct file *file,
> > return count;
> > }
> >
> > + if (!strcmp(name, "skb_single_user")) {
> > + len = num_arg(&user_buffer[i], 1, &value);
> > + if (len < 0)
> > + return len;
> > +
> > + i += len;
> > + pkt_dev->skb_single_user = value;
> > + sprintf(pg_result, "OK: skb_single_user=%u", pkt_dev->skb_single_user);
> > + return count;
> > + }
> > +
>
> Since skb_single_user is a boolean, it seems that it should be a flag
> (pkt_dev->flags), not a parameter.
>
> Since "non shared" skbs don't really have a name, I would suggest to
> avoid inventing a new name and instead call the flag "SHARED" and make
> it on by default. So the user would unset the flag to enable the new
> behavior.
>
> This patch should also document the new option in
> Documentation/networking/pktgen.rst
>
Sure, "SHARED" sounds good to me as well, and the relevant document
will be supplied.
> > sprintf(pkt_dev->result, "No such parameter \"%s\"", name);
> > return -EINVAL;
> > }
> > @@ -3460,6 +3472,14 @@ static void pktgen_xmit(struct pktgen_dev *pkt_dev)
> > return;
> > }
> >
> > + /* If clone_skb, burst, or count parameters are configured,
> > + * it implies the need for skb reuse, hence single user skb
> > + * transmission is not allowed.
> > + */
> > + if (pkt_dev->skb_single_user && (pkt_dev->clone_skb ||
> > + burst > 1 || pkt_dev->count))
> > + pkt_dev->skb_single_user = 0;
> > +
>
> count > 0 does not imply reuse. That restriction can be lifted.
>
If 'count' is set, pktgen_wait_for_skb waits for the completion of the
last sent skb by polling the user count. So it still has an implicit
dependency on the user count.
> Instead of silently disabling the option, how about adding these checks
> to pktgen_if_write()? The "clone_skb" parameter works that way, for
> example.
>
Yes, silently disabling the option can be confusing for users. We will
make this a parameter condition check instead.
> > /* If no skb or clone count exhausted then get new one */
> > if (!pkt_dev->skb || (pkt_dev->last_ok &&
> > ++pkt_dev->clone_count >= pkt_dev->clone_skb)) {
> > @@ -3483,7 +3503,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->skb_single_user)
> > + refcount_add(burst, &skb->users);
> > local_bh_disable();
> > do {
> > ret = netif_receive_skb(skb);
> > @@ -3491,6 +3512,12 @@ static void pktgen_xmit(struct pktgen_dev *pkt_dev)
> > pkt_dev->errors++;
> > pkt_dev->sofar++;
> > pkt_dev->seq_num++;
> > +
> > + if (pkt_dev->skb_single_user) {
> > + pkt_dev->skb = NULL;
> > + break;
> > + }
> > +
>
> The assignment can be moved out of the loop, with the other 'if' in the
> previous hunk.
>
Sure.
> > if (refcount_read(&skb->users) != burst) {
> > /* skb was queued by rps/rfs or taps,
> > * so cannot reuse this skb
> > @@ -3509,7 +3536,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->skb_single_user)
> > + refcount_inc(&pkt_dev->skb->users);
> >
> > ret = dev_queue_xmit(pkt_dev->skb);
> > switch (ret) {
> > @@ -3517,6 +3545,8 @@ 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->skb_single_user)
> > + pkt_dev->skb = NULL;
>
> > break;
> > case NET_XMIT_DROP:
> > case NET_XMIT_CN:
>
> This code can lead to a use after free of pkt_dev->skb when
> dev_queue_xmit() returns ex. NET_XMIT_DROP. The skb has been freed by
> the stack but pkt_dev->skb is still set.
>
> It can be triggered like this:
> ip link add dummy0 up type dummy
> tc qdisc add dev dummy0 clsact
> tc filter add dev dummy0 egress matchall action drop
> And then run pktgen on dummy0 with "skb_single_user 1" and "xmit_mode
> queue_xmit"
Sure. Thanks for pointing out this issue! It will be fixed in v2.
Thanks,
Liang
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH net-next] pktgen: Introducing a parameter for non-shared skb testing
2023-09-11 6:25 ` Liang Chen
@ 2023-09-13 4:51 ` Liang Chen
0 siblings, 0 replies; 6+ messages in thread
From: Liang Chen @ 2023-09-13 4:51 UTC (permalink / raw)
To: Benjamin Poirier; +Cc: davem, edumazet, kuba, pabeni, netdev
On Mon, Sep 11, 2023 at 2:25 PM Liang Chen <liangchen.linux@gmail.com> wrote:
>
> On Fri, Sep 8, 2023 at 4:19 AM Benjamin Poirier
> <benjamin.poirier@gmail.com> wrote:
> >
> > On 2023-09-07 11:54 +0800, Liang Chen wrote:
> > > On Thu, Sep 7, 2023 at 6:32 AM Benjamin Poirier
> > > <benjamin.poirier@gmail.com> wrote:
> > > >
> > > > On 2023-09-06 18:35 +0800, Liang Chen wrote:
> > > > > Currently, skbs generated by pktgen always have their reference count
> > > > > incremented before transmission, leading to two issues:
> > > > > 1. Only the code paths for shared skbs can be tested.
> > > > > 2. Skbs can only be released by pktgen.
> > > > > To enhance testing comprehensiveness, introducing the "skb_single_user"
> > > > > parameter, which 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 network stack.
> > > >
> > > > If my understanding of the code is correct, pktgen operates in the same
> > > > way with parameter clone_skb = 0 and clone_skb = 1.
> > > >
> > >
> > > Yeah. pktgen seems to treat user count of 2 as not shared, as long as
> > > the skb is not reused for burst or clone_skb. In that case the only
> > > thing left to do with the skb is to check if user count is
> > > decremented.
> > >
> > > > clone_skb = 0 is already meant to work on devices that don't support
> > > > shared skbs (see IFF_TX_SKB_SHARING check in pktgen_if_write()). Instead
> > > > of introducing a new option for your purpose, how about changing
> > > > pktgen_xmit() to send "not shared" skbs when clone_skb == 0?
> > > >
> > >
> > > Using clone_skb = 0 to enforce non-sharing makes sense to me. However,
> > > we are a bit concerned that such a change would affect existing users
> > > who have been assuming the current behavior.
> >
> > I looked into it more and mode netif_receive only supports clone_skb = 0
> > and normally reuses the same skb all the time. In order to support
> > shared/non-shared, I think a new parameter is needed, indeed.
> >
>
> Sure, we will introduce a new parameter and store the value in the
> flag, as you suggested below. Thanks.
>
> > Here are some comments on the rest of the patch:
> >
> > > ---
> > > net/core/pktgen.c | 39 ++++++++++++++++++++++++++++++++++++---
> > > 1 file changed, 36 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/net/core/pktgen.c b/net/core/pktgen.c
> > > index f56b8d697014..8f48272b9d4b 100644
> > > --- a/net/core/pktgen.c
> > > +++ b/net/core/pktgen.c
> > > @@ -423,6 +423,7 @@ struct pktgen_dev {
> > > __u32 skb_priority; /* skb priority field */
> > > unsigned int burst; /* number of duplicated packets to burst */
> > > int node; /* Memory node */
> > > + int skb_single_user; /* allow single user skb for transmission */
> > >
> > > #ifdef CONFIG_XFRM
> > > __u8 ipsmode; /* IPSEC mode (config) */
> > > @@ -1805,6 +1806,17 @@ static ssize_t pktgen_if_write(struct file *file,
> > > return count;
> > > }
> > >
> > > + if (!strcmp(name, "skb_single_user")) {
> > > + len = num_arg(&user_buffer[i], 1, &value);
> > > + if (len < 0)
> > > + return len;
> > > +
> > > + i += len;
> > > + pkt_dev->skb_single_user = value;
> > > + sprintf(pg_result, "OK: skb_single_user=%u", pkt_dev->skb_single_user);
> > > + return count;
> > > + }
> > > +
> >
> > Since skb_single_user is a boolean, it seems that it should be a flag
> > (pkt_dev->flags), not a parameter.
> >
> > Since "non shared" skbs don't really have a name, I would suggest to
> > avoid inventing a new name and instead call the flag "SHARED" and make
> > it on by default. So the user would unset the flag to enable the new
> > behavior.
> >
> > This patch should also document the new option in
> > Documentation/networking/pktgen.rst
> >
>
> Sure, "SHARED" sounds good to me as well, and the relevant document
> will be supplied.
>
> > > sprintf(pkt_dev->result, "No such parameter \"%s\"", name);
> > > return -EINVAL;
> > > }
> > > @@ -3460,6 +3472,14 @@ static void pktgen_xmit(struct pktgen_dev *pkt_dev)
> > > return;
> > > }
> > >
> > > + /* If clone_skb, burst, or count parameters are configured,
> > > + * it implies the need for skb reuse, hence single user skb
> > > + * transmission is not allowed.
> > > + */
> > > + if (pkt_dev->skb_single_user && (pkt_dev->clone_skb ||
> > > + burst > 1 || pkt_dev->count))
> > > + pkt_dev->skb_single_user = 0;
> > > +
> >
> > count > 0 does not imply reuse. That restriction can be lifted.
> >
>
> If 'count' is set, pktgen_wait_for_skb waits for the completion of the
> last sent skb by polling the user count. So it still has an implicit
> dependency on the user count.
>
> > Instead of silently disabling the option, how about adding these checks
> > to pktgen_if_write()? The "clone_skb" parameter works that way, for
> > example.
> >
>
> Yes, silently disabling the option can be confusing for users. We will
> make this a parameter condition check instead.
>
> > > /* If no skb or clone count exhausted then get new one */
> > > if (!pkt_dev->skb || (pkt_dev->last_ok &&
> > > ++pkt_dev->clone_count >= pkt_dev->clone_skb)) {
> > > @@ -3483,7 +3503,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->skb_single_user)
> > > + refcount_add(burst, &skb->users);
> > > local_bh_disable();
> > > do {
> > > ret = netif_receive_skb(skb);
> > > @@ -3491,6 +3512,12 @@ static void pktgen_xmit(struct pktgen_dev *pkt_dev)
> > > pkt_dev->errors++;
> > > pkt_dev->sofar++;
> > > pkt_dev->seq_num++;
> > > +
> > > + if (pkt_dev->skb_single_user) {
> > > + pkt_dev->skb = NULL;
> > > + break;
> > > + }
> > > +
> >
> > The assignment can be moved out of the loop, with the other 'if' in the
> > previous hunk.
> >
>
> Sure.
The assignment needs to occur after netif_receive_skb. So moving the
assignment above the loop would require netif_receive_skb and its
associated statistics being duplicated as well. Therefore, we mark the
condition 'unlikely' to optimize the case when 'burst' is set and it
loops.
>
> > > if (refcount_read(&skb->users) != burst) {
> > > /* skb was queued by rps/rfs or taps,
> > > * so cannot reuse this skb
> > > @@ -3509,7 +3536,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->skb_single_user)
> > > + refcount_inc(&pkt_dev->skb->users);
> > >
> > > ret = dev_queue_xmit(pkt_dev->skb);
> > > switch (ret) {
> > > @@ -3517,6 +3545,8 @@ 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->skb_single_user)
> > > + pkt_dev->skb = NULL;
> >
> > > break;
> > > case NET_XMIT_DROP:
> > > case NET_XMIT_CN:
> >
> > This code can lead to a use after free of pkt_dev->skb when
> > dev_queue_xmit() returns ex. NET_XMIT_DROP. The skb has been freed by
> > the stack but pkt_dev->skb is still set.
> >
> > It can be triggered like this:
> > ip link add dummy0 up type dummy
> > tc qdisc add dev dummy0 clsact
> > tc filter add dev dummy0 egress matchall action drop
> > And then run pktgen on dummy0 with "skb_single_user 1" and "xmit_mode
> > queue_xmit"
>
> Sure. Thanks for pointing out this issue! It will be fixed in v2.
>
>
> Thanks,
> Liang
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-09-13 4:51 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-09-06 10:35 [RFC PATCH net-next] pktgen: Introducing a parameter for non-shared skb testing Liang Chen
2023-09-06 22:32 ` Benjamin Poirier
2023-09-07 3:54 ` Liang Chen
2023-09-07 20:19 ` Benjamin Poirier
2023-09-11 6:25 ` Liang Chen
2023-09-13 4:51 ` 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).