[PATCH -next 0/2] Introduce memset_range() helper for wiping members

Xiu Jianfeng (2):
  string.h: Introduce memset_range() for wiping members
  bpf: use memset_range helper in __mark_reg_known

 include/linux/string.h | 20 ++++++++++++++++++++
 kernel/bpf/verifier.c  |  5 ++---
 lib/memcpy_kunit.c     | 12 ++++++++++++
 3 files changed, 34 insertions(+), 3 deletions(-)

-- 
2.17.1

Motivated by memset_after() and memset_startat(), introduce a new helper,
memset_range() that takes the target struct instance, the byte to write,
and two member names where zeroing should start and end.

Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com>
---
 include/linux/string.h | 20 ++++++++++++++++++++
 lib/memcpy_kunit.c     | 12 ++++++++++++
 2 files changed, 32 insertions(+)

diff --git a/include/linux/string.h b/include/linux/string.h
index b6572aeca2f5..7f19863253f2 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h

@@ -291,6 +291,26 @@ void memcpy_and_pad(void *dest, size_t dest_len, const void *src, size_t count,
 	       sizeof(*(obj)) - offsetof(typeof(*(obj)), member));	\
 })
 
+/**
+ * memset_range - Set a value ranging from member1 to member2, boundary included.
+ *
+ * @obj: Address of target struct instance
+ * @v: Byte value to repeatedly write
+ * @member1: struct member to start writing at
+ * @member2: struct member where writing should stop
+ *
+ */
+#define memset_range(obj, v, member_1, member_2)			\
+({									\
+	u8 *__ptr = (u8 *)(obj);					\
+	typeof(v) __val = (v);						\
+	BUILD_BUG_ON(offsetof(typeof(*(obj)), member_1) >		\
+		     offsetof(typeof(*(obj)), member_2));		\
+	memset(__ptr + offsetof(typeof(*(obj)), member_1), __val,	\
+	       offsetofend(typeof(*(obj)), member_2) -			\
+	       offsetof(typeof(*(obj)), member_1));			\
+})
+
 /**
  * str_has_prefix - Test if a string has a given prefix
  * @str: The string to test

diff --git a/lib/memcpy_kunit.c b/lib/memcpy_kunit.c
index 62f8ffcbbaa3..0dd800412455 100644
--- a/lib/memcpy_kunit.c
+++ b/lib/memcpy_kunit.c

@@ -229,6 +229,13 @@ static void memset_test(struct kunit *test)
 			  0x79, 0x79, 0x79, 0x79, 0x79, 0x79, 0x79, 0x79,
 			},
 	};
+	struct some_bytes range = {
+		.data = { 0x30, 0x30, 0x30, 0x30, 0x81, 0x81, 0x81, 0x30,
+			  0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
+			  0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
+			  0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
+			},
+	};
 	struct some_bytes dest = { };
 	int count, value;
 	u8 *ptr;

@@ -269,6 +276,11 @@ static void memset_test(struct kunit *test)
 	dest = control;
 	memset_startat(&dest, 0x79, four);
 	compare("memset_startat()", dest, startat);
+
+	/* Verify memset_range() */
+	dest = control;
+	memset_range(&dest, 0x81, two, three);
+	compare("memset_range()", dest, range);
 #undef TEST_OP
 }
 

-- 
2.17.1

Replace the open-coded memset with memset_range helper to simplify
the code, there is no functional change in this patch.

Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com>
---
 kernel/bpf/verifier.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 7a20e12f2e45..317f259c0103 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c

@@ -1099,9 +1099,8 @@ static void ___mark_reg_known(struct bpf_reg_state *reg, u64 imm)
  */
 static void __mark_reg_known(struct bpf_reg_state *reg, u64 imm)
 {
-	/* Clear id, off, and union(map_ptr, range) */
-	memset(((u8 *)reg) + sizeof(reg->type), 0,
-	       offsetof(struct bpf_reg_state, var_off) - sizeof(reg->type));
+	/* Clear off, union(map_ptr, range), id, and ref_obj_id */
+	memset_range(reg, 0, off, ref_obj_id);
 	___mark_reg_known(reg, imm);
 }
 

-- 
2.17.1

