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: <CAKhg4t+mx3m+_JgR7CWRtLUS_8k+mm_NFVQh5aFdycq7M5LomQ@mail.gmail.com> 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