| 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: Mon, 6 Sep 2021 10:06:30 +0200
From: Magnus Karlsson <magnus.karlsson@...il.com>
To: Maciej Fijalkowski <maciej.fijalkowski@...el.com>
Cc: "Karlsson, Magnus" <magnus.karlsson@...el.com>,
Björn Töpel <bjorn@...nel.org>,
Alexei Starovoitov <ast@...nel.org>,
Daniel Borkmann <daniel@...earbox.net>,
Network Development <netdev@...r.kernel.org>,
Jonathan Lemon <jonathan.lemon@...il.com>,
Ciara Loftus <ciara.loftus@...el.com>,
bpf <bpf@...r.kernel.org>, Yonghong Song <yhs@...com>,
Andrii Nakryiko <andrii@...nel.org>
Subject: Re: [PATCH bpf-next 19/20] selftests: xsk: add tests for invalid xsk descriptors
On Fri, Sep 3, 2021 at 6:57 PM Maciej Fijalkowski
<maciej.fijalkowski@...el.com> wrote:
>
> On Wed, Sep 01, 2021 at 12:47:31PM +0200, Magnus Karlsson wrote:
> > From: Magnus Karlsson <magnus.karlsson@...el.com>
> >
> > Add tests for invalid xsk descriptors in the Tx ring. A number of
> > handcrafted nasty invalid descriptors are created and submitted to the
> > tx ring to check that they are validated correctly. Corener case valid
>
> Corner
Will fix.
> > ones are also sent. The tests are run for both aligned and unaligned
> > mode.
> >
> > pkt_stream_set() is introduced to be able to create a hand-crafted
> > packet stream where every single packet is specified in detail.
> >
> > Signed-off-by: Magnus Karlsson <magnus.karlsson@...el.com>
> > ---
> > tools/testing/selftests/bpf/xdpxceiver.c | 143 ++++++++++++++++++++---
> > tools/testing/selftests/bpf/xdpxceiver.h | 7 +-
> > 2 files changed, 131 insertions(+), 19 deletions(-)
> >
> > diff --git a/tools/testing/selftests/bpf/xdpxceiver.c b/tools/testing/selftests/bpf/xdpxceiver.c
> > index d085033afd53..a4f6ce3a6b14 100644
> > --- a/tools/testing/selftests/bpf/xdpxceiver.c
> > +++ b/tools/testing/selftests/bpf/xdpxceiver.c
> > @@ -46,6 +46,8 @@
> > * then remove xsk sockets from queue 0 on both veth interfaces and
> > * finally run a traffic on queues ids 1
> > * g. unaligned mode
> > + * h. tests for invalid and corner case Tx descriptors so that the correct ones
> > + * are discarded and let through, respectively.
> > *
> > * Total tests: 12
> > *
> > @@ -394,7 +396,7 @@ static void __test_spec_init(struct test_spec *test, struct ifobject *ifobj_tx,
> > for (j = 0; j < MAX_SOCKETS; j++) {
> > memset(&ifobj->umem_arr[j], 0, sizeof(ifobj->umem_arr[j]));
> > memset(&ifobj->xsk_arr[j], 0, sizeof(ifobj->xsk_arr[j]));
> > - ifobj->umem_arr[j].num_frames = DEFAULT_PKT_CNT / 4;
> > + ifobj->umem_arr[j].num_frames = DEFAULT_UMEM_BUFFERS;
> > ifobj->umem_arr[j].frame_size = XSK_UMEM__DEFAULT_FRAME_SIZE;
> > ifobj->xsk_arr[j].rxqsize = XSK_RING_CONS__DEFAULT_NUM_DESCS;
> > }
> > @@ -450,6 +452,16 @@ static struct pkt *pkt_stream_get_pkt(struct pkt_stream *pkt_stream, u32 pkt_nb)
> > return &pkt_stream->pkts[pkt_nb];
> > }
> >
> > +static struct pkt *pkt_stream_get_next_rx_pkt(struct pkt_stream *pkt_stream)
> > +{
> > + while (pkt_stream->rx_pkt_nb < pkt_stream->nb_pkts) {
> > + if (pkt_stream->pkts[pkt_stream->rx_pkt_nb].valid)
> > + return &pkt_stream->pkts[pkt_stream->rx_pkt_nb++];
> > + pkt_stream->rx_pkt_nb++;
> > + }
> > + return NULL;
> > +}
> > +
> > static void pkt_stream_delete(struct pkt_stream *pkt_stream)
> > {
> > free(pkt_stream->pkts);
> > @@ -465,17 +477,31 @@ static void pkt_stream_restore_default(struct test_spec *test)
> > test->ifobj_rx->pkt_stream = test->pkt_stream_default;
> > }
> >
> > -static struct pkt_stream *pkt_stream_generate(struct xsk_umem_info *umem, u32 nb_pkts, u32 pkt_len)
> > +static struct pkt_stream *__pkt_stream_alloc(u32 nb_pkts)
> > {
> > struct pkt_stream *pkt_stream;
> > - u32 i;
> >
> > - pkt_stream = malloc(sizeof(*pkt_stream));
> > + pkt_stream = calloc(1, sizeof(*pkt_stream));
>
> So with that probably my previous comment about use_addr_for_fill being
> not set is not relevant.
Will fix this earlier.
> > if (!pkt_stream)
> > - exit_with_error(ENOMEM);
> > + return NULL;
> >
> > pkt_stream->pkts = calloc(nb_pkts, sizeof(*pkt_stream->pkts));
> > - if (!pkt_stream->pkts)
> > + if (!pkt_stream->pkts) {
> > + free(pkt_stream);
> > + return NULL;
> > + }
> > +
> > + pkt_stream->nb_pkts = nb_pkts;
> > + return pkt_stream;
> > +}
> > +
> > +static struct pkt_stream *pkt_stream_generate(struct xsk_umem_info *umem, u32 nb_pkts, u32 pkt_len)
> > +{
> > + struct pkt_stream *pkt_stream;
> > + u32 i;
> > +
> > + pkt_stream = __pkt_stream_alloc(nb_pkts);
> > + if (!pkt_stream)
> > exit_with_error(ENOMEM);
> >
> > pkt_stream->nb_pkts = nb_pkts;
> > @@ -525,6 +551,26 @@ static void pkt_stream_replace_half(struct test_spec *test, u32 pkt_len, u32 off
> > test->ifobj_rx->pkt_stream = pkt_stream;
> > }
> >
> > +static void pkt_stream_set(struct test_spec *test, struct pkt *pkts, u32 nb_pkts)
>
> This is still a generation of pkt stream, can we name this is as:
> pkt_stream_generate_custom or is this too long? WDYT?
Sounds good.
> > +{
> > + struct pkt_stream *pkt_stream;
> > + u32 i;
> > +
> > + pkt_stream = __pkt_stream_alloc(nb_pkts);
> > + if (!pkt_stream)
> > + exit_with_error(ENOMEM);
> > +
> > + test->ifobj_tx->pkt_stream = pkt_stream;
> > + test->ifobj_rx->pkt_stream = pkt_stream;
> > +
> > + for (i = 0; i < nb_pkts; i++) {
> > + pkt_stream->pkts[i].addr = pkts[i].addr;
> > + pkt_stream->pkts[i].len = pkts[i].len;
> > + pkt_stream->pkts[i].payload = i;
> > + pkt_stream->pkts[i].valid = pkts[i].valid;
> > + }
> > +}
> > +
> > static struct pkt *pkt_generate(struct ifobject *ifobject, u32 pkt_nb)
> > {
> > struct pkt *pkt = pkt_stream_get_pkt(ifobject->pkt_stream, pkt_nb);
> > @@ -535,6 +581,8 @@ static struct pkt *pkt_generate(struct ifobject *ifobject, u32 pkt_nb)
> >
> > if (!pkt)
> > return NULL;
> > + if (!pkt->valid || pkt->len < PKT_SIZE)
> > + return pkt;
> >
> > data = xsk_umem__get_data(ifobject->umem->buffer, pkt->addr);
> > udp_hdr = (struct udphdr *)(data + sizeof(struct ethhdr) + sizeof(struct iphdr));
> > @@ -596,19 +644,24 @@ static bool is_pkt_valid(struct pkt *pkt, void *buffer, u64 addr, u32 len)
> > return false;
> > }
> >
> > + if (len < PKT_SIZE) {
> > + /*Do not try to verify packets that are smaller than minimun size. */
>
> minimum
>
> > + return true;
> > + }
> > +
> > + if (pkt->len != len) {
> > + ksft_test_result_fail
> > + ("ERROR: [%s] expected length [%d], got length [%d]\n",
> > + __func__, pkt->len, len);
> > + return false;
> > + }
> > +
> > if (iphdr->version == IP_PKT_VER && iphdr->tos == IP_PKT_TOS) {
> > u32 seqnum = ntohl(*((u32 *)(data + PKT_HDR_SIZE)));
> >
> > if (opt_pkt_dump)
> > pkt_dump(data, PKT_SIZE);
> >
> > - if (pkt->len != len) {
> > - ksft_test_result_fail
> > - ("ERROR: [%s] expected length [%d], got length [%d]\n",
> > - __func__, pkt->len, len);
> > - return false;
> > - }
> > -
> > if (pkt->payload != seqnum) {
> > ksft_test_result_fail
> > ("ERROR: [%s] expected seqnum [%d], got seqnum [%d]\n",
> > @@ -645,6 +698,15 @@ static void complete_pkts(struct xsk_socket_info *xsk, int batch_size)
> >
> > rcvd = xsk_ring_cons__peek(&xsk->umem->cq, batch_size, &idx);
> > if (rcvd) {
> > + if (rcvd > xsk->outstanding_tx) {
> > + u64 addr = *xsk_ring_cons__comp_addr(&xsk->umem->cq, idx + rcvd - 1);
> > +
> > + ksft_test_result_fail("ERROR: [%s] Too many packets completed\n",
> > + __func__);
> > + ksft_print_msg("Last completion address: %llx\n", addr);
> > + return;
> > + }
> > +
> > xsk_ring_cons__release(&xsk->umem->cq, rcvd);
> > xsk->outstanding_tx -= rcvd;
> > }
> > @@ -653,11 +715,10 @@ static void complete_pkts(struct xsk_socket_info *xsk, int batch_size)
> > static void receive_pkts(struct pkt_stream *pkt_stream, struct xsk_socket_info *xsk,
> > struct pollfd *fds)
> > {
> > - u32 idx_rx = 0, idx_fq = 0, rcvd, i, pkt_count = 0;
> > - struct pkt *pkt;
> > + struct pkt *pkt = pkt_stream_get_next_rx_pkt(pkt_stream);
> > + u32 idx_rx = 0, idx_fq = 0, rcvd, i;
> > int ret;
> >
> > - pkt = pkt_stream_get_pkt(pkt_stream, pkt_count++);
> > while (pkt) {
> > rcvd = xsk_ring_cons__peek(&xsk->rx, BATCH_SIZE, &idx_rx);
> > if (!rcvd) {
> > @@ -685,13 +746,21 @@ static void receive_pkts(struct pkt_stream *pkt_stream, struct xsk_socket_info *
> > const struct xdp_desc *desc = xsk_ring_cons__rx_desc(&xsk->rx, idx_rx++);
> > u64 addr = desc->addr, orig;
> >
> > + if (!pkt) {
> > + ksft_test_result_fail("ERROR: [%s] Received too many packets.\n",
> > + __func__);
> > + ksft_print_msg("Last packet has addr: %llx len: %u\n",
> > + addr, desc->len);
> > + return;
> > + }
> > +
> > orig = xsk_umem__extract_addr(addr);
> > addr = xsk_umem__add_offset_to_addr(addr);
> > if (!is_pkt_valid(pkt, xsk->umem->buffer, addr, desc->len))
> > return;
> >
> > *xsk_ring_prod__fill_addr(&xsk->umem->fq, idx_fq++) = orig;
> > - pkt = pkt_stream_get_pkt(pkt_stream, pkt_count++);
> > + pkt = pkt_stream_get_next_rx_pkt(pkt_stream);
> > }
> >
> > xsk_ring_prod__submit(&xsk->umem->fq, rcvd);
> > @@ -875,6 +944,7 @@ static void testapp_cleanup_xsk_res(struct ifobject *ifobj)
> > {
> > print_verbose("Destroying socket\n");
> > xsk_socket__delete(ifobj->xsk->xsk);
> > + munmap(ifobj->umem->buffer, ifobj->umem->num_frames * ifobj->umem->frame_size);
> > xsk_umem__delete(ifobj->umem->umem);
> > }
> >
> > @@ -1118,6 +1188,33 @@ static bool testapp_unaligned(struct test_spec *test)
> > return true;
> > }
> >
> > +static void testapp_inv_desc(struct test_spec *test)
>
> I'd say that speaking out whole 'invalid' word wouldn't hurt us here.
>
> > +{
> > + struct pkt pkts[] = {{0, 0, 0, true}, /* Zero packet length and zero address allowed */
>
> Above looks a bit weird, maybe this should be formatted same as the lines
> below:
>
> struct pkt pkts[] = {
> /* Zero packet length and zero address allowed */
> {0, 0, 0, true},
>
> > + /* Zero packet length allowed */
> > + {0x1000, 0, 0, true},
> > + /* Straddling the start of umem */
> > + {-2, PKT_SIZE, 0, false},
> > + /* Packet too large */
> > + {0x2000, XSK_UMEM__INVALID_FRAME_SIZE, 0, false},
> > + /* After umem ends */
> > + {UMEM_SIZE, PKT_SIZE, 0, false},
> > + /* Straddle the end of umem */
> > + {UMEM_SIZE - PKT_SIZE / 2, PKT_SIZE, 0, false},
> > + /* Straddle a page boundrary */
> > + {0x3000 - PKT_SIZE / 2, PKT_SIZE, 0, false},
> > + /* Valid packet for synch so that something is received */
> > + {0x4000, PKT_SIZE, 0, true}};
> > +
> > + if (test->ifobj_tx->umem->unaligned_mode) {
> > + /* Crossing a page boundrary allowed */
> > + pkts[6].valid = true;
> > + }
> > + pkt_stream_set(test, pkts, ARRAY_SIZE(pkts));
> > + testapp_validate_traffic(test);
> > + pkt_stream_restore_default(test);
> > +}
> > +
> > static void init_iface(struct ifobject *ifobj, const char *dst_mac, const char *src_mac,
> > const char *dst_ip, const char *src_ip, const u16 dst_port,
> > const u16 src_port, thread_func_t func_ptr)
> > @@ -1159,7 +1256,7 @@ static void run_pkt_test(struct test_spec *test, enum test_mode mode, enum test_
> > case TEST_TYPE_BPF_RES:
> > testapp_bpf_res(test);
> > break;
> > - case TEST_TYPE_NOPOLL:
> > + case TEST_TYPE_RUN_TO_COMPLETION:
>
> I think that you were updating some comment around it on previous patches
> and it probably belongs to this one.
Will fix plus all the other suggestions above.
Thank you Maciej!
/Magnus
> > test_spec_set_name(test, "RUN_TO_COMPLETION");
> > testapp_validate_traffic(test);
> > break;
> > @@ -1169,6 +1266,16 @@ static void run_pkt_test(struct test_spec *test, enum test_mode mode, enum test_
> > test_spec_set_name(test, "POLL");
> > testapp_validate_traffic(test);
> > break;
> > + case TEST_TYPE_ALIGNED_INV_DESC:
> > + test_spec_set_name(test, "ALIGNED_INV_DESC");
> > + testapp_inv_desc(test);
> > + break;
> > + case TEST_TYPE_UNALIGNED_INV_DESC:
> > + test_spec_set_name(test, "UNALIGNED_INV_DESC");
> > + test->ifobj_tx->umem->unaligned_mode = true;
> > + test->ifobj_rx->umem->unaligned_mode = true;
> > + testapp_inv_desc(test);
> > + break;
> > case TEST_TYPE_UNALIGNED:
> > if (!testapp_unaligned(test))
> > return;
> > diff --git a/tools/testing/selftests/bpf/xdpxceiver.h b/tools/testing/selftests/bpf/xdpxceiver.h
> > index 129801eb013c..2d9efb89ea28 100644
> > --- a/tools/testing/selftests/bpf/xdpxceiver.h
> > +++ b/tools/testing/selftests/bpf/xdpxceiver.h
> > @@ -38,6 +38,8 @@
> > #define BATCH_SIZE 8
> > #define POLL_TMOUT 1000
> > #define DEFAULT_PKT_CNT (4 * 1024)
> > +#define DEFAULT_UMEM_BUFFERS (DEFAULT_PKT_CNT / 4)
> > +#define UMEM_SIZE (DEFAULT_UMEM_BUFFERS * XSK_UMEM__DEFAULT_FRAME_SIZE)
> > #define RX_FULL_RXQSIZE 32
> > #define DEFAULT_OFFSET 256
> > #define XSK_UMEM__INVALID_FRAME_SIZE (XSK_UMEM__DEFAULT_FRAME_SIZE + 1)
> > @@ -51,9 +53,11 @@ enum test_mode {
> > };
> >
> > enum test_type {
> > - TEST_TYPE_NOPOLL,
> > + TEST_TYPE_RUN_TO_COMPLETION,
> > TEST_TYPE_POLL,
> > TEST_TYPE_UNALIGNED,
> > + TEST_TYPE_ALIGNED_INV_DESC,
> > + TEST_TYPE_UNALIGNED_INV_DESC,
> > TEST_TYPE_TEARDOWN,
> > TEST_TYPE_BIDI,
> > TEST_TYPE_STATS,
> > @@ -104,6 +108,7 @@ struct pkt {
> >
> > struct pkt_stream {
> > u32 nb_pkts;
> > + u32 rx_pkt_nb;
> > struct pkt *pkts;
> > bool use_addr_for_fill;
> > };
> > --
> > 2.29.0
> >
Powered by blists - more mailing lists