linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH -next 0/2] Introduce memset_range() helper for wiping members
@ 2021-12-08  3:04 Xiu Jianfeng
  2021-12-08  3:04 ` [PATCH -next 1/2] string.h: Introduce memset_range() " Xiu Jianfeng
                   ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Xiu Jianfeng @ 2021-12-08  3:04 UTC (permalink / raw)
  To: akpm, keescook, laniel_francis, andriy.shevchenko, adobriyan,
	linux, andreyknvl, dja, ast, daniel, andrii, kafai,
	songliubraving, yhs, john.fastabend, kpsingh
  Cc: linux-kernel, netdev, bpf

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH -next 1/2] string.h: Introduce memset_range() for wiping members
  2021-12-08  3:04 [PATCH -next 0/2] Introduce memset_range() helper for wiping members Xiu Jianfeng
@ 2021-12-08  3:04 ` Xiu Jianfeng
  2021-12-08  4:28   ` Andrew Morton
  2021-12-08 17:12   ` Alexey Dobriyan
  2021-12-08  3:04 ` [PATCH -next 2/2] bpf: use memset_range helper in __mark_reg_known Xiu Jianfeng
  2021-12-08  5:27 ` [PATCH -next 0/2] Introduce memset_range() helper for wiping members Kees Cook
  2 siblings, 2 replies; 12+ messages in thread
From: Xiu Jianfeng @ 2021-12-08  3:04 UTC (permalink / raw)
  To: akpm, keescook, laniel_francis, andriy.shevchenko, adobriyan,
	linux, andreyknvl, dja, ast, daniel, andrii, kafai,
	songliubraving, yhs, john.fastabend, kpsingh
  Cc: linux-kernel, netdev, bpf

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


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH -next 2/2] bpf: use memset_range helper in __mark_reg_known
  2021-12-08  3:04 [PATCH -next 0/2] Introduce memset_range() helper for wiping members Xiu Jianfeng
  2021-12-08  3:04 ` [PATCH -next 1/2] string.h: Introduce memset_range() " Xiu Jianfeng
@ 2021-12-08  3:04 ` Xiu Jianfeng
  2021-12-08  5:27 ` [PATCH -next 0/2] Introduce memset_range() helper for wiping members Kees Cook
  2 siblings, 0 replies; 12+ messages in thread
From: Xiu Jianfeng @ 2021-12-08  3:04 UTC (permalink / raw)
  To: akpm, keescook, laniel_francis, andriy.shevchenko, adobriyan,
	linux, andreyknvl, dja, ast, daniel, andrii, kafai,
	songliubraving, yhs, john.fastabend, kpsingh
  Cc: linux-kernel, netdev, bpf

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


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH -next 1/2] string.h: Introduce memset_range() for wiping members
  2021-12-08  3:04 ` [PATCH -next 1/2] string.h: Introduce memset_range() " Xiu Jianfeng
@ 2021-12-08  4:28   ` Andrew Morton
  2021-12-08 10:30     ` xiujianfeng
  2021-12-08 17:12   ` Alexey Dobriyan
  1 sibling, 1 reply; 12+ messages in thread
From: Andrew Morton @ 2021-12-08  4:28 UTC (permalink / raw)
  To: Xiu Jianfeng
  Cc: keescook, laniel_francis, andriy.shevchenko, adobriyan, linux,
	andreyknvl, dja, ast, daniel, andrii, kafai, songliubraving, yhs,
	john.fastabend, kpsingh, linux-kernel, netdev, bpf

On Wed, 8 Dec 2021 11:04:50 +0800 Xiu Jianfeng <xiujianfeng@huawei.com> 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?

> ...
>
> --- 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.

> + *
> + * @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"?

> + *
> + */
> +#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'?



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH -next 0/2] Introduce memset_range() helper for wiping members
  2021-12-08  3:04 [PATCH -next 0/2] Introduce memset_range() helper for wiping members Xiu Jianfeng
  2021-12-08  3:04 ` [PATCH -next 1/2] string.h: Introduce memset_range() " Xiu Jianfeng
  2021-12-08  3:04 ` [PATCH -next 2/2] bpf: use memset_range helper in __mark_reg_known Xiu Jianfeng
@ 2021-12-08  5:27 ` Kees Cook
  2021-12-08 10:30   ` xiujianfeng
  2 siblings, 1 reply; 12+ messages in thread
From: Kees Cook @ 2021-12-08  5:27 UTC (permalink / raw)
  To: Xiu Jianfeng
  Cc: akpm, laniel_francis, andriy.shevchenko, adobriyan, linux,
	andreyknvl, dja, ast, daniel, andrii, kafai, songliubraving, yhs,
	john.fastabend, kpsingh, linux-kernel, netdev, bpf

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH -next 1/2] string.h: Introduce memset_range() for wiping members
  2021-12-08  4:28   ` Andrew Morton
