Message-ID: <CAKhg4tJzFnntRJhLyo-W3gR=kt59vrA3hiu1+shJu1G=LK_C2g@mail.gmail.com>
Date: Wed, 13 Sep 2023 12:51:29 +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 Mon, Sep 11, 2023 at 2:25 PM Liang Chen <liangchen.linux@...il.com> wrote:
>
> 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.

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

Powered by blists - more mailing lists