| 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
| ||
|
Message-ID: <CAKhg4tKZsx6cwcZf94ZgkErzWy-EQRJBiXEJJ45bso9g1xAkpg@mail.gmail.com>
Date: Sat, 16 Sep 2023 11:20:24 +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: [PATCH net-next v3 2/2] pktgen: Introducing 'SHARED' flag for
testing with non-shared skb
On Sat, Sep 16, 2023 at 8:45 AM Benjamin Poirier
<benjamin.poirier@...il.com> wrote:
>
> On 2023-09-15 20:23 +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@...il.com>
> >
> > ---
> > Changes from v2:
> > - Lifted the check on 'count' when 'not shared' is configured.
> > - Fixed a use-after-free problem when sending failed
> > ---
> > Documentation/networking/pktgen.rst | 12 ++++++++
> > net/core/pktgen.c | 47 ++++++++++++++++++++++++-----
> > 2 files changed, 51 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 ffd659dbd6c3..5cc69feec7d7 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;
> > +
>
> Make sure to run checkpatch and check the patchwork results. There are
> some points to correct:
> https://patchwork.kernel.org/project/netdevbpf/patch/20230915122317.100390-2-liangchen.linux@gmail.com/
>
Sure. Will do.
> > pkt_dev->burst = value < 1 ? 1 : value;
> > sprintf(pg_result, "OK: burst=%u", pkt_dev->burst);
> > return count;
> > @@ -1335,10 +1341,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", 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;
> > + }
> > } else {
> > pg_result += sprintf(pg_result,
> > "Flag -:%s:- unknown\n%s", f,
> > @@ -3485,7 +3499,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);
> > @@ -3493,6 +3508,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
> > @@ -3511,9 +3530,14 @@ 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);
> > +
> > + if (!(pkt_dev->flags & F_SHARED) && dev_xmit_complete(ret))
> > + pkt_dev->skb = NULL;
> > +
> > switch (ret) {
> > case NET_XMIT_SUCCESS:
> > pkt_dev->sofar++;
> > @@ -3551,11 +3575,15 @@ 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);
> >
> > + if (!(pkt_dev->flags & F_SHARED) && dev_xmit_complete(ret))
> > + pkt_dev->skb = NULL;
> > +
> > switch (ret) {
> > case NETDEV_TX_OK:
> > pkt_dev->last_ok = 1;
> > @@ -3577,7 +3605,8 @@ static void pktgen_xmit(struct pktgen_dev *pkt_dev)
> > fallthrough;
> > case NETDEV_TX_BUSY:
> > /* Retry it next time */
> > - refcount_dec(&(pkt_dev->skb->users));
> > + if (!(pkt_dev->flags & F_SHARED))
> > + refcount_dec(&(pkt_dev->skb->users));
>
> With "flag !SHARED", this leads to a refcount underflow if the driver
> returns NETDEV_TX_BUSY.
>
> It looks like the condition is inverted, no?
>
Yeah, my bad. Sorry about that. Thanks!
> I tested it by hacking e1000_xmit_frame() to return NETDEV_TX_BUSY right
> at the beginning.
Powered by blists - more mailing lists