@ 2021-12-08 10:30     ` xiujianfeng
  2021-12-08 23:44       ` Andrew Morton
  0 siblings, 1 reply; 12+ messages in thread
From: xiujianfeng @ 2021-12-08 10:30 UTC (permalink / raw)
  To: Andrew Morton
  Cc: keescook, laniel_francis, andriy.shevchenko, adobriyan, linux,
	andreyknvl, dja, ast, daniel, andrii, kafai, songliubraving, yhs,
	john.fastabend, kpsingh, linux-kernel, netdev, bpf


在 2021/12/8 12:28, Andrew Morton 写道:
> On Wed, 8 Dec 2021 11:04:50 +0800 Xiu Jianfeng <xiujianfeng@huawei.com> 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?
There maybe more call site for this function, but I just use bpf as an 
example.
>
>> ...
>>
>> --- 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)
>
>> + *
>> + * @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
>
>> + *
>> + */
>> +#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);
>
>
> .

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH -next 0/2] Introduce memset_range() helper for wiping members
  2021-12-08  5:27 ` [PATCH -next 0/2] Introduce memset_range() helper for wiping members Kees Cook
@ 2021-12-08 10:30   ` xiujianfeng
  0 siblings, 0 replies; 12+ messages in thread
From: xiujianfeng @ 2021-12-08 10:30 UTC (permalink / raw)
  To: Kees Cook
  Cc: akpm, laniel_francis, andriy.shevchenko, adobriyan, linux,
	andreyknvl, dja, ast, daniel, andrii, kafai, songliubraving, yhs,
	john.fastabend, kpsingh, linux-kernel, netdev, bpf


在 2021/12/8 13:27, Kees Cook 写道:
> 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.
Do you mean there is no need to introduce this helper,  but to use 
struct_group in the struct directly?
>
>>    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?
>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH -next 1/2] string.h: Introduce memset_range() for wiping members
  2021-12-08  3:04 ` [PATCH -next 1/2] string.h: Introduce memset_range() " Xiu Jianfeng
  2021-12-08  4:28   ` Andrew Morton
@ 2021-12-08 17:12   ` Alexey Dobriyan
  1 sibling, 0 replies; 12+ messages in thread
From: Alexey Dobriyan @ 2021-12-08 17:12 UTC (permalink / raw)
  To: Xiu Jianfeng
  Cc: akpm, keescook, laniel_francis, andriy.shevchenko, linux,
	andreyknvl, dja, ast, daniel, andrii, kafai, songliubraving, yhs,
	john.fastabend, kpsingh, linux-kernel, netdev, bpf

On Wed, Dec 08, 2021 at 11:04:50AM +0800, Xiu Jianfeng 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.
> 
> 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));			\
> +})

"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...

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH -next 1/2] string.h: Introduce memset_range() for wiping members
  2021-12-08 10:30     ` xiujianfeng
@ 2021-12-08 23:44       ` Andrew Morton
  2021-12-09  5:17         ` Kees Cook
  2021-12-09  6:29         ` xiujianfeng
  0 siblings, 2 replies; 12+ messages in thread
From: Andrew Morton @ 2021-12-08 23:44 UTC (permalink / raw)
  To: xiujianfeng
  Cc: keescook, laniel_francis, andriy.shevchenko, adobriyan, linux,
	andreyknvl, dja, ast, daniel, andrii, kafai, songliubraving, yhs,
	john.fastabend, kpsingh, linux-kernel, netdev, bpf

On Wed, 8 Dec 2021 18:30:26 +0800 xiujianfeng <xiujianfeng@huawei.com> wrote:

> 
> 在 2021/12/8 12:28, Andrew Morton 写道:
> > On Wed, 8 Dec 2021 11:04:50 +0800 Xiu Jianfeng <xiujianfeng@huawei.com> 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?
> There maybe more call site for this function, but I just use bpf as an 
> example.
> >
> >> ...
> >>
> >> --- 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)
> >
> >> + *
> >> + * @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.

> >
> >> + *
> >> + */
> >> +#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.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH -next 1/2] string.h: Introduce memset_range() for wiping members
  2021-12-08 23:44       ` Andrew Morton
