Skip to contentSkip to conversation

bpf: add sk_msg helper sk_msg_pop_data

PATCH Applied John Fastabend 3 participants 6 messages Nov 27, 2018 – Nov 29, 2018

Subsystems (9)
Networking Network drivers Kselftest BPF +5 more subsystems
lore
Share & tools
lore

John Fastabend

author maintainer
#0 lore raw

v2 cover bpf: add sk_msg helper sk_msg_pop_data

After being able to add metadata to messages with sk_msg_push_data we have also found it useful to be able to "pop" this metadata off before sending it to applications in some cases. This series adds a new helper sk_msg_pop_data() and the associated patches to add tests and tools/lib support.

Thanks!

v2: Daniel caught that we missed adding sk_msg_pop_data to the changes
data helper so that the verifier ensures BPF programs revalidate data after using this helper. Also improve documentation adding a return description and using RST syntax per Quentin's comment. And delta calculations for DROP with pop'd data (albeit a strange set of operations for a program to be doing) had potential to be incorrect possibly confusing user space applications, so fix it.

John Fastabend (3):
bpf: helper to pop data from messages
bpf: add msg_pop_data helper to tools
bpf: test_sockmap, add options for msg_pop_data() helper usage

 include/uapi/linux/bpf.h                        |  13 +-
 net/core/filter.c                               | 169 ++++++++++++++++++++++++
 net/ipv4/tcp_bpf.c                              |  14 +-
 tools/include/uapi/linux/bpf.h                  |  13 +-
 tools/testing/selftests/bpf/bpf_helpers.h       |   2 +
 tools/testing/selftests/bpf/test_sockmap.c      | 127 +++++++++++++++++-
 tools/testing/selftests/bpf/test_sockmap_kern.h |  70 ++++++++--
 7 files changed, 386 insertions(+), 22 deletions(-)
Signature · 1 line
2.7.4
author maintainer
#1 lore raw

PATCH v2 1/3 bpf: helper to pop data from messages

This adds a BPF SK_MSG program helper so that we can pop data from a msg. We use this to pop metadata from a previous push data call.

Signed-off-by John Fastabend <[email protected]>
---
 include/uapi/linux/bpf.h |  16 ++++-
 net/core/filter.c        | 171 +++++++++++++++++++++++++++++++++++++++++++++++
 net/ipv4/tcp_bpf.c       |  17 ++++-
 net/tls/tls_sw.c         |  11 ++-
 4 files changed, 209 insertions(+), 6 deletions(-)