On Wed, 8 Dec 2021 11:04:50 +0800 Xiu Jianfeng [off-list ref] wrote:

Motivated by memset_after() and memset_startat(), introduce a new helper,
memset_range() that takes the target struct instance, the byte to write,
and two member names where zeroing should start and end.

Is this likely to have more than a single call site?

I'm not sure what "boundary included" means.

Perhaps "struct member before which writing should stop"?

struct a {
	int b;
	int c;
	int d;
};

How do I zero out `c' and `d'?

On Wed, Dec 08, 2021 at 11:04:49AM +0800, Xiu Jianfeng wrote:

Xiu Jianfeng (2):
  string.h: Introduce memset_range() for wiping members

For doing a memset range, the preferred method is to use
a struct_group in the structure itself. This makes the range
self-documenting, and allows the compile to validate the exact size,
makes it addressable, etc. The other memset helpers are for "everything
to the end", which doesn't usually benefit from the struct_group style
of range declaration.

  bpf: use memset_range helper in __mark_reg_known

I never saw this patch arrive on the list?

-- 
Kees Cook

在 2021/12/8 12:28, Andrew Morton 写道:

On Wed, 8 Dec 2021 11:04:50 +0800 Xiu Jianfeng [off-list ref] wrote:

quoted

Motivated by memset_after() and memset_startat(), introduce a new helper,
memset_range() that takes the target struct instance, the byte to write,
and two member names where zeroing should start and end.

Is this likely to have more than a single call site?

There maybe more call site for this function, but I just use bpf as an example.

quoted

...

--- a/include/linux/string.h
+++ b/include/linux/string.h

@@ -291,6 +291,26 @@ void memcpy_and_pad(void *dest, size_t dest_len, const void *src, size_t count,
  	       sizeof(*(obj)) - offsetof(typeof(*(obj)), member));	\
  })
  
+/**
+ * memset_range - Set a value ranging from member1 to member2, boundary included.

I'm not sure what "boundary included" means.

I mean zeroing from member1 to member2(including position indicated by member1 and member2)

quoted

+ *
+ * @obj: Address of target struct instance
+ * @v: Byte value to repeatedly write
+ * @member1: struct member to start writing at
+ * @member2: struct member where writing should stop

Perhaps "struct member before which writing should stop"?

memset_range should include position indicated by member2 as well

quoted

+ *
+ */
+#define memset_range(obj, v, member_1, member_2)			\
+({									\
+	u8 *__ptr = (u8 *)(obj);					\
+	typeof(v) __val = (v);						\
+	BUILD_BUG_ON(offsetof(typeof(*(obj)), member_1) >		\
+		     offsetof(typeof(*(obj)), member_2));		\
+	memset(__ptr + offsetof(typeof(*(obj)), member_1), __val,	\
+	       offsetofend(typeof(*(obj)), member_2) -			\
+	       offsetof(typeof(*(obj)), member_1));			\
+})

struct a {
	int b;
	int c;
	int d;
};

How do I zero out `c' and `d'?

if you want to zero out 'c' and 'd', you can use it like memset_range(a_ptr, c, d);

.

在 2021/12/8 13:27, Kees Cook 写道:

On Wed, Dec 08, 2021 at 11:04:49AM +0800, Xiu Jianfeng wrote:

quoted

Xiu Jianfeng (2):
   string.h: Introduce memset_range() for wiping members

For doing a memset range, the preferred method is to use
a struct_group in the structure itself. This makes the range
self-documenting, and allows the compile to validate the exact size,
makes it addressable, etc. The other memset helpers are for "everything
to the end", which doesn't usually benefit from the struct_group style
of range declaration.

Do you mean there is no need to introduce this helper,  but to use struct_group in the struct directly?

quoted

   bpf: use memset_range helper in __mark_reg_known

I never saw this patch arrive on the list?

I have send this patch as well, can you please check again?

On Wed, Dec 08, 2021 at 11:04:50AM +0800, Xiu Jianfeng wrote:

"u8*" should be "void*" as kernel legitimises pointer arithmetic on void*
and there is no dereference.

__val is redundant, just toss "v" into memset(), it will do the right
thing. In fact, toss "__ptr" as well, it is simply unnecessary.

All previous memsets are the same...

