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 <[email protected]>
---
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 <[email protected]>
---
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 <[email protected]> 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'?
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 <[email protected]> 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);
>
>
> .
在 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?
>
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 <[email protected]>
> ---
> 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...
On Wed, 8 Dec 2021 18:30:26 +0800 xiujianfeng <[email protected]> wrote:
>
> 在 2021/12/8 12:28, Andrew Morton 写道:
> > On Wed, 8 Dec 2021 11:04:50 +0800 Xiu Jianfeng <[email protected]> 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.
On Wed, Dec 08, 2021 at 03:44:37PM -0800, Andrew Morton wrote:
> On Wed, 8 Dec 2021 18:30:26 +0800 xiujianfeng <[email protected]> wrote:
>
> >
> > 在 2021/12/8 12:28, Andrew Morton 写道:
> > > On Wed, 8 Dec 2021 11:04:50 +0800 Xiu Jianfeng <[email protected]> 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
在 2021/12/9 7:44, Andrew Morton 写道:
> On Wed, 8 Dec 2021 18:30:26 +0800 xiujianfeng <[email protected]> wrote:
>
>> 在 2021/12/8 12:28, Andrew Morton 写道:
>>> On Wed, 8 Dec 2021 11:04:50 +0800 Xiu Jianfeng <[email protected]> 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.
> .
在 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 <[email protected]> wrote:
>>
>>> 在 2021/12/8 12:28, Andrew Morton 写道:
>>>> On Wed, 8 Dec 2021 11:04:50 +0800 Xiu Jianfeng <[email protected]> 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.