include/uapi/linux/bpf.h +15 -1
index 23e2031..597afdb 100644
@@ -2268,6 +2268,19 @@ union bpf_attr {
  *
  *	Return
  *		0 on success, or a negative error in case of failure.
+ *
+ * int bpf_msg_pop_data(struct sk_msg_buff *msg, u32 start, u32 pop, u64 flags)
+ *	 Description
+ *		Will remove *pop* bytes from a *msg* starting at byte *start*.
+ *		This may result in **ENOMEM** errors under certain situations if
+ *		an allocation and copy are required due to a full ring buffer.
+ *		However, the helper will try to avoid doing the allocation
+ *		if possible. Other errors can occur if input parameters are
+ *		invalid either due to *start* byte not being valid part of msg
+ *		payload and/or *pop* value being to large.
+ *
+ *	Return
+ *		0 on success, or a negative erro in case of failure.
  */
 #define __BPF_FUNC_MAPPER(FN)		\
 	FN(unspec),			\

@@ -2360,7 +2373,8 @@ union bpf_attr {
 	FN(map_push_elem),		\
 	FN(map_pop_elem),		\
 	FN(map_peek_elem),		\
-	FN(msg_push_data),
+	FN(msg_push_data),		\
+	FN(msg_pop_data),
 
 /* integer value in 'imm' field of BPF_CALL instruction selects which helper
  * function eBPF program intends to call
net/core/filter.c +171 -0

189 lines not on this page · raw message

net/ipv4/tcp_bpf.c +14 -3
index 3b45fe5..a47c1cd 100644
@@ -289,12 +289,23 @@ static int tcp_bpf_send_verdict(struct sock *sk, struct sk_psock *psock,
 {
 	bool cork = false, enospc = msg->sg.start == msg->sg.end;
 	struct sock *sk_redir;
-	u32 tosend;
+	u32 tosend, delta = 0;
 	int ret;
 
 more_data:
-	if (psock->eval == __SK_NONE)
+	if (psock->eval == __SK_NONE) {
+		/* Track delta in msg size to add/subtract it on SK_DROP from
+		 * returned to user copied size. This ensures user doesn't
+		 * get a positive return code with msg_cut_data and SK_DROP
+		 * verdict.
+		 */
+		delta = msg->sg.size;
 		psock->eval = sk_psock_msg_verdict(sk, psock, msg);
+		if (msg->sg.size < delta)
+			delta -= msg->sg.size;
+		else
+			delta = 0;
+	}
 
 	if (msg->cork_bytes &&
 	    msg->cork_bytes > msg->sg.size && !enospc) {

@@ -350,7 +361,7 @@ static int tcp_bpf_send_verdict(struct sock *sk, struct sk_psock *psock,
 	default:
 		sk_msg_free_partial(sk, msg, tosend);
 		sk_msg_apply_bytes(psock, tosend);
-		*copied -= tosend;
+		*copied -= (tosend + delta);
 		return -EACCES;
 	}
net/tls/tls_sw.c +9 -2
index 7b1af8b..d4ecc66 100644
@@ -687,6 +687,7 @@ static int bpf_exec_tx_verdict(struct sk_msg *msg, struct sock *sk,
 	struct sock *sk_redir;
 	struct tls_rec *rec;
 	int err = 0, send;
+	u32 delta = 0;
 	bool enospc;
 
 	psock = sk_psock_get(sk);

@@ -694,8 +695,14 @@ static int bpf_exec_tx_verdict(struct sk_msg *msg, struct sock *sk,
 		return tls_push_record(sk, flags, record_type);
 more_data:
 	enospc = sk_msg_full(msg);
-	if (psock->eval == __SK_NONE)
+	if (psock->eval == __SK_NONE) {
+		delta = msg->sg.size;
 		psock->eval = sk_psock_msg_verdict(sk, psock, msg);
+		if (delta < msg->sg.size)
+			delta -= msg->sg.size;
+		else
+			delta = 0;
+	}
 	if (msg->cork_bytes && msg->cork_bytes > msg->sg.size &&
 	    !enospc && !full_record) {
 		err = -ENOSPC;

@@ -743,7 +750,7 @@ static int bpf_exec_tx_verdict(struct sk_msg *msg, struct sock *sk,
 			msg->apply_bytes -= send;
 		if (msg->sg.size == 0)
 			tls_free_open_rec(sk);
-		*copied -= send;
+		*copied -= (send + delta);
 		err = -EACCES;
 	}
Signature · 1 line
2.7.4
author maintainer
#2 lore raw

PATCH v2 2/3 bpf: add msg_pop_data helper to tools

In reply to John Fastabend

Add the necessary header definitions to tools for new msg_pop_data_helper.

Signed-off-by John Fastabend <[email protected]>
---
 tools/include/uapi/linux/bpf.h            | 16 +++++++++++++++-
 tools/testing/selftests/bpf/bpf_helpers.h |  2 ++
 2 files changed, 17 insertions(+), 1 deletion(-)
tools/include/uapi/linux/bpf.h +15 -1
index 23e2031..597afdb 100644
@@ -2268,6 +2268,19 @@ union bpf_attr {
  *
  *	Return
  *		0 on success, or a negative error in case of failure.
+ *
+ * int bpf_msg_pop_data(struct sk_msg_buff *msg, u32 start, u32 pop, u64 flags)
+ *	 Description
+ *		Will remove *pop* bytes from a *msg* starting at byte *start*.
+ *		This may result in **ENOMEM** errors under certain situations if
+ *		an allocation and copy are required due to a full ring buffer.
+ *		However, the helper will try to avoid doing the allocation
+ *		if possible. Other errors can occur if input parameters are
+ *		invalid either due to *start* byte not being valid part of msg
+ *		payload and/or *pop* value being to large.
+ *
+ *	Return
+ *		0 on success, or a negative erro in case of failure.
  */
 #define __BPF_FUNC_MAPPER(FN)		\
 	FN(unspec),			\

@@ -2360,7 +2373,8 @@ union bpf_attr {
 	FN(map_push_elem),		\
 	FN(map_pop_elem),		\
 	FN(map_peek_elem),		\
-	FN(msg_push_data),
+	FN(msg_push_data),		\
+	FN(msg_pop_data),
 
 /* integer value in 'imm' field of BPF_CALL instruction selects which helper
  * function eBPF program intends to call
tools/testing/selftests/bpf/bpf_helpers.h +2 -0
index 686e57c..7b69519 100644
@@ -113,6 +113,8 @@ static int (*bpf_msg_pull_data)(void *ctx, int start, int end, int flags) =
 	(void *) BPF_FUNC_msg_pull_data;
 static int (*bpf_msg_push_data)(void *ctx, int start, int end, int flags) =
 	(void *) BPF_FUNC_msg_push_data;
+static int (*bpf_msg_pop_data)(void *ctx, int start, int cut, int flags) =
+	(void *) BPF_FUNC_msg_pop_data;
 static int (*bpf_bind)(void *ctx, void *addr, int addr_len) =
 	(void *) BPF_FUNC_bind;
 static int (*bpf_xdp_adjust_tail)(void *ctx, int offset) =
Signature · 1 line
2.7.4
author maintainer
#3 lore raw

PATCH v2 3/3 bpf: test_sockmap, add options for msg_pop_data() helper

In reply to John Fastabend

Similar to msg_pull_data and msg_push_data add a set of options to have msg_pop_data() exercised.

Signed-off-by John Fastabend <[email protected]>
---
 tools/testing/selftests/bpf/test_sockmap.c      | 127 +++++++++++++++++++++++-
 tools/testing/selftests/bpf/test_sockmap_kern.h |  70 ++++++++++---
 2 files changed, 180 insertions(+), 17 deletions(-)
tools/testing/selftests/bpf/test_sockmap.c +123 -4
index 622ade0..e85a771 100644
@@ -79,6 +79,8 @@ int txmsg_start;
 int txmsg_end;
 int txmsg_start_push;
 int txmsg_end_push;
+int txmsg_start_pop;
+int txmsg_pop;
 int txmsg_ingress;
 int txmsg_skb;
 int ktls;

@@ -104,6 +106,8 @@ static const struct option long_options[] = {
 	{"txmsg_end",	required_argument,	NULL, 'e'},
 	{"txmsg_start_push", required_argument,	NULL, 'p'},
 	{"txmsg_end_push",   required_argument,	NULL, 'q'},
+	{"txmsg_start_pop",  required_argument,	NULL, 'w'},
+	{"txmsg_pop",	     required_argument,	NULL, 'x'},
 	{"txmsg_ingress", no_argument,		&txmsg_ingress, 1 },
 	{"txmsg_skb", no_argument,		&txmsg_skb, 1 },
 	{"ktls", no_argument,			&ktls, 1 },

@@ -473,13 +477,27 @@ static int msg_loop(int fd, int iov_count, int iov_length, int cnt,
 		clock_gettime(CLOCK_MONOTONIC, &s->end);
 	} else {
 		int slct, recvp = 0, recv, max_fd = fd;
+		float total_bytes, txmsg_pop_total;
 		int fd_flags = O_NONBLOCK;
 		struct timeval timeout;
-		float total_bytes;
 		fd_set w;
 
 		fcntl(fd, fd_flags);
+		/* Account for pop bytes noting each iteration of apply will
+		 * call msg_pop_data helper so we need to account for this
+		 * by calculating the number of apply iterations. Note user
+		 * of the tool can create cases where no data is sent by
+		 * manipulating pop/push/pull/etc. For example txmsg_apply 1
+		 * with txmsg_pop 1 will try to apply 1B at a time but each
+		 * iteration will then pop 1B so no data will ever be sent.
+		 * This is really only useful for testing edge cases in code
+		 * paths.
+		 */
 		total_bytes = (float)iov_count * (float)iov_length * (float)cnt;
+		txmsg_pop_total = txmsg_pop;
+		if (txmsg_apply)
+			txmsg_pop_total *= (total_bytes / txmsg_apply);
+		total_bytes -= txmsg_pop_total;
 		err = clock_gettime(CLOCK_MONOTONIC, &s->start);
 		if (err < 0)
 			perror("recv start time: ");

@@ -488,7 +506,7 @@ static int msg_loop(int fd, int iov_count, int iov_length, int cnt,
 				timeout.tv_sec = 0;
 				timeout.tv_usec = 300000;
 			} else {
-				timeout.tv_sec = 1;
+				timeout.tv_sec = 3;
 				timeout.tv_usec = 0;
 			}
 

@@ -503,7 +521,7 @@ static int msg_loop(int fd, int iov_count, int iov_length, int cnt,
 				goto out_errno;
 			} else if (!slct) {
 				if (opt->verbose)
-					fprintf(stderr, "unexpected timeout\n");
+					fprintf(stderr, "unexpected timeout: recved %zu/%f pop_total %f\n", s->bytes_recvd, total_bytes, txmsg_pop_total);
 				errno = -EIO;
 				clock_gettime(CLOCK_MONOTONIC, &s->end);
 				goto out_errno;

@@ -619,7 +637,7 @@ static int sendmsg_test(struct sockmap_options *opt)
 			iov_count = 1;
 		err = msg_loop(rx_fd, iov_count, iov_buf,
 			       cnt, &s, false, opt);
-		if (err && opt->verbose)
+		if (opt->verbose)
 			fprintf(stderr,
 				"msg_loop_rx: iov_count %i iov_buf %i cnt %i err %i\n",
 				iov_count, iov_buf, cnt, err);

@@ -931,6 +949,39 @@ static int run_options(struct sockmap_options *options, int cg_fd,  int test)
 			}
 		}
 
+		if (txmsg_start_pop) {
+			i = 4;
+			err = bpf_map_update_elem(map_fd[5],
+						  &i, &txmsg_start_pop, BPF_ANY);
+			if (err) {
+				fprintf(stderr,
+					"ERROR: bpf_map_update_elem %i@%i (txmsg_start_pop):  %d (%s)\n",
+					txmsg_start_pop, i, err, strerror(errno));
+				goto out;
+			}
+		} else {
+			i = 4;
+			bpf_map_update_elem(map_fd[5],
+						  &i, &txmsg_start_pop, BPF_ANY);
+		}
+
+		if (txmsg_pop) {
+			i = 5;
+			err = bpf_map_update_elem(map_fd[5],
+						  &i, &txmsg_pop, BPF_ANY);
+			if (err) {
+				fprintf(stderr,
+					"ERROR: bpf_map_update_elem %i@%i (txmsg_pop):  %d (%s)\n",
+					txmsg_pop, i, err, strerror(errno));
+				goto out;
+			}
+		} else {
+			i = 5;
+			bpf_map_update_elem(map_fd[5],
+					    &i, &txmsg_pop, BPF_ANY);
+
+		}
+
 		if (txmsg_ingress) {
 			int in = BPF_F_INGRESS;
 

@@ -1082,6 +1133,11 @@ static void test_options(char *options)
 		snprintf(tstr, OPTSTRING, "end %d,", txmsg_end);
 		strncat(options, tstr, OPTSTRING);
 	}
+	if (txmsg_start_pop) {
+		snprintf(tstr, OPTSTRING, "pop (%d,%d),",
+			 txmsg_start_pop, txmsg_start_pop + txmsg_pop);
+		strncat(options, tstr, OPTSTRING);
+	}
 	if (txmsg_ingress)
 		strncat(options, "ingress,", OPTSTRING);
 	if (txmsg_skb)

@@ -1264,6 +1320,7 @@ static int test_mixed(int cgrp)
 	txmsg_apply = txmsg_cork = 0;
 	txmsg_start = txmsg_end = 0;
 	txmsg_start_push = txmsg_end_push = 0;
+	txmsg_start_pop = txmsg_pop = 0;
 
 	/* Test small and large iov_count values with pass/redir/apply/cork */
 	txmsg_pass = 1;

@@ -1383,6 +1440,19 @@ static int test_start_end(int cgrp)
 	txmsg_end = 2;
 	txmsg_start_push = 1;
 	txmsg_end_push = 2;
+	txmsg_start_pop = 1;
+	txmsg_pop = 1;
+	err = test_txmsg(cgrp);
+	if (err)
+		goto out;
+
+	/* Cut a byte of pushed data but leave reamining in place */
+	txmsg_start = 1;
+	txmsg_end = 2;
+	txmsg_start_push = 1;
+	txmsg_end_push = 3;
+	txmsg_start_pop = 1;
+	txmsg_pop = 1;
 	err = test_txmsg(cgrp);
 	if (err)
 		goto out;

@@ -1393,6 +1463,9 @@ static int test_start_end(int cgrp)
 	opt.iov_length = 100;
 	txmsg_cork = 1600;
 
+	txmsg_start_pop = 0;
+	txmsg_pop = 0;
+
 	for (i = 99; i <= 1600; i += 500) {
 		txmsg_start = 0;
 		txmsg_end = i;

@@ -1403,6 +1476,17 @@ static int test_start_end(int cgrp)
 			goto out;
 	}
 
+	/* Test pop data in middle of cork */
+	for (i = 99; i <= 1600; i += 500) {
+		txmsg_start_pop = 10;
+		txmsg_pop = i;
+		err = test_exec(cgrp, &opt);
+		if (err)
+			goto out;
+	}
+	txmsg_start_pop = 0;
+	txmsg_pop = 0;
+
 	/* Test start/end with cork but pull data in middle */
 	for (i = 199; i <= 1600; i += 500) {
 		txmsg_start = 100;

@@ -1423,6 +1507,15 @@ static int test_start_end(int cgrp)
 	if (err)
 		goto out;
 
+	/* Test pop with cork pulling last sg entry */
+	txmsg_start_pop = 1500;
+	txmsg_pop = 1600;
+	err = test_exec(cgrp, &opt);
+	if (err)
+		goto out;
+	txmsg_start_pop = 0;
+	txmsg_pop = 0;
+
 	/* Test start/end pull of single byte in last page */
 	txmsg_start = 1111;
 	txmsg_end = 1112;

@@ -1432,6 +1525,13 @@ static int test_start_end(int cgrp)
 	if (err)
 		goto out;
 
+	/* Test pop of single byte in last page */
+	txmsg_start_pop = 1111;
+	txmsg_pop = 1112;
+	err = test_exec(cgrp, &opt);
+	if (err)
+		goto out;
+
 	/* Test start/end with end < start */
 	txmsg_start = 1111;
 	txmsg_end = 0;

@@ -1456,7 +1556,20 @@ static int test_start_end(int cgrp)
 	txmsg_start_push = 1601;
 	txmsg_end_push = 1600;
 	err = test_exec(cgrp, &opt);
+	if (err)
+		goto out;
+
+	/* Test pop with start > data */
+	txmsg_start_pop = 1601;
+	txmsg_pop = 1;
+	err = test_exec(cgrp, &opt);
+	if (err)
+		goto out;
 
+	/* Test pop with pop range > data */
+	txmsg_start_pop = 1599;
+	txmsg_pop = 10;
+	err = test_exec(cgrp, &opt);
 out:
 	txmsg_start = 0;
 	txmsg_end = 0;

@@ -1641,6 +1754,12 @@ int main(int argc, char **argv)
 		case 'q':
 			txmsg_end_push = atoi(optarg);
 			break;
+		case 'w':
+			txmsg_start_pop = atoi(optarg);
+			break;
+		case 'x':
+			txmsg_pop = atoi(optarg);
+			break;
 		case 'a':
 			txmsg_apply = atoi(optarg);
 			break;
tools/testing/selftests/bpf/test_sockmap_kern.h +57 -13

136 lines not on this page · raw message

Signature · 1 line
2.7.4
Acked-by on the v2 series
#4 lore raw
In reply to John Fastabend
Quoted 8 lines · John Fastabend

After being able to add metadata to messages with sk_msg_push_data we have also found it useful to be able to "pop" this metadata off before sending it to applications in some cases. This series adds a new helper sk_msg_pop_data() and the associated patches to add tests and tools/lib support.

Thanks!

Quoted 7 lines

v2: Daniel caught that we missed adding sk_msg_pop_data to the changes
data helper so that the verifier ensures BPF programs revalidate data after using this helper. Also improve documentation adding a return description and using RST syntax per Quentin's comment. And delta calculations for DROP with pop'd data (albeit a strange set of operations for a program to be doing) had potential to be incorrect possibly confusing user space applications, so fix it.

lgtm
for the set:

Acked-by Alexei Starovoitov <[email protected]>
maintainer Applied
#5 lore raw
In reply to John Fastabend
Quoted 21 lines · John Fastabend

After being able to add metadata to messages with sk_msg_push_data we have also found it useful to be able to "pop" this metadata off before sending it to applications in some cases. This series adds a new helper sk_msg_pop_data() and the associated patches to add tests and tools/lib support.

Thanks!

v2: Daniel caught that we missed adding sk_msg_pop_data to the changes
data helper so that the verifier ensures BPF programs revalidate data after using this helper. Also improve documentation adding a return description and using RST syntax per Quentin's comment. And delta calculations for DROP with pop'd data (albeit a strange set of operations for a program to be doing) had potential to be incorrect possibly confusing user space applications, so fix it.

John Fastabend (3):
bpf: helper to pop data from messages
bpf: add msg_pop_data helper to tools
bpf: test_sockmap, add options for msg_pop_data() helper usage

Quoted 9 lines
 include/uapi/linux/bpf.h                        |  13 +-
 net/core/filter.c                               | 169 ++++++++++++++++++++++++
 net/ipv4/tcp_bpf.c                              |  14 +-
 tools/include/uapi/linux/bpf.h                  |  13 +-
 tools/testing/selftests/bpf/bpf_helpers.h       |   2 +
 tools/testing/selftests/bpf/test_sockmap.c      | 127 +++++++++++++++++-
 tools/testing/selftests/bpf/test_sockmap_kern.h |  70 ++++++++--
 7 files changed, 386 insertions(+), 22 deletions(-)

Applied to bpf-next, thanks!