On Wed, 8 Dec 2021 18:30:26 +0800 xiujianfeng [off-list ref] wrote:

在 2021/12/8 12:28, Andrew Morton 写道:

quoted

On Wed, 8 Dec 2021 11:04:50 +0800 Xiu Jianfeng [off-list ref] wrote:

quoted

Motivated by memset_after() and memset_startat(), introduce a new helper,
memset_range() that takes the target struct instance, the byte to write,
and two member names where zeroing should start and end.

Is this likely to have more than a single call site?

There maybe more call site for this function, but I just use bpf as an 
example.

quoted

quoted

...

--- a/include/linux/string.h
+++ b/include/linux/string.h

@@ -291,6 +291,26 @@ void memcpy_and_pad(void *dest, size_t dest_len, const void *src, size_t count,
  	       sizeof(*(obj)) - offsetof(typeof(*(obj)), member));	\
  })
  
+/**
+ * memset_range - Set a value ranging from member1 to member2, boundary included.

I'm not sure what "boundary included" means.

I mean zeroing from member1 to member2(including position indicated by 
member1 and member2)

quoted

quoted

+ *
+ * @obj: Address of target struct instance
+ * @v: Byte value to repeatedly write
+ * @member1: struct member to start writing at
+ * @member2: struct member where writing should stop

Perhaps "struct member before which writing should stop"?

memset_range should include position indicated by member2 as well

In that case we could say "struct member where writing should stop
(inclusive)", to make it very clear.

quoted

quoted

+ *
+ */
+#define memset_range(obj, v, member_1, member_2)			\
+({									\
+	u8 *__ptr = (u8 *)(obj);					\
+	typeof(v) __val = (v);						\
+	BUILD_BUG_ON(offsetof(typeof(*(obj)), member_1) >		\
+		     offsetof(typeof(*(obj)), member_2));		\
+	memset(__ptr + offsetof(typeof(*(obj)), member_1), __val,	\
+	       offsetofend(typeof(*(obj)), member_2) -			\
+	       offsetof(typeof(*(obj)), member_1));			\
+})

struct a {
	int b;
	int c;
	int d;
};

How do I zero out `c' and `d'?

if you want to zero out 'c' and 'd', you can use it like 
memset_range(a_ptr, c, d);

But I don't think that's what the code does!

it expands to

	memset(__ptr + offsetof(typeof(*(a)), c), __val,
	       offsetofend(typeof(*(a)), d) -
	       offsetof(typeof(*(a)), c));

which expands to

	memset(__ptr + 4, __val,
	       8 -
	       4);

and `d' will not be written to.

On Wed, Dec 08, 2021 at 03:44:37PM -0800, Andrew Morton wrote:

On Wed, 8 Dec 2021 18:30:26 +0800 xiujianfeng [off-list ref] wrote:

quoted

在 2021/12/8 12:28, Andrew Morton 写道:

quoted

On Wed, 8 Dec 2021 11:04:50 +0800 Xiu Jianfeng [off-list ref] wrote:

quoted

Motivated by memset_after() and memset_startat(), introduce a new helper,
memset_range() that takes the target struct instance, the byte to write,
and two member names where zeroing should start and end.

Is this likely to have more than a single call site?

There maybe more call site for this function, but I just use bpf as an 
example.

quoted

quoted

...

--- a/include/linux/string.h
+++ b/include/linux/string.h

@@ -291,6 +291,26 @@ void memcpy_and_pad(void *dest, size_t dest_len, const void *src, size_t count,
  	       sizeof(*(obj)) - offsetof(typeof(*(obj)), member));	\
  })
  
+/**
+ * memset_range - Set a value ranging from member1 to member2, boundary included.

I'm not sure what "boundary included" means.

I mean zeroing from member1 to member2(including position indicated by 
member1 and member2)

quoted

quoted

+ *
+ * @obj: Address of target struct instance
+ * @v: Byte value to repeatedly write
+ * @member1: struct member to start writing at
+ * @member2: struct member where writing should stop

Perhaps "struct member before which writing should stop"?

memset_range should include position indicated by member2 as well

In that case we could say "struct member where writing should stop
(inclusive)", to make it very clear.

quoted

quoted

quoted

+ *
+ */
+#define memset_range(obj, v, member_1, member_2)			\
+({									\
+	u8 *__ptr = (u8 *)(obj);					\
+	typeof(v) __val = (v);						\
+	BUILD_BUG_ON(offsetof(typeof(*(obj)), member_1) >		\
+		     offsetof(typeof(*(obj)), member_2));		\
+	memset(__ptr + offsetof(typeof(*(obj)), member_1), __val,	\
+	       offsetofend(typeof(*(obj)), member_2) -			\
+	       offsetof(typeof(*(obj)), member_1));			\
+})

struct a {
	int b;
	int c;
	int d;
};

How do I zero out `c' and `d'?

