[PATCH bpf-next 0/4] Formatting and typo fixes in kernel/bpf

[PATCH bpf-next 0/4] Formatting and typo fixes in kernel/bpf

[Andrii Nakryiko]

Few formatting fixes, as well as some comments clean up.

Andrii Nakryiko (4):
  bpf: formatting fixes in syscall.c
  bpf/verifier: fix code formatting
  bpf/verifier: typo fixes and reflow verifier overview comment
  bpf/verifier: fix minor typos in comments

 kernel/bpf/syscall.c  |   5 +-
 kernel/bpf/verifier.c | 169 +++++++++++++++++++++---------------------
 2 files changed, 87 insertions(+), 87 deletions(-)

-- 
2.17.1

[PATCH bpf-next 1/4] bpf: formatting fixes in syscall.c

[Andrii Nakryiko]

Fix few formatting errors.

Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
 kernel/bpf/syscall.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 174581dfe225..5272ba491e00 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -214,6 +214,7 @@ static int bpf_map_init_memlock(struct bpf_map *map)
 static void bpf_map_release_memlock(struct bpf_map *map)
 {
 	struct user_struct *user = map->user;
+
 	bpf_uncharge_memlock(user, map->pages);
 	free_uid(user);
 }
@@ -299,7 +300,7 @@ static void bpf_map_put_uref(struct bpf_map *map)
 }
 
 /* decrement map refcnt and schedule it for freeing via workqueue
- * (unrelying map implementation ops->map_free() might sleep)
+ * (underlying map implementation ops->map_free() might sleep)
  */
 static void __bpf_map_put(struct bpf_map *map, bool do_idr_lock)
 {
@@ -1414,7 +1415,7 @@ struct bpf_prog *bpf_prog_inc_not_zero(struct bpf_prog *prog)
 EXPORT_SYMBOL_GPL(bpf_prog_inc_not_zero);
 
 bool bpf_prog_get_ok(struct bpf_prog *prog,
-			    enum bpf_prog_type *attach_type, bool attach_drv)
+		     enum bpf_prog_type *attach_type, bool attach_drv)
 {
 	/* not an attachment, just a refcount inc, always allow */
 	if (!attach_type)
-- 
2.17.1

[PATCH bpf-next 2/4] bpf/verifier: fix code formatting

[Andrii Nakryiko]

Fix few formatting problems.

Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
 kernel/bpf/verifier.c | 66 +++++++++++++++++++++----------------------
 1 file changed, 33 insertions(+), 33 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 0e4edd7e3c5f..c09ec3222d13 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -389,7 +389,7 @@ static bool is_release_function(enum bpf_func_id func_id)
 static bool is_acquire_function(enum bpf_func_id func_id)
 {
 	return func_id == BPF_FUNC_sk_lookup_tcp ||
-		func_id == BPF_FUNC_sk_lookup_udp;
+	       func_id == BPF_FUNC_sk_lookup_udp;
 }
 
 /* string representation of 'enum bpf_reg_type' */
@@ -559,39 +559,39 @@ COPY_STATE_FN(reference, acquired_refs, refs, 1)
 COPY_STATE_FN(stack, allocated_stack, stack, BPF_REG_SIZE)
 #undef COPY_STATE_FN
 
-#define REALLOC_STATE_FN(NAME, COUNT, FIELD, SIZE)			\
-static int realloc_##NAME##_state(struct bpf_func_state *state, int size, \
-				  bool copy_old)			\
-{									\
-	u32 old_size = state->COUNT;					\
-	struct bpf_##NAME##_state *new_##FIELD;				\
-	int slot = size / SIZE;						\
-									\
-	if (size <= old_size || !size) {				\
-		if (copy_old)						\
-			return 0;					\
-		state->COUNT = slot * SIZE;				\
-		if (!size && old_size) {				\
-			kfree(state->FIELD);				\
-			state->FIELD = NULL;				\
-		}							\
-		return 0;						\
-	}								\
+#define REALLOC_STATE_FN(NAME, COUNT, FIELD, SIZE)			     \
+static int realloc_##NAME##_state(struct bpf_func_state *state, int size,    \
+				  bool copy_old)			     \
+{									     \
+	u32 old_size = state->COUNT;					     \
+	struct bpf_##NAME##_state *new_##FIELD;				     \
+	int slot = size / SIZE;						     \
+									     \
+	if (size <= old_size || !size) {				     \
+		if (copy_old)						     \
+			return 0;					     \
+		state->COUNT = slot * SIZE;				     \
+		if (!size && old_size) {				     \
+			kfree(state->FIELD);				     \
+			state->FIELD = NULL;				     \
+		}							     \
+		return 0;						     \
+	}								     \
 	new_##FIELD = kmalloc_array(slot, sizeof(struct bpf_##NAME##_state), \
-				    GFP_KERNEL);			\
-	if (!new_##FIELD)						\
-		return -ENOMEM;						\
-	if (copy_old) {							\
-		if (state->FIELD)					\
-			memcpy(new_##FIELD, state->FIELD,		\
-			       sizeof(*new_##FIELD) * (old_size / SIZE)); \
-		memset(new_##FIELD + old_size / SIZE, 0,		\
-		       sizeof(*new_##FIELD) * (size - old_size) / SIZE); \
-	}								\
-	state->COUNT = slot * SIZE;					\
-	kfree(state->FIELD);						\
-	state->FIELD = new_##FIELD;					\
-	return 0;							\
+				    GFP_KERNEL);			     \
+	if (!new_##FIELD)						     \
+		return -ENOMEM;						     \
+	if (copy_old) {							     \
+		if (state->FIELD)					     \
+			memcpy(new_##FIELD, state->FIELD,		     \
+			       sizeof(*new_##FIELD) * (old_size / SIZE));    \
+		memset(new_##FIELD + old_size / SIZE, 0,		     \
+		       sizeof(*new_##FIELD) * (size - old_size) / SIZE);     \
+	}								     \
+	state->COUNT = slot * SIZE;					     \
+	kfree(state->FIELD);						     \
+	state->FIELD = new_##FIELD;					     \
+	return 0;							     \
 }
 /* realloc_reference_state() */
 REALLOC_STATE_FN(reference, acquired_refs, refs, 1)
-- 
2.17.1

[PATCH bpf-next 3/4] bpf/verifier: typo fixes and reflow verifier overview comment

[Andrii Nakryiko]

Fix few typos and reflow text in long high-level verifier comment.

Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
 kernel/bpf/verifier.c | 94 +++++++++++++++++++++----------------------
 1 file changed, 47 insertions(+), 47 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index c09ec3222d13..c6911ede94af 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -39,9 +39,9 @@ static const struct bpf_verifier_ops * const bpf_verifier_ops[] = {
 #undef BPF_MAP_TYPE
 };
 
-/* bpf_check() is a static code analyzer that walks eBPF program
- * instruction by instruction and updates register/stack state.
- * All paths of conditional branches are analyzed until 'bpf_exit' insn.
+/* bpf_check() is a static code analyzer that walks eBPF program instruction by
+ * instruction and updates register/stack state. All paths of conditional
+ * branches are analyzed until 'bpf_exit' insn.
  *
  * The first pass is depth-first-search to check that the program is a DAG.
  * It rejects the following programs:
@@ -49,15 +49,15 @@ static const struct bpf_verifier_ops * const bpf_verifier_ops[] = {
  * - if loop is present (detected via back-edge)
  * - unreachable insns exist (shouldn't be a forest. program = one function)
  * - out of bounds or malformed jumps
- * The second pass is all possible path descent from the 1st insn.
- * Since it's analyzing all pathes through the program, the length of the
- * analysis is limited to 64k insn, which may be hit even if total number of
- * insn is less then 4K, but there are too many branches that change stack/regs.
- * Number of 'branches to be analyzed' is limited to 1k
+ * The second pass is all possible path descent from the 1st insn. Since it's
+ * analyzing all pathes through the program, the length of the analysis is
+ * limited to 64k insn, which may be hit even if total number of insn is less
+ * than 4K, but there are too many branches that change stack/regs. Number of
+ * 'branches to be analyzed' is limited to 1k.
  *
  * On entry to each instruction, each register has a type, and the instruction
- * changes the types of the registers depending on instruction semantics.
- * If instruction is BPF_MOV64_REG(BPF_REG_1, BPF_REG_5), then type of R5 is
+ * changes the types of the registers depending on instruction semantics. If
+ * instruction is BPF_MOV64_REG(BPF_REG_1, BPF_REG_5), then type of R5 is
  * copied to R1.
  *
  * All registers are 64-bit.
@@ -66,37 +66,36 @@ static const struct bpf_verifier_ops * const bpf_verifier_ops[] = {
  * R6-R9 callee saved registers
  * R10 - frame pointer read-only
  *
- * At the start of BPF program the register R1 contains a pointer to bpf_context
- * and has type PTR_TO_CTX.
+ * At the start of BPF program the register R1 contains a pointer to
+ * bpf_context and has type PTR_TO_CTX.
  *
  * Verifier tracks arithmetic operations on pointers in case:
  *    BPF_MOV64_REG(BPF_REG_1, BPF_REG_10),
  *    BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, -20),
- * 1st insn copies R10 (which has FRAME_PTR) type into R1
- * and 2nd arithmetic instruction is pattern matched to recognize
- * that it wants to construct a pointer to some element within stack.
- * So after 2nd insn, the register R1 has type PTR_TO_STACK
- * (and -20 constant is saved for further stack bounds checking).
- * Meaning that this reg is a pointer to stack plus known immediate constant.
+ * 1st insn copies R10 (which has FRAME_PTR) type into R1 and 2nd arithmetic
+ * instruction is pattern matched to recognize that it wants to construct
+ * a pointer to some element within stack. So after 2nd insn, the register R1
+ * has type PTR_TO_STACK (and -20 constant is saved for further stack bounds
+ * checking). Meaning that this reg is a pointer to stack plus known immediate
+ * constant.
  *
- * Most of the time the registers have SCALAR_VALUE type, which
- * means the register has some value, but it's not a valid pointer.
- * (like pointer plus pointer becomes SCALAR_VALUE type)
+ * Most of the time the registers have SCALAR_VALUE type, which means the
+ * register has some value, but it's not a valid pointer (like pointer plus
+ * pointer becomes SCALAR_VALUE type).
  *
- * When verifier sees load or store instructions the type of base register
- * can be: PTR_TO_MAP_VALUE, PTR_TO_CTX, PTR_TO_STACK, PTR_TO_SOCKET. These are
+ * When verifier sees load or store instructions the type of base register can
+ * be: PTR_TO_MAP_VALUE, PTR_TO_CTX, PTR_TO_STACK, PTR_TO_SOCKET. These are
  * four pointer types recognized by check_mem_access() function.
  *
  * PTR_TO_MAP_VALUE means that this register is pointing to 'map element value'
  * and the range of [ptr, ptr + map's value_size) is accessible.
  *
- * registers used to pass values to function calls are checked against
+ * Registers used to pass values to function calls are checked against
  * function argument constraints.
  *
- * ARG_PTR_TO_MAP_KEY is one of such argument constraints.
- * It means that the register type passed to this function must be
- * PTR_TO_STACK and it will be used inside the function as
- * 'pointer to map element key'
+ * ARG_PTR_TO_MAP_KEY is one of such argument constraints. It means that the
+ * register type passed to this function must be PTR_TO_STACK and it will be
+ * used inside the function as 'pointer to map element key'
  *
  * For example the argument constraints for bpf_map_lookup_elem():
  *   .ret_type = RET_PTR_TO_MAP_VALUE_OR_NULL,
@@ -105,8 +104,8 @@ static const struct bpf_verifier_ops * const bpf_verifier_ops[] = {
  *
  * ret_type says that this function returns 'pointer to map elem value or null'
  * function expects 1st argument to be a const pointer to 'struct bpf_map' and
- * 2nd argument should be a pointer to stack, which will be used inside
- * the helper function as a pointer to map element key.
+ * 2nd argument should be a pointer to stack, which will be used inside the
+ * helper function as a pointer to map element key.
  *
  * On the kernel side the helper function looks like:
  * u64 bpf_map_lookup_elem(u64 r1, u64 r2, u64 r3, u64 r4, u64 r5)
@@ -115,9 +114,9 @@ static const struct bpf_verifier_ops * const bpf_verifier_ops[] = {
  *    void *key = (void *) (unsigned long) r2;
  *    void *value;
  *
- *    here kernel can access 'key' and 'map' pointers safely, knowing that
- *    [key, key + map->key_size) bytes are valid and were initialized on
- *    the stack of eBPF program.
+ *    Here kernel can access 'key' and 'map' pointers safely, knowing that
+ *    [key, key + map->key_size) bytes are valid and were initialized on the
+ *    stack of eBPF program.
  * }
  *
  * Corresponding eBPF program may look like:
@@ -126,21 +125,21 @@ static const struct bpf_verifier_ops * const bpf_verifier_ops[] = {
  *    BPF_LD_MAP_FD(BPF_REG_1, map_fd),      // after this insn R1 type is CONST_PTR_TO_MAP
  *    BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_map_lookup_elem),
  * here verifier looks at prototype of map_lookup_elem() and sees:
- * .arg1_type == ARG_CONST_MAP_PTR and R1->type == CONST_PTR_TO_MAP, which is ok,
- * Now verifier knows that this map has key of R1->map_ptr->key_size bytes
+ * .arg1_type == ARG_CONST_MAP_PTR and R1->type == CONST_PTR_TO_MAP, which is
+ * ok. Now verifier knows that this map has key of R1->map_ptr->key_size bytes.
  *
- * Then .arg2_type == ARG_PTR_TO_MAP_KEY and R2->type == PTR_TO_STACK, ok so far,
- * Now verifier checks that [R2, R2 + map's key_size) are within stack limits
- * and were initialized prior to this call.
- * If it's ok, then verifier allows this BPF_CALL insn and looks at
- * .ret_type which is RET_PTR_TO_MAP_VALUE_OR_NULL, so it sets
- * R0->type = PTR_TO_MAP_VALUE_OR_NULL which means bpf_map_lookup_elem() function
- * returns ether pointer to map value or NULL.
+ * Then .arg2_type == ARG_PTR_TO_MAP_KEY and R2->type == PTR_TO_STACK, ok so
+ * far. Now verifier checks that [R2, R2 + map's key_size) are within stack
+ * limits and were initialized prior to this call. If it's ok, then verifier
+ * allows this BPF_CALL insn and looks at .ret_type which is
+ * RET_PTR_TO_MAP_VALUE_OR_NULL, so it sets R0->type = PTR_TO_MAP_VALUE_OR_NULL
+ * which means bpf_map_lookup_elem() function returns either pointer to a map
+ * value or NULL.
  *
  * When type PTR_TO_MAP_VALUE_OR_NULL passes through 'if (reg != 0) goto +off'
  * insn, the register holding that pointer in the true branch changes state to
- * PTR_TO_MAP_VALUE and the same register changes state to CONST_IMM in the false
- * branch. See check_cond_jmp_op().
+ * PTR_TO_MAP_VALUE and the same register changes state to CONST_IMM in the
+ * false branch. See check_cond_jmp_op().
  *
  * After the call R0 is set to return type of the function and registers R1-R5
  * are set to NOT_INIT to indicate that they are no longer readable.
@@ -148,10 +147,11 @@ static const struct bpf_verifier_ops * const bpf_verifier_ops[] = {
  * The following reference types represent a potential reference to a kernel
  * resource which, after first being allocated, must be checked and freed by
  * the BPF program:
- * - PTR_TO_SOCKET_OR_NULL, PTR_TO_SOCKET
+ * - PTR_TO_SOCKET_OR_NULL
+ * - PTR_TO_SOCKET
  *
- * When the verifier sees a helper call return a reference type, it allocates a
- * pointer id for the reference and stores it in the current function state.
+ * When the verifier sees a helper call return a reference type, it allocates
+ * a pointer id for the reference and stores it in the current function state.
  * Similar to the way that PTR_TO_MAP_VALUE_OR_NULL is converted into
  * PTR_TO_MAP_VALUE, PTR_TO_SOCKET_OR_NULL becomes PTR_TO_SOCKET when the type
  * passes through a NULL-check conditional. For the branch wherein the state is
-- 
2.17.1

[PATCH bpf-next 4/4] bpf/verifier: fix minor typos in comments

[Andrii Nakryiko]

Few more minor fixes in comments.

Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
 kernel/bpf/verifier.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index c6911ede94af..0ee788bfd462 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -258,7 +258,7 @@ void bpf_verifier_vlog(struct bpf_verifier_log *log, const char *fmt,
 		log->ubuf = NULL;
 }
 
-/* log_level controls verbosity level of eBPF verifier.
+/* env->log.level controls verbosity level of eBPF verifier.
  * bpf_verifier_log_write() is used to dump the verification trace to the log,
  * so the user can figure out what's wrong with the program
  */
@@ -617,7 +617,7 @@ static int realloc_func_state(struct bpf_func_state *state, int stack_size,
 
 /* Acquire a pointer id from the env and update the state->refs to include
  * this new pointer reference.
- * On success, returns a valid pointer id to associate with the register
+ * On success, returns a valid pointer id to associate with the register.
  * On failure, returns a negative errno.
  */
 static int acquire_reference_state(struct bpf_verifier_env *env, int insn_idx)
@@ -714,7 +714,7 @@ static int copy_verifier_state(struct bpf_verifier_state *dst_state,
 	struct bpf_func_state *dst;
 	int i, err;
 
-	/* if dst has more stack frames then src frame, free them */
+	/* if dst has more stack frames than src frame, free them */
 	for (i = src->curframe + 1; i <= dst_state->curframe; i++) {
 		free_func_state(dst_state->frame[i]);
 		dst_state->frame[i] = NULL;
@@ -863,8 +863,7 @@ static bool reg_is_init_pkt_pointer(const struct bpf_reg_state *reg,
 				    enum bpf_reg_type which)
 {
 	/* The register can already have a range from prior markings.
-	 * This is fine as long as it hasn't been advanced from its
-	 * origin.
+	 * This is fine as long as it hasn't been advanced from its origin.
 	 */
 	return reg->type == which &&
 	       reg->id == 0 &&
-- 
2.17.1

Re: [PATCH bpf-next 0/4] Formatting and typo fixes in kernel/bpf

[Song Liu]

On Thu, Feb 28, 2019 at 6:08 PM Andrii Nakryiko <andriin@fb.com> wrote:
>
> Few formatting fixes, as well as some comments clean up.

For the series:

Acked-by: Song Liu <songliubraving@fb.com>

>
> Andrii Nakryiko (4):
>   bpf: formatting fixes in syscall.c
>   bpf/verifier: fix code formatting
>   bpf/verifier: typo fixes and reflow verifier overview comment
>   bpf/verifier: fix minor typos in comments
>
>  kernel/bpf/syscall.c  |   5 +-
>  kernel/bpf/verifier.c | 169 +++++++++++++++++++++---------------------
>  2 files changed, 87 insertions(+), 87 deletions(-)
>
> --
> 2.17.1
>

Re: [PATCH bpf-next 2/4] bpf/verifier: fix code formatting

[Alexei Starovoitov]

On 2/28/19 5:37 PM, Andrii Nakryiko wrote:
> -#define REALLOC_STATE_FN(NAME, COUNT, FIELD, SIZE)			\
> -static int realloc_##NAME##_state(struct bpf_func_state *state, int size, \
> -				  bool copy_old)			\
> -{									\
> -	u32 old_size = state->COUNT;					\
> -	struct bpf_##NAME##_state *new_##FIELD;				\
> -	int slot = size / SIZE;						\
> -									\
> -	if (size <= old_size || !size) {				\
> -		if (copy_old)						\
> -			return 0;					\
> -		state->COUNT = slot * SIZE;				\
> -		if (!size && old_size) {				\
> -			kfree(state->FIELD);				\
> -			state->FIELD = NULL;				\
> -		}							\
> -		return 0;						\
> -	}								\
> +#define REALLOC_STATE_FN(NAME, COUNT, FIELD, SIZE)			     \
> +static int realloc_##NAME##_state(struct bpf_func_state *state, int size,    \
> +				  bool copy_old)			     \
> +{									     \
> +	u32 old_size = state->COUNT;					     \

what is the change here?
extra tab at the end of every line?
I think that adds too much noise to git history.
I'd rather keep this bit as-is.

Re: [PATCH bpf-next 2/4] bpf/verifier: fix code formatting

[Andrii Nakryiko]

On Fri, Mar 1, 2019 at 11:24 AM Alexei Starovoitov <ast@fb.com> wrote:
>
> On 2/28/19 5:37 PM, Andrii Nakryiko wrote:
> > -#define REALLOC_STATE_FN(NAME, COUNT, FIELD, SIZE)                   \
> > -static int realloc_##NAME##_state(struct bpf_func_state *state, int size, \
> > -                               bool copy_old)                        \
> > -{                                                                    \
> > -     u32 old_size = state->COUNT;                                    \
> > -     struct bpf_##NAME##_state *new_##FIELD;                         \
> > -     int slot = size / SIZE;                                         \
> > -                                                                     \
> > -     if (size <= old_size || !size) {                                \
> > -             if (copy_old)                                           \
> > -                     return 0;                                       \
> > -             state->COUNT = slot * SIZE;                             \
> > -             if (!size && old_size) {                                \
> > -                     kfree(state->FIELD);                            \
> > -                     state->FIELD = NULL;                            \
> > -             }                                                       \
> > -             return 0;                                               \
> > -     }                                                               \
> > +#define REALLOC_STATE_FN(NAME, COUNT, FIELD, SIZE)                        \
> > +static int realloc_##NAME##_state(struct bpf_func_state *state, int size,    \
> > +                               bool copy_old)                             \
> > +{                                                                         \
> > +     u32 old_size = state->COUNT;                                         \
>
> what is the change here?
> extra tab at the end of every line?
> I think that adds too much noise to git history.
> I'd rather keep this bit as-is.
>
>

I feel like this whole patchset doesn't add much value and will just
pollute history. I'm just going to abandon it for now.