Re: [PATCH net-next v4 2/2] pktgen: Introducing 'SHARED' flag for testing with non-shared skb

[Paolo Abeni <pabeni@redhat.com>]

On Wed, 2023-09-20 at 12:09 +0800, Liang Chen wrote:
> On Tue, Sep 19, 2023 at 4:09 PM Paolo Abeni <pabeni@redhat.com> wrote:
> > 
> > On Mon, 2023-09-18 at 16:28 +0200, Eric Dumazet wrote:
> > > On Sat, Sep 16, 2023 at 3:30 PM Liang Chen <liangchen.linux@gmail.com> 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 | 12 ++++++++
> > > >  net/core/pktgen.c                   | 48 ++++++++++++++++++++++++-----
> > > >  2 files changed, 52 insertions(+), 8 deletions(-)
> > > > 
> > > > diff --git a/Documentation/networking/pktgen.rst b/Documentation/networking/pktgen.rst
> > > > index 1225f0f63ff0..c945218946e1 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,16 @@ 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).
> > > > +To test with non-shared SKBs, remove the "SHARED" flag by simply setting::
> > > > +
> > > > +       pg_set "flag !SHARED"
> > > > +
> > > > +However, if the "clone_skb" or "burst" 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 +368,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 48306a101fd9..c4e0814df325 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;
> > > > @@ -1257,6 +1259,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 +1340,19 @@ static ssize_t pktgen_if_write(struct file *file,
> > > > 
> > > >                 flag = pktgen_read_flag(f, &disable);
> > > >                 if (flag) {
> > > > -                       if (disable)
> > > > +                       if (disable) {
> > > > +                               /* 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))
> > > > +                                       return -EINVAL;
> > > >                                 pkt_dev->flags &= ~flag;
> > > > -                       else
> > > > +                       } else {
> > > >                                 pkt_dev->flags |= flag;
> > > > +                       }
> > > > 
> > > >                         sprintf(pg_result, "OK: flags=0x%x", pkt_dev->flags);
> > > >                         return count;
> > > > @@ -3489,7 +3504,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);
> > > > @@ -3497,6 +3513,10 @@ static void pktgen_xmit(struct pktgen_dev *pkt_dev)
> > > >                                 pkt_dev->errors++;
> > > >                         pkt_dev->sofar++;
> > > >                         pkt_dev->seq_num++;
> > > 
> > > Since pkt_dev->flags can change under us, I would rather read pkt_dev->flags
> > > once in pktgen_xmit() to avoid surprises...
> > 
> > Additionally I *think* we can't assume pkt_dev->burst and pkt_dev-
> > > flags have consistent values in pktgen_xmit(). The user-space
> > (syzkaller) could flip burst and flag in between the read access in
> > pktgen_xmit().
> > 
> 
> Thanks for pointing out the issue! We are trying to fix it in the following way,
> 
>  static void pktgen_xmit(struct pktgen_dev *pkt_dev)
>  {
> -       unsigned int burst = READ_ONCE(pkt_dev->burst);
> +       bool skb_shared = !!(READ_ONCE(pkt_dev->flags) & F_SHARED);
>         struct net_device *odev = pkt_dev->odev;
>         struct netdev_queue *txq;
> +       unsigned int burst = 1;
>         struct sk_buff *skb;
> +       int clone_skb = 0;
>         int ret;
> 
> +       if (skb_shared) {
> +               burst = READ_ONCE(pkt_dev->burst);
> +               clone_skb = READ_ONCE(pkt_dev->clone_skb);
> +       }
> +
> 
> So that pktgen_xmit will have consistent 'burst', 'clone_skb', and
> 'skb_shared' values. 

I agree it makes sense and address the potential issues.

Thanks,

Paolo