if you want to zero out 'c' and 'd', you can use it like 
memset_range(a_ptr, c, d);

But I don't think that's what the code does!

it expands to

	memset(__ptr + offsetof(typeof(*(a)), c), __val,
	       offsetofend(typeof(*(a)), d) -
	       offsetof(typeof(*(a)), c));

which expands to

	memset(__ptr + 4, __val,
	       8 -
	       4);

and `d' will not be written to.

Please don't add memset_range(): just use a struct_group() to capture
the range and use memset() against the new substruct. This will allow
for the range to be documented where it is defined in the struct (rather
than deep in some code), keep any changes centralized instead of spread
around in memset_range() calls, protect against accidental struct member
reordering breaking things, and lets the compiler be able to examine
the range explicitly and do all the correct bounds checking:

struct a {
	int b;
	struct_group(range,
		int c;
		int d;
	);
	int e;
};

memset(&instance->range, 0, sizeof(instance->range));

memset_from/after() were added because of the very common case of "wipe
from here to end", which stays tied to a single member, and addressed
cases where struct_group() couldn't help (e.g. trailing padding).

-- 
Kees Cook

在 2021/12/9 7:44, Andrew Morton 写道:

On Wed, 8 Dec 2021 18:30:26 +0800 xiujianfeng [off-list ref] wrote:

quoted

在 2021/12/8 12:28, Andrew Morton 写道:

quoted

On Wed, 8 Dec 2021 11:04:50 +0800 Xiu Jianfeng [off-list ref] wrote:

quoted

Motivated by memset_after() and memset_startat(), introduce a new helper,
memset_range() that takes the target struct instance, the byte to write,
and two member names where zeroing should start and end.

Is this likely to have more than a single call site?

There maybe more call site for this function, but I just use bpf as an
example.

quoted

quoted

...

--- a/include/linux/string.h
+++ b/include/linux/string.h

@@ -291,6 +291,26 @@ void memcpy_and_pad(void *dest, size_t dest_len, const void *src, size_t count,
   	       sizeof(*(obj)) - offsetof(typeof(*(obj)), member));	\
   })
   
+/**
+ * memset_range - Set a value ranging from member1 to member2, boundary included.

I'm not sure what "boundary included" means.

I mean zeroing from member1 to member2(including position indicated by
member1 and member2)

quoted

quoted

+ *
+ * @obj: Address of target struct instance
+ * @v: Byte value to repeatedly write
+ * @member1: struct member to start writing at
+ * @member2: struct member where writing should stop

Perhaps "struct member before which writing should stop"?

memset_range should include position indicated by member2 as well

In that case we could say "struct member where writing should stop
(inclusive)", to make it very clear.

that is good, thank you for you advice :)

quoted

quoted

quoted

+ *
+ */
+#define memset_range(obj, v, member_1, member_2)			\
+({									\
+	u8 *__ptr = (u8 *)(obj);					\
+	typeof(v) __val = (v);						\
+	BUILD_BUG_ON(offsetof(typeof(*(obj)), member_1) >		\
+		     offsetof(typeof(*(obj)), member_2));		\
+	memset(__ptr + offsetof(typeof(*(obj)), member_1), __val,	\
+	       offsetofend(typeof(*(obj)), member_2) -			\
+	       offsetof(typeof(*(obj)), member_1));			\
+})

struct a {
	int b;
	int c;
	int d;
};

How do I zero out `c' and `d'?

if you want to zero out 'c' and 'd', you can use it like
memset_range(a_ptr, c, d);

