bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Fletcher Dunn <fletcherd@valvesoftware.com>
To: 'Alexei Starovoitov' <ast@kernel.org>,
	'Daniel Borkmann' <daniel@iogearbox.net>
Cc: 'Martin KaFai Lau' <kafai@fb.com>,
	'Song Liu' <songliubraving@fb.com>, 'Yonghong Song' <yhs@fb.com>,
	'Andrii Nakryiko' <andriin@fb.com>,
	"'netdev@vger.kernel.org'" <netdev@vger.kernel.org>,
	"'bpf@vger.kernel.org'" <bpf@vger.kernel.org>,
	Brandon Gilmore <bgilmore@valvesoftware.com>,
	"Steven Noonan" <steven@valvesoftware.com>
Subject: [PATCH bpf-next] xsk: Init all ring members in xsk_umem__create and xsk_socket__create
Date: Fri, 27 Mar 2020 03:24:07 +0000	[thread overview]
Message-ID: <85f12913cde94b19bfcb598344701c38@valvesoftware.com> (raw)

Fix a sharp edge in xsk_umem__create and xsk_socket__create.  Almost all of
the members of the ring buffer structs are initialized, but the "cached_xxx"
variables are not all initialized.  The caller is required to zero them.
This is needlessly dangerous.  The results if you don't do it can be very bad.
For example, they can cause xsk_prod_nb_free and xsk_cons_nb_avail to return
values greater than the size of the queue.  xsk_ring_cons__peek can return an
index that does not refer to an item that has been queued.

I have confirmed that without this change, my program misbehaves unless I
memset the ring buffers to zero before calling the function.  Afterwards,
my program works without (or with) the memset.

Signed-off-by: Fletcher Dunn <fletcherd@valvesoftware.com>

---

diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c
index 9807903f121e..f7f4efb70a4c 100644
--- a/tools/lib/bpf/xsk.c
+++ b/tools/lib/bpf/xsk.c
@@ -280,7 +280,11 @@ int xsk_umem__create_v0_0_4(struct xsk_umem **umem_ptr, void *umem_area,
 	fill->consumer = map + off.fr.consumer;
 	fill->flags = map + off.fr.flags;
 	fill->ring = map + off.fr.desc;
-	fill->cached_cons = umem->config.fill_size;
+	fill->cached_prod = *fill->producer;
+	/* cached_cons is "size" bigger than the real consumer pointer
+	 * See xsk_prod_nb_free
+	 */
+	fill->cached_cons = *fill->consumer + umem->config.fill_size;
 
 	map = mmap(NULL, off.cr.desc + umem->config.comp_size * sizeof(__u64),
 		   PROT_READ | PROT_WRITE, MAP_SHARED | MAP_POPULATE, umem->fd,
@@ -297,6 +301,8 @@ int xsk_umem__create_v0_0_4(struct xsk_umem **umem_ptr, void *umem_area,
 	comp->consumer = map + off.cr.consumer;
 	comp->flags = map + off.cr.flags;
 	comp->ring = map + off.cr.desc;
+	comp->cached_prod = *comp->producer;
+	comp->cached_cons = *comp->consumer;
 
 	*umem_ptr = umem;
 	return 0;
@@ -672,6 +678,8 @@ int xsk_socket__create(struct xsk_socket **xsk_ptr, const char *ifname,
 		rx->consumer = rx_map + off.rx.consumer;
 		rx->flags = rx_map + off.rx.flags;
 		rx->ring = rx_map + off.rx.desc;
+		rx->cached_prod = *rx->producer;
+		rx->cached_cons = *rx->consumer;
 	}
 	xsk->rx = rx;
 
@@ -691,7 +699,11 @@ int xsk_socket__create(struct xsk_socket **xsk_ptr, const char *ifname,
 		tx->consumer = tx_map + off.tx.consumer;
 		tx->flags = tx_map + off.tx.flags;
 		tx->ring = tx_map + off.tx.desc;
-		tx->cached_cons = xsk->config.tx_size;
+		tx->cached_prod = *tx->producer;
+		/* cached_cons is r->size bigger than the real consumer pointer
+		 * See xsk_prod_nb_free
+		 */
+		tx->cached_cons = *tx->consumer + xsk->config.tx_size;
 	}
 	xsk->tx = tx;


             reply	other threads:[~2020-03-27  3:40 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-27  3:24 Fletcher Dunn [this message]
2020-03-28  9:18 ` [PATCH bpf-next] xsk: Init all ring members in xsk_umem__create and xsk_socket__create Magnus Karlsson
2020-03-28 17:14   ` Daniel Borkmann

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=85f12913cde94b19bfcb598344701c38@valvesoftware.com \
    --to=fletcherd@valvesoftware.com \
    --cc=andriin@fb.com \
    --cc=ast@kernel.org \
    --cc=bgilmore@valvesoftware.com \
    --cc=bpf@vger.kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=kafai@fb.com \
    --cc=netdev@vger.kernel.org \
    --cc=songliubraving@fb.com \
    --cc=steven@valvesoftware.com \
    --cc=yhs@fb.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).