| 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
| ||
|
Date: Mon, 11 Sep 2023 14:25:08 +0800
From: Liang Chen <liangchen.linux@...il.com>
To: Benjamin Poirier <benjamin.poirier@...il.com>
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
pabeni@...hat.com, netdev@...r.kernel.org
Subject: Re: [RFC PATCH net-next] pktgen: Introducing a parameter for
non-shared skb testing
On Fri, Sep 8, 2023 at 4:19 AM Benjamin Poirier
<benjamin.poirier@...il.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@...il.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
Powered by blists - more mailing lists