| 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: <CANn89iLA5irwbuqvJdnptGs9pQNO_63qQsJ1jjZd1E0Cd4JVMw@mail.gmail.com>
Date: Mon, 18 Sep 2023 16:28:34 +0200
From: Eric Dumazet <edumazet@...gle.com>
To: Liang Chen <liangchen.linux@...il.com>
Cc: davem@...emloft.net, kuba@...nel.org, pabeni@...hat.com,
benjamin.poirier@...il.com, netdev@...r.kernel.org
Subject: Re: [PATCH net-next v4 2/2] pktgen: Introducing 'SHARED' flag for
testing with non-shared skb
On Sat, Sep 16, 2023 at 3:30 PM Liang Chen <liangchen.linux@...il.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@...il.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...
syzbot probably never figured out how to run pktgen, it is a matter of time...
Powered by blists - more mailing lists