| Message ID | 20211208030451.219751-2-xiujianfeng@huawei.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | Introduce memset_range() helper for wiping members | expand |
| Context | Check | Description |
|---|---|---|
| bpf/vmtest-bpf-next | pending | VM_Test |
| bpf/vmtest-bpf-next-PR | pending | PR summary |
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
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'?
在 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); > > > .
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...
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.
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).
在 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. > .
在 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.
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 }
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(+)