@ 2021-12-09  5:17         ` Kees Cook
  2021-12-09  6:29           ` xiujianfeng
  2021-12-09  6:29         ` xiujianfeng
  1 sibling, 1 reply; 12+ messages in thread
From: Kees Cook @ 2021-12-09  5:17 UTC (permalink / raw)
  To: Andrew Morton
  Cc: xiujianfeng, laniel_francis, andriy.shevchenko, adobriyan, linux,
	andreyknvl, dja, ast, daniel, andrii, kafai, songliubraving, yhs,
	john.fastabend, kpsingh, linux-kernel, netdev, bpf

On Wed, Dec 08, 2021 at 03:44:37PM -0800, Andrew Morton wrote:
> On Wed, 8 Dec 2021 18:30:26 +0800 xiujianfeng <xiujianfeng@huawei.com> wrote:
> 
> > 
> > 在 2021/12/8 12:28, Andrew Morton 写道:
> > > On Wed, 8 Dec 2021 11:04:50 +0800 Xiu Jianfeng <xiujianfeng@huawei.com> 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?
> > There maybe more call site for this function, but I just use bpf as an 
> > example.
> > >
> > >> ...
> > >>
> > >> --- 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)
> > >
> > >> + *
> > >> + * @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.
> 
> > >
> > >> + *
> > >> + */
> > >> +#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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH -next 1/2] string.h: Introduce memset_range() for wiping members
  2021-12-08 23:44       ` Andrew Morton
  2021-12-09  5:17         ` Kees Cook
@ 2021-12-09  6:29         ` xiujianfeng
  1 sibling, 0 replies; 12+ messages in thread
From: xiujianfeng @ 2021-12-09  6:29 UTC (permalink / raw)
  To: Andrew Morton
  Cc: keescook, laniel_francis, andriy.shevchenko, adobriyan, linux,
	andreyknvl, dja, ast, daniel, andrii, kafai, songliubraving, yhs,
	john.fastabend, kpsingh, linux-kernel, netdev, bpf


在 2021/12/9 7:44, Andrew Morton 写道:
> On Wed, 8 Dec 2021 18:30:26 +0800 xiujianfeng <xiujianfeng@huawei.com> wrote:
>
>> 在 2021/12/8 12:28, Andrew Morton 写道:
>>> On Wed, 8 Dec 2021 11:04:50 +0800 Xiu Jianfeng <xiujianfeng@huawei.com> 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?
>> There maybe more call site for this function, but I just use bpf as an
>> example.
>>>> ...
>>>>
>>>> --- 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)
>>>> + *
>>>> + * @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 :)
>
>>>> + *
>>>> + */
>>>> +#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.

> .

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH -next 1/2] string.h: Introduce memset_range() for wiping members
  2021-12-09  5:17         ` Kees Cook
@ 2021-12-09  6:29           ` xiujianfeng
  0 siblings, 0 replies; 12+ messages in thread
From: xiujianfeng @ 2021-12-09  6:29 UTC (permalink / raw)
  To: Kees Cook, Andrew Morton
  Cc: laniel_francis, andriy.shevchenko, adobriyan, linux, andreyknvl,
	dja, ast, daniel, andrii, kafai, songliubraving, yhs,
	john.fastabend, kpsingh, linux-kernel, netdev, bpf


在 2021/12/9 13:17, Kees Cook 写道:
> On Wed, Dec 08, 2021 at 03:44:37PM -0800, Andrew Morton wrote:
>> On Wed, 8 Dec 2021 18:30:26 +0800 xiujianfeng <xiujianfeng@huawei.com> wrote:
>>
>>> 在 2021/12/8 12:28, Andrew Morton 写道:
>>>> On Wed, 8 Dec 2021 11:04:50 +0800 Xiu Jianfeng <xiujianfeng@huawei.com> 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?
>>> There maybe more call site for this function, but I just use bpf as an
>>> example.
>>>>> ...
>>>>>
>>>>> --- 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)
>>>>> + *
>>>>> + * @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.
>>
>>>>> + *
>>>>> + */
>>>>> +#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.

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2021-12-09  6:29 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-08  3:04 [PATCH -next 0/2] Introduce memset_range() helper for wiping members Xiu Jianfeng
2021-12-08  3:04 ` [PATCH -next 1/2] string.h: Introduce memset_range() " Xiu Jianfeng
2021-12-08  4:28   ` Andrew Morton
2021-12-08 10:30     ` xiujianfeng
2021-12-08 23:44       ` Andrew Morton
2021-12-09  5:17         ` Kees Cook
2021-12-09  6:29           ` xiujianfeng
2021-12-09  6:29         ` xiujianfeng
2021-12-08 17:12   ` Alexey Dobriyan
2021-12-08  3:04 ` [PATCH -next 2/2] bpf: use memset_range helper in __mark_reg_known Xiu Jianfeng
2021-12-08  5:27 ` [PATCH -next 0/2] Introduce memset_range() helper for wiping members Kees Cook
2021-12-08 10:30   ` xiujianfeng

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).