But I don't think that's what the code does!

it expands to

	memset(__ptr + offsetof(typeof(*(a)), c), __val,
	       offsetofend(typeof(*(a)), d) -
	       offsetof(typeof(*(a)), c));

which expands to

	memset(__ptr + 4, __val,
	       8 -
	       4);

and `d' will not be written to.

#define offsetofend(TYPE, MEMBER) \
             (offsetof(TYPE, MEMBER)>+ sizeof_field(TYPE, MEMBER))

if I understand correctly, offsetofend(typeof(*(a), d) is 12, so it expands to

    memset(__ptr + 4, __val,

                  12 -

                  4);

Anyway,  I will drop this patch because of Kees's suggestion, thank you.

.

在 2021/12/9 13:17, Kees Cook 写道:

On Wed, Dec 08, 2021 at 03:44:37PM -0800, Andrew Morton wrote:

quoted

On Wed, 8 Dec 2021 18:30:26 +0800 xiujianfeng [off-list ref] wrote:

quoted

在 2021/12/8 12:28, Andrew Morton 写道:

quoted

On Wed, 8 Dec 2021 11:04:50 +0800 Xiu Jianfeng [off-list ref] wrote:

quoted

Motivated by memset_after() and memset_startat(), introduce a new helper,
memset_range() that takes the target struct instance, the byte to write,
and two member names where zeroing should start and end.

Is this likely to have more than a single call site?

There maybe more call site for this function, but I just use bpf as an
example.

quoted

quoted

...

--- a/include/linux/string.h
+++ b/include/linux/string.h

@@ -291,6 +291,26 @@ void memcpy_and_pad(void *dest, size_t dest_len, const void *src, size_t count,
   	       sizeof(*(obj)) - offsetof(typeof(*(obj)), member));	\
   })
   
+/**
+ * memset_range - Set a value ranging from member1 to member2, boundary included.

I'm not sure what "boundary included" means.

I mean zeroing from member1 to member2(including position indicated by
member1 and member2)

quoted

quoted

+ *
+ * @obj: Address of target struct instance
+ * @v: Byte value to repeatedly write
+ * @member1: struct member to start writing at
+ * @member2: struct member where writing should stop

Perhaps "struct member before which writing should stop"?

memset_range should include position indicated by member2 as well

In that case we could say "struct member where writing should stop
(inclusive)", to make it very clear.

quoted

quoted

quoted

+ *
+ */
+#define memset_range(obj, v, member_1, member_2)			\
+({									\
+	u8 *__ptr = (u8 *)(obj);					\
+	typeof(v) __val = (v);						\
+	BUILD_BUG_ON(offsetof(typeof(*(obj)), member_1) >		\
+		     offsetof(typeof(*(obj)), member_2));		\
+	memset(__ptr + offsetof(typeof(*(obj)), member_1), __val,	\
+	       offsetofend(typeof(*(obj)), member_2) -			\
+	       offsetof(typeof(*(obj)), member_1));			\
+})

struct a {
	int b;
	int c;
	int d;
};

How do I zero out `c' and `d'?

if you want to zero out 'c' and 'd', you can use it like
memset_range(a_ptr, c, d);

But I don't think that's what the code does!

it expands to

	memset(__ptr + offsetof(typeof(*(a)), c), __val,
	       offsetofend(typeof(*(a)), d) -
	       offsetof(typeof(*(a)), c));

which expands to

	memset(__ptr + 4, __val,
	       8 -
	       4);

and `d' will not be written to.

Please don't add memset_range(): just use a struct_group() to capture
the range and use memset() against the new substruct. This will allow
for the range to be documented where it is defined in the struct (rather
than deep in some code), keep any changes centralized instead of spread
around in memset_range() calls, protect against accidental struct member
reordering breaking things, and lets the compiler be able to examine
the range explicitly and do all the correct bounds checking:

struct a {
	int b;
	struct_group(range,
		int c;
		int d;
	);
	int e;
};

memset(&instance->range, 0, sizeof(instance->range));

memset_from/after() were added because of the very common case of "wipe
from here to end", which stays tied to a single member, and addressed
cases where struct_group() couldn't help (e.g. trailing padding).

got it, thank you, I will drop this patch.