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