| 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: Thu, 14 Sep 2023 11:32:44 +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 v2] pktgen: Introducing 'SHARED' flag for testing
with non-shared skb
On Thu, Sep 14, 2023 at 1:02 AM Benjamin Poirier
<benjamin.poirier@...il.com> wrote:
>
> On 2023-09-13 13:10 +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>
> > ---
> > Documentation/networking/pktgen.rst | 13 ++++++++
> > net/core/pktgen.c | 50 +++++++++++++++++++++++++----
> > 2 files changed, 56 insertions(+), 7 deletions(-)
> >
> > diff --git a/Documentation/networking/pktgen.rst b/Documentation/networking/pktgen.rst
> > index 1225f0f63ff0..ffd976c0cbf0 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,17 @@ 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).
> > +If you need to test with non-shared SKBs, you can remove the "SHARED" flag
> > +by simply setting::
> > +
> > + pg_set "flag !SHARED"
> > +
> > +However, if the "clone_skb," "burst," or "count" 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 +369,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 f56b8d697014..3cf00090cf09 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 see that imix uses EINVAL but I would suggest to add the new check to
> the previous condition which returns -ENOTSUPP. A value like "clone_skb
> 1" is not invalid by itself.
>
Returning 'ENOTSUPP' seems to imply that "such a option is not
supported by the NIC", while we are trying to convey to users "such a
combination is invalid'. So it is a bit confusing as to which one to
return.
> >
> > i += len;
> > @@ -1212,6 +1214,9 @@ static ssize_t pktgen_if_write(struct file *file,
> > if (len < 0)
> > return len;
> >
> > + if ((value > 0) && !(pkt_dev->flags & F_SHARED))
> > + return -EINVAL;
> > +
>
> I think the restriction on count == 0 can be dropped by adding the
> following changes, do you agree?
>
Yes, I agree. At least, this doesn't change the existing behavior
without using the 'SHARED' flag. Only in that case, the code doesn't
wait for the last packet to be transmitted.
> diff --git a/net/core/pktgen.c b/net/core/pktgen.c
> index 3cf00090cf09..6fe19783b060 100644
> --- a/net/core/pktgen.c
> +++ b/net/core/pktgen.c
> @@ -1214,9 +1214,6 @@ static ssize_t pktgen_if_write(struct file *file,
> if (len < 0)
> return len;
>
> - if ((value > 0) && !(pkt_dev->flags & F_SHARED))
> - return -EINVAL;
> -
> i += len;
> pkt_dev->count = value;
> sprintf(pg_result, "OK: count=%llu",
> @@ -1344,12 +1341,12 @@ static ssize_t pktgen_if_write(struct file *file,
>
> if (flag) {
> if (disable) {
> - /* If "clone_skb", "burst", or "count" parameters are
> + /* 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 || pkt_dev->count))
> + pkt_dev->burst > 1))
> return -EINVAL;
> pkt_dev->flags &= ~flag;
> } else {
> @@ -3623,7 +3620,8 @@ static void pktgen_xmit(struct pktgen_dev *pkt_dev)
>
> /* If pkt_dev->count is zero, then run forever */
> if ((pkt_dev->count != 0) && (pkt_dev->sofar >= pkt_dev->count)) {
> - pktgen_wait_for_skb(pkt_dev);
> + if (pkt_dev->skb)
> + pktgen_wait_for_skb(pkt_dev);
>
> /* Done with this */
> pktgen_stop_device(pkt_dev);
>
>
> > i += len;
> > pkt_dev->count = value;
> > sprintf(pg_result, "OK: count=%llu",
> > @@ -1257,6 +1262,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;
> > +
>
> Please integrate the new check with the existing ones in the 'if' just
> above.
>
> > pkt_dev->burst = value < 1 ? 1 : value;
> > sprintf(pg_result, "OK: burst=%u", pkt_dev->burst);
> > return count;
> > @@ -1334,10 +1343,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", "burst", or "count" 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 || pkt_dev->count))
> > + return -EINVAL;
>
> netdev uses an 80 column limit
> https://github.com/kuba-moo/nipa/blob/853db1f2a67758d839324920f7319b94630efd17/tests/patch/checkpatch/checkpatch.sh#L16
>
Sure.
> > pkt_dev->flags &= ~flag;
> > - else
> > + } else {
> > pkt_dev->flags |= flag;
> > + }
> > } else {
> > sprintf(pg_result,
> > "Flag -:%s:- unknown\nAvailable flags, (prepend ! to un-set flag):\n%s",
> > @@ -1350,7 +1367,8 @@ static ssize_t pktgen_if_write(struct file *file,
> > #ifdef CONFIG_XFRM
> > "IPSEC, "
> > #endif
> > - "NODE_ALLOC\n");
> > + "NODE_ALLOC, "
> > + "SHARED\n");
>
> This triggers a checkpatch warning. How about cleaning that code up in a
> preceding patch to use pkt_flag_names[] instead?
>
Sure.
> > return count;
> > }
> > sprintf(pg_result, "OK: flags=0x%x", pkt_dev->flags);
> > @@ -3483,7 +3501,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);
> > @@ -3491,6 +3510,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
> > @@ -3509,7 +3532,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->flags & F_SHARED)
> > + refcount_inc(&pkt_dev->skb->users);
> >
> > ret = dev_queue_xmit(pkt_dev->skb);
> > switch (ret) {
> > @@ -3517,8 +3541,13 @@ 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->flags & F_SHARED))
> > + pkt_dev->skb = NULL;
> > break;
> > case NET_XMIT_DROP:
> > + if (!(pkt_dev->flags & F_SHARED))
> > + pkt_dev->skb = NULL;
> > + fallthrough;
> > case NET_XMIT_CN:
> > /* These are all valid return codes for a qdisc but
> > * indicate packets are being dropped or will likely
>
> This patch introduces almost the same use after free problem as v1.
> Please take the time to look at the existing code to understand the
> cases when a skb refcount is dropped by the stack.
>
> This time the problem can be triggered with:
> ip link add dummy0 up type dummy
> tc qdisc add dev dummy0 root pfifo_head_drop limit 0
> And then run pktgen on dummy0 with "flag !SHARED" and "xmit_mode
> queue_xmit"
Thank you for pointing it out. We will take a closer look into this problem.
Thanks,
Liang
Powered by blists - more mailing lists