On Sun, Mar 17, 2024 at 07:49:43PM -0700, David Wei wrote: > I'm working on a similar proposal for zero copy Rx but to host memory > and depend on this memory provider API. How do you need a different provider for that vs just udmabuf? > Jakub also designed this API for hugepages too IIRC. Basically there's > going to be at least three fancy ways of providing pages (one of which > isn't actually pages, hence the merged netmem_t series) to drivers. How do hugepages different from a normal page allocation? They should just a different ordered passed to the page allocator.
Guenter Roeck <linux@roeck-us.net> writes:
> On 3/14/24 07:37, Guenter Roeck wrote:
>> On 3/14/24 06:36, Geert Uytterhoeven wrote:
>>> Hi Günter,
>>>
>>> On Tue, Mar 12, 2024 at 6:03 PM Guenter Roeck <linux@roeck-us.net> wrote:
>>>> Some unit tests intentionally trigger warning backtraces by passing bad
>>>> parameters to kernel API functions. Such unit tests typically check the
>>>> return value from such calls, not the existence of the warning backtrace.
>>>>
>>>> Such intentionally generated warning backtraces are neither desirable
>>>> nor useful for a number of reasons.
>>>> - They can result in overlooked real problems.
>>>> - A warning that suddenly starts to show up in unit tests needs to be
>>>> investigated and has to be marked to be ignored, for example by
>>>> adjusting filter scripts. Such filters are ad-hoc because there is
>>>> no real standard format for warnings. On top of that, such filter
>>>> scripts would require constant maintenance.
>>>>
>>>> One option to address problem would be to add messages such as "expected
>>>> warning backtraces start / end here" to the kernel log. However, that
>>>> would again require filter scripts, it might result in missing real
>>>> problematic warning backtraces triggered while the test is running, and
>>>> the irrelevant backtrace(s) would still clog the kernel log.
>>>>
>>>> Solve the problem by providing a means to identify and suppress specific
>>>> warning backtraces while executing test code. Support suppressing multiple
>>>> backtraces while at the same time limiting changes to generic code to the
>>>> absolute minimum. Architecture specific changes are kept at minimum by
>>>> retaining function names only if both CONFIG_DEBUG_BUGVERBOSE and
>>>> CONFIG_KUNIT are enabled.
>>>>
>>>> The first patch of the series introduces the necessary infrastructure.
>>>> The second patch introduces support for counting suppressed backtraces.
>>>> This capability is used in patch three to implement unit tests.
>>>> Patch four documents the new API.
>>>> The next two patches add support for suppressing backtraces in drm_rect
>>>> and dev_addr_lists unit tests. These patches are intended to serve as
>>>> examples for the use of the functionality introduced with this series.
>>>> The remaining patches implement the necessary changes for all
>>>> architectures with GENERIC_BUG support.
>>>
>>> Thanks for your series!
>>>
>>> I gave it a try on m68k, just running backtrace-suppression-test,
>>> and that seems to work fine.
>>>
>>>> Design note:
>>>> Function pointers are only added to the __bug_table section if both
>>>> CONFIG_KUNIT and CONFIG_DEBUG_BUGVERBOSE are enabled to avoid image
>>>> size increases if CONFIG_KUNIT=n. There would be some benefits to
>>>> adding those pointers all the time (reduced complexity, ability to
>>>> display function names in BUG/WARNING messages). That change, if
>>>> desired, can be made later.
>>>
>>> Unfortunately this also increases kernel size in the CONFIG_KUNIT=m
>>> case (ca. 80 KiB for atari_defconfig), making it less attractive to have
>>> kunit and all tests enabled as modules in my standard kernel.
>>>
>>
>> Good point. Indeed, it does. I wanted to avoid adding a configuration option,
>> but maybe I should add it after all. How about something like this ?
>>
>> +config KUNIT_SUPPRESS_BACKTRACE
>> + bool "KUnit - Enable backtrace suppression"
>> + default y
>> + help
>> + Enable backtrace suppression for KUnit. If enabled, backtraces
>> + generated intentionally by KUnit tests can be suppressed. Disable
>> + to reduce kernel image size if image size is more important than
>> + suppression of backtraces generated by KUnit tests.
>
> Any more comments / feedback on this ? Otherwise I'll introduce the
> above configuration option in v2 of the series.
>
> In this context, any suggestions if it should be enabled or disabled by
> default ? I personally think it would be more important to be able to
> suppress backtraces, but I understand that others may not be willing to
> accept a ~1% image size increase with CONFIG_KUNIT=m unless
> KUNIT_SUPPRESS_BACKTRACE is explicitly disabled.
Please enable it by default.
There are multiple CI systems that will benefit from it, whereas the
number of users enabling KUNIT in severely spaced constrainted
environments is surely small - perhaps just Geert ;).
cheers
On 2024-03-17 19:02, Christoph Hellwig wrote: > On Mon, Mar 04, 2024 at 06:01:37PM -0800, Mina Almasry wrote: >> From: Jakub Kicinski <kuba@kernel.org> >> >> The page providers which try to reuse the same pages will >> need to hold onto the ref, even if page gets released from >> the pool - as in releasing the page from the pp just transfers >> the "ownership" reference from pp to the provider, and provider >> will wait for other references to be gone before feeding this >> page back into the pool. > > The word hook always rings a giant warning bell for me, and looking into > this series I am concerned indeed. > > The only provider provided here is the dma-buf one, and that basically > is the only sensible one for the documented design. So instead of > adding hooks that random proprietary crap can hook into, why not hard > code the dma buf provide and just use a flag? That'll also avoid > expensive indirect calls. I'm working on a similar proposal for zero copy Rx but to host memory and depend on this memory provider API. Jakub also designed this API for hugepages too IIRC. Basically there's going to be at least three fancy ways of providing pages (one of which isn't actually pages, hence the merged netmem_t series) to drivers. >
On Mon, Mar 04, 2024 at 06:01:37PM -0800, Mina Almasry wrote:
> From: Jakub Kicinski <kuba@kernel.org>
>
> The page providers which try to reuse the same pages will
> need to hold onto the ref, even if page gets released from
> the pool - as in releasing the page from the pp just transfers
> the "ownership" reference from pp to the provider, and provider
> will wait for other references to be gone before feeding this
> page back into the pool.
The word hook always rings a giant warning bell for me, and looking into
this series I am concerned indeed.
The only provider provided here is the dma-buf one, and that basically
is the only sensible one for the documented design. So instead of
adding hooks that random proprietary crap can hook into, why not hard
code the dma buf provide and just use a flag? That'll also avoid
expensive indirect calls.
The pull request you sent on Sat, 16 Mar 2024 22:49:56 +0100: > git://git.kernel.org/pub/scm/linux/kernel/git/deller/parisc-linux.git tags/parisc-for-6.9-rc1 has been merged into torvalds/linux.git: https://git.kernel.org/torvalds/c/342d965376c5dafa124eb1c642d2fce043407cea Thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/prtracker.html
Hi Linus, Please pull updates and fixes for the parisc architecture for 6.9-rc1: Fixes for the IPv4 and IPv6 checksum functions, a fix for the 64-bit unaligned memory exception handler and various code cleanups. Most of the patches are tagged for stable series. Thanks! Helge ---------------------------------------------------------------- The following changes since commit d206a76d7d2726f3b096037f2079ce0bd3ba329b: Linux 6.8-rc6 (2024-02-25 15:46:06 -0800) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/deller/parisc-linux.git tags/parisc-for-6.9-rc1 for you to fetch changes up to 26dd48780bd2232a8f50f878929a9e448b7fd531: parisc: led: Convert to platform remove callback returning void (2024-03-08 10:00:07 +0100) ---------------------------------------------------------------- parisc architecture updates and fixes for kernel v6.9-rc1: - Fix inline assembly in ipv4 and ipv6 checksum functions (Guenter Roeck) - Rewrite 64-bit inline assembly of emulate_ldd() (Guenter Roeck) - Do not clobber carry/borrow bits in tophys and tovirt macros (John David Anglin) - Warn when kernel accesses unaligned memory ---------------------------------------------------------------- Arnd Bergmann (1): parisc: avoid c23 'nullptr' idenitifier Guenter Roeck (5): parisc/unaligned: Rewrite 64-bit inline assembly of emulate_ldd() parisc: Fix ip_fast_csum parisc: Fix csum_ipv6_magic on 32-bit systems parisc: Fix csum_ipv6_magic on 64-bit systems parisc: Strip upper 32 bit of sum in csum_ipv6_magic for 64-bit builds Helge Deller (2): parisc: Use irq_enter_rcu() to fix warning at kernel/context_tracking.c:367 parisc: Show kernel unaligned memory accesses John David Anglin (1): parisc: Avoid clobbering the C/B bits in the PSW with tophys and tovirt macros Ricardo B. Marliere (1): parisc: make parisc_bus_type const Uwe Kleine-König (1): parisc: led: Convert to platform remove callback returning void arch/parisc/include/asm/assembly.h | 18 +++++++++-------- arch/parisc/include/asm/checksum.h | 10 ++++++---- arch/parisc/include/asm/parisc-device.h | 2 +- arch/parisc/kernel/drivers.c | 2 +- arch/parisc/kernel/irq.c | 4 ++-- arch/parisc/kernel/unaligned.c | 34 ++++++++++++++++++--------------- arch/parisc/math-emu/dfsqrt.c | 4 ++-- arch/parisc/math-emu/fcnvff.c | 8 ++++---- arch/parisc/math-emu/fcnvfu.c | 16 ++++++++-------- arch/parisc/math-emu/fcnvfut.c | 16 ++++++++-------- arch/parisc/math-emu/fcnvfx.c | 16 ++++++++-------- arch/parisc/math-emu/fcnvfxt.c | 16 ++++++++-------- arch/parisc/math-emu/fcnvuf.c | 16 ++++++++-------- arch/parisc/math-emu/fcnvxf.c | 16 ++++++++-------- arch/parisc/math-emu/frnd.c | 8 ++++---- arch/parisc/math-emu/sfsqrt.c | 4 ++-- drivers/parisc/led.c | 6 ++---- 17 files changed, 101 insertions(+), 95 deletions(-)
On 3/14/24 07:37, Guenter Roeck wrote:
> On 3/14/24 06:36, Geert Uytterhoeven wrote:
>> Hi Günter,
>>
>> On Tue, Mar 12, 2024 at 6:03 PM Guenter Roeck <linux@roeck-us.net> wrote:
>>> Some unit tests intentionally trigger warning backtraces by passing bad
>>> parameters to kernel API functions. Such unit tests typically check the
>>> return value from such calls, not the existence of the warning backtrace.
>>>
>>> Such intentionally generated warning backtraces are neither desirable
>>> nor useful for a number of reasons.
>>> - They can result in overlooked real problems.
>>> - A warning that suddenly starts to show up in unit tests needs to be
>>> investigated and has to be marked to be ignored, for example by
>>> adjusting filter scripts. Such filters are ad-hoc because there is
>>> no real standard format for warnings. On top of that, such filter
>>> scripts would require constant maintenance.
>>>
>>> One option to address problem would be to add messages such as "expected
>>> warning backtraces start / end here" to the kernel log. However, that
>>> would again require filter scripts, it might result in missing real
>>> problematic warning backtraces triggered while the test is running, and
>>> the irrelevant backtrace(s) would still clog the kernel log.
>>>
>>> Solve the problem by providing a means to identify and suppress specific
>>> warning backtraces while executing test code. Support suppressing multiple
>>> backtraces while at the same time limiting changes to generic code to the
>>> absolute minimum. Architecture specific changes are kept at minimum by
>>> retaining function names only if both CONFIG_DEBUG_BUGVERBOSE and
>>> CONFIG_KUNIT are enabled.
>>>
>>> The first patch of the series introduces the necessary infrastructure.
>>> The second patch introduces support for counting suppressed backtraces.
>>> This capability is used in patch three to implement unit tests.
>>> Patch four documents the new API.
>>> The next two patches add support for suppressing backtraces in drm_rect
>>> and dev_addr_lists unit tests. These patches are intended to serve as
>>> examples for the use of the functionality introduced with this series.
>>> The remaining patches implement the necessary changes for all
>>> architectures with GENERIC_BUG support.
>>
>> Thanks for your series!
>>
>> I gave it a try on m68k, just running backtrace-suppression-test,
>> and that seems to work fine.
>>
>>> Design note:
>>> Function pointers are only added to the __bug_table section if both
>>> CONFIG_KUNIT and CONFIG_DEBUG_BUGVERBOSE are enabled to avoid image
>>> size increases if CONFIG_KUNIT=n. There would be some benefits to
>>> adding those pointers all the time (reduced complexity, ability to
>>> display function names in BUG/WARNING messages). That change, if
>>> desired, can be made later.
>>
>> Unfortunately this also increases kernel size in the CONFIG_KUNIT=m
>> case (ca. 80 KiB for atari_defconfig), making it less attractive to have
>> kunit and all tests enabled as modules in my standard kernel.
>>
>
> Good point. Indeed, it does. I wanted to avoid adding a configuration option,
> but maybe I should add it after all. How about something like this ?
>
> +config KUNIT_SUPPRESS_BACKTRACE
> + bool "KUnit - Enable backtrace suppression"
> + default y
> + help
> + Enable backtrace suppression for KUnit. If enabled, backtraces
> + generated intentionally by KUnit tests can be suppressed. Disable
> + to reduce kernel image size if image size is more important than
> + suppression of backtraces generated by KUnit tests.
> +
>
Any more comments / feedback on this ? Otherwise I'll introduce the
above configuration option in v2 of the series.
In this context, any suggestions if it should be enabled or disabled by
default ? I personally think it would be more important to be able to
suppress backtraces, but I understand that others may not be willing to
accept a ~1% image size increase with CONFIG_KUNIT=m unless
KUNIT_SUPPRESS_BACKTRACE is explicitly disabled.
Thanks,
Guenter
On 3/12/24 18:03, Guenter Roeck wrote: > Add name of functions triggering warning backtraces to the __bug_table > object section to enable support for suppressing WARNING backtraces. > > To limit image size impact, the pointer to the function name is only added > to the __bug_table section if both CONFIG_KUNIT and CONFIG_DEBUG_BUGVERBOSE > are enabled. Otherwise, the __func__ assembly parameter is replaced with a > (dummy) NULL parameter to avoid an image size increase due to unused > __func__ entries (this is necessary because __func__ is not a define but a > virtual variable). > > While at it, declare assembler parameters as constants where possible. > Refine .blockz instructions to calculate the necessary padding instead > of using fixed values. > > Signed-off-by: Guenter Roeck <linux@roeck-us.net> Acked-by: Helge Deller <deller@gmx.de> Helge > --- > arch/parisc/include/asm/bug.h | 29 +++++++++++++++++++++-------- > 1 file changed, 21 insertions(+), 8 deletions(-) > > diff --git a/arch/parisc/include/asm/bug.h b/arch/parisc/include/asm/bug.h > index 833555f74ffa..792dacc2a653 100644 > --- a/arch/parisc/include/asm/bug.h > +++ b/arch/parisc/include/asm/bug.h > @@ -23,8 +23,17 @@ > # define __BUG_REL(val) ".word " __stringify(val) > #endif > > - > #ifdef CONFIG_DEBUG_BUGVERBOSE > + > +#if IS_ENABLED(CONFIG_KUNIT) > +# define HAVE_BUG_FUNCTION > +# define __BUG_FUNC_PTR __BUG_REL(%c1) > +# define __BUG_FUNC __func__ > +#else > +# define __BUG_FUNC_PTR > +# define __BUG_FUNC NULL > +#endif /* IS_ENABLED(CONFIG_KUNIT) */ > + > #define BUG() \ > do { \ > asm volatile("\n" \ > @@ -33,10 +42,12 @@ > "\t.align 4\n" \ > "2:\t" __BUG_REL(1b) "\n" \ > "\t" __BUG_REL(%c0) "\n" \ > - "\t.short %1, %2\n" \ > - "\t.blockz %3-2*4-2*2\n" \ > + "\t" __BUG_FUNC_PTR "\n" \ > + "\t.short %c2, %c3\n" \ > + "\t.blockz %c4-(.-2b)\n" \ > "\t.popsection" \ > - : : "i" (__FILE__), "i" (__LINE__), \ > + : : "i" (__FILE__), "i" (__BUG_FUNC), \ > + "i" (__LINE__), \ > "i" (0), "i" (sizeof(struct bug_entry)) ); \ > unreachable(); \ > } while(0) > @@ -58,10 +69,12 @@ > "\t.align 4\n" \ > "2:\t" __BUG_REL(1b) "\n" \ > "\t" __BUG_REL(%c0) "\n" \ > - "\t.short %1, %2\n" \ > - "\t.blockz %3-2*4-2*2\n" \ > + "\t" __BUG_FUNC_PTR "\n" \ > + "\t.short %c2, %3\n" \ > + "\t.blockz %c4-(.-2b)\n" \ > "\t.popsection" \ > - : : "i" (__FILE__), "i" (__LINE__), \ > + : : "i" (__FILE__), "i" (__BUG_FUNC), \ > + "i" (__LINE__), \ > "i" (BUGFLAG_WARNING|(flags)), \ > "i" (sizeof(struct bug_entry)) ); \ > } while(0) > @@ -74,7 +87,7 @@ > "\t.align 4\n" \ > "2:\t" __BUG_REL(1b) "\n" \ > "\t.short %0\n" \ > - "\t.blockz %1-4-2\n" \ > + "\t.blockz %c1-(.-2b)\n" \ > "\t.popsection" \ > : : "i" (BUGFLAG_WARNING|(flags)), \ > "i" (sizeof(struct bug_entry)) ); \
Hello: This series was applied to bpf/bpf-next.git (master) by Alexei Starovoitov <ast@kernel.org>: On Fri, 8 Mar 2024 06:38:07 +0100 you wrote: > set_memory_ro() can fail, leaving memory unprotected. > > Check its return and take it into account as an error. > > Link: https://github.com/KSPP/linux/issues/7 > Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> > Cc: linux-hardening@vger.kernel.org <linux-hardening@vger.kernel.org> > Reviewed-by: Kees Cook <keescook@chromium.org> > > [...] Here is the summary with links: - [bpf-next,v3,1/2] bpf: Take return from set_memory_ro() into account with bpf_prog_lock_ro() https://git.kernel.org/bpf/bpf-next/c/7d2cc63eca0c - [bpf-next,v3,2/2] bpf: Take return from set_memory_rox() into account with bpf_jit_binary_lock_ro() https://git.kernel.org/bpf/bpf-next/c/e60adf513275 You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html
On 3/14/24 08:02, Maxime Ripard wrote:
> On Thu, Mar 14, 2024 at 07:37:13AM -0700, Guenter Roeck wrote:
>> On 3/14/24 06:36, Geert Uytterhoeven wrote:
>>> Hi Günter,
>>>
>>> On Tue, Mar 12, 2024 at 6:03 PM Guenter Roeck <linux@roeck-us.net> wrote:
>>>> Some unit tests intentionally trigger warning backtraces by passing bad
>>>> parameters to kernel API functions. Such unit tests typically check the
>>>> return value from such calls, not the existence of the warning backtrace.
>>>>
>>>> Such intentionally generated warning backtraces are neither desirable
>>>> nor useful for a number of reasons.
>>>> - They can result in overlooked real problems.
>>>> - A warning that suddenly starts to show up in unit tests needs to be
>>>> investigated and has to be marked to be ignored, for example by
>>>> adjusting filter scripts. Such filters are ad-hoc because there is
>>>> no real standard format for warnings. On top of that, such filter
>>>> scripts would require constant maintenance.
>>>>
>>>> One option to address problem would be to add messages such as "expected
>>>> warning backtraces start / end here" to the kernel log. However, that
>>>> would again require filter scripts, it might result in missing real
>>>> problematic warning backtraces triggered while the test is running, and
>>>> the irrelevant backtrace(s) would still clog the kernel log.
>>>>
>>>> Solve the problem by providing a means to identify and suppress specific
>>>> warning backtraces while executing test code. Support suppressing multiple
>>>> backtraces while at the same time limiting changes to generic code to the
>>>> absolute minimum. Architecture specific changes are kept at minimum by
>>>> retaining function names only if both CONFIG_DEBUG_BUGVERBOSE and
>>>> CONFIG_KUNIT are enabled.
>>>>
>>>> The first patch of the series introduces the necessary infrastructure.
>>>> The second patch introduces support for counting suppressed backtraces.
>>>> This capability is used in patch three to implement unit tests.
>>>> Patch four documents the new API.
>>>> The next two patches add support for suppressing backtraces in drm_rect
>>>> and dev_addr_lists unit tests. These patches are intended to serve as
>>>> examples for the use of the functionality introduced with this series.
>>>> The remaining patches implement the necessary changes for all
>>>> architectures with GENERIC_BUG support.
>>>
>>> Thanks for your series!
>>>
>>> I gave it a try on m68k, just running backtrace-suppression-test,
>>> and that seems to work fine.
>>>
>>>> Design note:
>>>> Function pointers are only added to the __bug_table section if both
>>>> CONFIG_KUNIT and CONFIG_DEBUG_BUGVERBOSE are enabled to avoid image
>>>> size increases if CONFIG_KUNIT=n. There would be some benefits to
>>>> adding those pointers all the time (reduced complexity, ability to
>>>> display function names in BUG/WARNING messages). That change, if
>>>> desired, can be made later.
>>>
>>> Unfortunately this also increases kernel size in the CONFIG_KUNIT=m
>>> case (ca. 80 KiB for atari_defconfig), making it less attractive to have
>>> kunit and all tests enabled as modules in my standard kernel.
>>>
>>
>> Good point. Indeed, it does. I wanted to avoid adding a configuration option,
>> but maybe I should add it after all. How about something like this ?
>>
>> +config KUNIT_SUPPRESS_BACKTRACE
>> + bool "KUnit - Enable backtrace suppression"
>> + default y
>> + help
>> + Enable backtrace suppression for KUnit. If enabled, backtraces
>> + generated intentionally by KUnit tests can be suppressed. Disable
>> + to reduce kernel image size if image size is more important than
>> + suppression of backtraces generated by KUnit tests.
>> +
>
> How are tests using that API supposed to handle it then?
>
> Select the config option or put an ifdef?
>
> If the former, we end up in the same situation than without the symbol.
> If the latter, we end up in a similar situation than disabling KUNIT
> entirely, with some tests not being run which is just terrible.
>
The API definitions are themselves within #ifdef and dummies if
KUNIT_SUPPRESS_BACKTRACE (currently CONFIG_KUNIT) is disabled.
In include/kunit/bug.h:
#ifdef CONFIG_KUNIT_SUPPRESS_BACKTRACE
...
#else
#define DEFINE_SUPPRESSED_WARNING(func)
#define START_SUPPRESSED_WARNING(func)
#define END_SUPPRESSED_WARNING(func)
#define IS_SUPPRESSED_WARNING(func) (false)
#define SUPPRESSED_WARNING_COUNT(func) (0)
#endif
Only difference to the current patch series would be
- #if IS_ENABLED(CONFIG_KUNIT)
+ #ifdef CONFIG_KUNIT_SUPPRESS_BACKTRACE
in that file and elsewhere.
With KUNIT_SUPPRESS_BACKTRACE=n you'd still get warning backtraces
triggered by the tests, but the number of tests executed as well
as the test results would still be the same.
Thanks,
Guenter
[-- Attachment #1: Type: text/plain, Size: 4185 bytes --] On Thu, Mar 14, 2024 at 07:37:13AM -0700, Guenter Roeck wrote: > On 3/14/24 06:36, Geert Uytterhoeven wrote: > > Hi Günter, > > > > On Tue, Mar 12, 2024 at 6:03 PM Guenter Roeck <linux@roeck-us.net> wrote: > > > Some unit tests intentionally trigger warning backtraces by passing bad > > > parameters to kernel API functions. Such unit tests typically check the > > > return value from such calls, not the existence of the warning backtrace. > > > > > > Such intentionally generated warning backtraces are neither desirable > > > nor useful for a number of reasons. > > > - They can result in overlooked real problems. > > > - A warning that suddenly starts to show up in unit tests needs to be > > > investigated and has to be marked to be ignored, for example by > > > adjusting filter scripts. Such filters are ad-hoc because there is > > > no real standard format for warnings. On top of that, such filter > > > scripts would require constant maintenance. > > > > > > One option to address problem would be to add messages such as "expected > > > warning backtraces start / end here" to the kernel log. However, that > > > would again require filter scripts, it might result in missing real > > > problematic warning backtraces triggered while the test is running, and > > > the irrelevant backtrace(s) would still clog the kernel log. > > > > > > Solve the problem by providing a means to identify and suppress specific > > > warning backtraces while executing test code. Support suppressing multiple > > > backtraces while at the same time limiting changes to generic code to the > > > absolute minimum. Architecture specific changes are kept at minimum by > > > retaining function names only if both CONFIG_DEBUG_BUGVERBOSE and > > > CONFIG_KUNIT are enabled. > > > > > > The first patch of the series introduces the necessary infrastructure. > > > The second patch introduces support for counting suppressed backtraces. > > > This capability is used in patch three to implement unit tests. > > > Patch four documents the new API. > > > The next two patches add support for suppressing backtraces in drm_rect > > > and dev_addr_lists unit tests. These patches are intended to serve as > > > examples for the use of the functionality introduced with this series. > > > The remaining patches implement the necessary changes for all > > > architectures with GENERIC_BUG support. > > > > Thanks for your series! > > > > I gave it a try on m68k, just running backtrace-suppression-test, > > and that seems to work fine. > > > > > Design note: > > > Function pointers are only added to the __bug_table section if both > > > CONFIG_KUNIT and CONFIG_DEBUG_BUGVERBOSE are enabled to avoid image > > > size increases if CONFIG_KUNIT=n. There would be some benefits to > > > adding those pointers all the time (reduced complexity, ability to > > > display function names in BUG/WARNING messages). That change, if > > > desired, can be made later. > > > > Unfortunately this also increases kernel size in the CONFIG_KUNIT=m > > case (ca. 80 KiB for atari_defconfig), making it less attractive to have > > kunit and all tests enabled as modules in my standard kernel. > > > > Good point. Indeed, it does. I wanted to avoid adding a configuration option, > but maybe I should add it after all. How about something like this ? > > +config KUNIT_SUPPRESS_BACKTRACE > + bool "KUnit - Enable backtrace suppression" > + default y > + help > + Enable backtrace suppression for KUnit. If enabled, backtraces > + generated intentionally by KUnit tests can be suppressed. Disable > + to reduce kernel image size if image size is more important than > + suppression of backtraces generated by KUnit tests. > + How are tests using that API supposed to handle it then? Select the config option or put an ifdef? If the former, we end up in the same situation than without the symbol. If the latter, we end up in a similar situation than disabling KUNIT entirely, with some tests not being run which is just terrible. Maxime [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --]
On 3/14/24 06:36, Geert Uytterhoeven wrote:
> Hi Günter,
>
> On Tue, Mar 12, 2024 at 6:03 PM Guenter Roeck <linux@roeck-us.net> wrote:
>> Some unit tests intentionally trigger warning backtraces by passing bad
>> parameters to kernel API functions. Such unit tests typically check the
>> return value from such calls, not the existence of the warning backtrace.
>>
>> Such intentionally generated warning backtraces are neither desirable
>> nor useful for a number of reasons.
>> - They can result in overlooked real problems.
>> - A warning that suddenly starts to show up in unit tests needs to be
>> investigated and has to be marked to be ignored, for example by
>> adjusting filter scripts. Such filters are ad-hoc because there is
>> no real standard format for warnings. On top of that, such filter
>> scripts would require constant maintenance.
>>
>> One option to address problem would be to add messages such as "expected
>> warning backtraces start / end here" to the kernel log. However, that
>> would again require filter scripts, it might result in missing real
>> problematic warning backtraces triggered while the test is running, and
>> the irrelevant backtrace(s) would still clog the kernel log.
>>
>> Solve the problem by providing a means to identify and suppress specific
>> warning backtraces while executing test code. Support suppressing multiple
>> backtraces while at the same time limiting changes to generic code to the
>> absolute minimum. Architecture specific changes are kept at minimum by
>> retaining function names only if both CONFIG_DEBUG_BUGVERBOSE and
>> CONFIG_KUNIT are enabled.
>>
>> The first patch of the series introduces the necessary infrastructure.
>> The second patch introduces support for counting suppressed backtraces.
>> This capability is used in patch three to implement unit tests.
>> Patch four documents the new API.
>> The next two patches add support for suppressing backtraces in drm_rect
>> and dev_addr_lists unit tests. These patches are intended to serve as
>> examples for the use of the functionality introduced with this series.
>> The remaining patches implement the necessary changes for all
>> architectures with GENERIC_BUG support.
>
> Thanks for your series!
>
> I gave it a try on m68k, just running backtrace-suppression-test,
> and that seems to work fine.
>
>> Design note:
>> Function pointers are only added to the __bug_table section if both
>> CONFIG_KUNIT and CONFIG_DEBUG_BUGVERBOSE are enabled to avoid image
>> size increases if CONFIG_KUNIT=n. There would be some benefits to
>> adding those pointers all the time (reduced complexity, ability to
>> display function names in BUG/WARNING messages). That change, if
>> desired, can be made later.
>
> Unfortunately this also increases kernel size in the CONFIG_KUNIT=m
> case (ca. 80 KiB for atari_defconfig), making it less attractive to have
> kunit and all tests enabled as modules in my standard kernel.
>
Good point. Indeed, it does. I wanted to avoid adding a configuration option,
but maybe I should add it after all. How about something like this ?
+config KUNIT_SUPPRESS_BACKTRACE
+ bool "KUnit - Enable backtrace suppression"
+ default y
+ help
+ Enable backtrace suppression for KUnit. If enabled, backtraces
+ generated intentionally by KUnit tests can be suppressed. Disable
+ to reduce kernel image size if image size is more important than
+ suppression of backtraces generated by KUnit tests.
+
Thanks,
Guenter
On 3/14/24 00:57, Geert Uytterhoeven wrote:
> Hi Günter,
>
> On Tue, Mar 12, 2024 at 6:06 PM Guenter Roeck <linux@roeck-us.net> wrote:
>> Add name of functions triggering warning backtraces to the __bug_table
>> object section to enable support for suppressing WARNING backtraces.
>>
>> To limit image size impact, the pointer to the function name is only added
>> to the __bug_table section if both CONFIG_KUNIT and CONFIG_DEBUG_BUGVERBOSE
>> are enabled. Otherwise, the __func__ assembly parameter is replaced with a
>> (dummy) NULL parameter to avoid an image size increase due to unused
>> __func__ entries (this is necessary because __func__ is not a define but a
>> virtual variable).
>>
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>
> Thanks for your patch!
>
>> --- a/arch/s390/include/asm/bug.h
>> +++ b/arch/s390/include/asm/bug.h
>> @@ -8,19 +8,30 @@
>>
>> #ifdef CONFIG_DEBUG_BUGVERBOSE
>>
>> +#if IS_ENABLED(CONFIG_KUNIT)
>> +# define HAVE_BUG_FUNCTION
>> +# define __BUG_FUNC_PTR " .long %0-.\n"
>> +# define __BUG_FUNC __func__
>> +#else
>> +# define __BUG_FUNC_PTR
>> +# define __BUG_FUNC NULL
>> +#endif /* IS_ENABLED(CONFIG_KUNIT) */
>> +
>> #define __EMIT_BUG(x) do { \
>> asm_inline volatile( \
>> "0: mc 0,0\n" \
>> ".section .rodata.str,\"aMS\",@progbits,1\n" \
>> "1: .asciz \""__FILE__"\"\n" \
>> ".previous\n" \
>> - ".section __bug_table,\"awM\",@progbits,%2\n" \
>> + ".section __bug_table,\"awM\",@progbits,%3\n" \
>
> This change conflicts with commit 3938490e78f443fb ("s390/bug:
> remove entry size from __bug_table section") in linus/master.
> I guess it should just be dropped?
>
Yes, I know. I'll send v2 rebased to v6.9-rc1 once it is available and,
yes, the change will be gone after that.
Thanks,
Guenter
Hi Günter, On Tue, Mar 12, 2024 at 6:03 PM Guenter Roeck <linux@roeck-us.net> wrote: > Some unit tests intentionally trigger warning backtraces by passing bad > parameters to kernel API functions. Such unit tests typically check the > return value from such calls, not the existence of the warning backtrace. > > Such intentionally generated warning backtraces are neither desirable > nor useful for a number of reasons. > - They can result in overlooked real problems. > - A warning that suddenly starts to show up in unit tests needs to be > investigated and has to be marked to be ignored, for example by > adjusting filter scripts. Such filters are ad-hoc because there is > no real standard format for warnings. On top of that, such filter > scripts would require constant maintenance. > > One option to address problem would be to add messages such as "expected > warning backtraces start / end here" to the kernel log. However, that > would again require filter scripts, it might result in missing real > problematic warning backtraces triggered while the test is running, and > the irrelevant backtrace(s) would still clog the kernel log. > > Solve the problem by providing a means to identify and suppress specific > warning backtraces while executing test code. Support suppressing multiple > backtraces while at the same time limiting changes to generic code to the > absolute minimum. Architecture specific changes are kept at minimum by > retaining function names only if both CONFIG_DEBUG_BUGVERBOSE and > CONFIG_KUNIT are enabled. > > The first patch of the series introduces the necessary infrastructure. > The second patch introduces support for counting suppressed backtraces. > This capability is used in patch three to implement unit tests. > Patch four documents the new API. > The next two patches add support for suppressing backtraces in drm_rect > and dev_addr_lists unit tests. These patches are intended to serve as > examples for the use of the functionality introduced with this series. > The remaining patches implement the necessary changes for all > architectures with GENERIC_BUG support. Thanks for your series! I gave it a try on m68k, just running backtrace-suppression-test, and that seems to work fine. > Design note: > Function pointers are only added to the __bug_table section if both > CONFIG_KUNIT and CONFIG_DEBUG_BUGVERBOSE are enabled to avoid image > size increases if CONFIG_KUNIT=n. There would be some benefits to > adding those pointers all the time (reduced complexity, ability to > display function names in BUG/WARNING messages). That change, if > desired, can be made later. Unfortunately this also increases kernel size in the CONFIG_KUNIT=m case (ca. 80 KiB for atari_defconfig), making it less attractive to have kunit and all tests enabled as modules in my standard kernel. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi Günter, On Tue, Mar 12, 2024 at 6:06 PM Guenter Roeck <linux@roeck-us.net> wrote: > Add name of functions triggering warning backtraces to the __bug_table > object section to enable support for suppressing WARNING backtraces. > > To limit image size impact, the pointer to the function name is only added > to the __bug_table section if both CONFIG_KUNIT and CONFIG_DEBUG_BUGVERBOSE > are enabled. Otherwise, the __func__ assembly parameter is replaced with a > (dummy) NULL parameter to avoid an image size increase due to unused > __func__ entries (this is necessary because __func__ is not a define but a > virtual variable). > > Signed-off-by: Guenter Roeck <linux@roeck-us.net> Thanks for your patch! > --- a/arch/s390/include/asm/bug.h > +++ b/arch/s390/include/asm/bug.h > @@ -8,19 +8,30 @@ > > #ifdef CONFIG_DEBUG_BUGVERBOSE > > +#if IS_ENABLED(CONFIG_KUNIT) > +# define HAVE_BUG_FUNCTION > +# define __BUG_FUNC_PTR " .long %0-.\n" > +# define __BUG_FUNC __func__ > +#else > +# define __BUG_FUNC_PTR > +# define __BUG_FUNC NULL > +#endif /* IS_ENABLED(CONFIG_KUNIT) */ > + > #define __EMIT_BUG(x) do { \ > asm_inline volatile( \ > "0: mc 0,0\n" \ > ".section .rodata.str,\"aMS\",@progbits,1\n" \ > "1: .asciz \""__FILE__"\"\n" \ > ".previous\n" \ > - ".section __bug_table,\"awM\",@progbits,%2\n" \ > + ".section __bug_table,\"awM\",@progbits,%3\n" \ This change conflicts with commit 3938490e78f443fb ("s390/bug: remove entry size from __bug_table section") in linus/master. I guess it should just be dropped? > "2: .long 0b-.\n" \ > " .long 1b-.\n" \ > - " .short %0,%1\n" \ > - " .org 2b+%2\n" \ > + __BUG_FUNC_PTR \ > + " .short %1,%2\n" \ > + " .org 2b+%3\n" \ > ".previous\n" \ > - : : "i" (__LINE__), \ > + : : "i" (__BUG_FUNC), \ > + "i" (__LINE__), \ > "i" (x), \ > "i" (sizeof(struct bug_entry))); \ > } while (0) Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Tue, 12 Mar 2024 at 22:33, Guenter Roeck <linux@roeck-us.net> wrote: <trim> > This series is based on the RFC patch and subsequent discussion at > https://patchwork.kernel.org/project/linux-kselftest/patch/02546e59-1afe-4b08-ba81-d94f3b691c9a@moroto.mountain/ > and offers a more comprehensive solution of the problem discussed there. Thanks for the patchset. This patch series applied on top of Linux next and tested. Tested-by: Linux Kernel Functional Testing <lkft@linaro.org> -- Linaro LKFT https://lkft.linaro.org
Le 12/03/2024 à 23:28, Rick Edgecombe a écrit : > Future changes will need to add a new member to struct > vm_unmapped_area_info. This would cause trouble for any call site that > doesn't initialize the struct. Currently every caller sets each member > manually, so if new members are added they will be uninitialized and the > core code parsing the struct will see garbage in the new member. > > It could be possible to initialize the new member manually to 0 at each > call site. This and a couple other options were discussed, and a working > consensus (see links) was that in general the best way to accomplish this > would be via static initialization with designated member initiators. > Having some struct vm_unmapped_area_info instances not zero initialized > will put those sites at risk of feeding garbage into vm_unmapped_area() if > the convention is to zero initialize the struct and any new member addition > misses a call site that initializes each member manually. > > It could be possible to leave the code mostly untouched, and just change > the line: > struct vm_unmapped_area_info info > to: > struct vm_unmapped_area_info info = {}; > > However, that would leave cleanup for the members that are manually set > to zero, as it would no longer be required. > > So to be reduce the chance of bugs via uninitialized members, instead > simply continue the process to initialize the struct this way tree wide. > This will zero any unspecified members. Move the member initializers to the > struct declaration when they are known at that time. Leave the members out > that were manually initialized to zero, as this would be redundant for > designated initializers. > > Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com> Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu> > Acked-by: Helge Deller <deller@gmx.de> > Cc: "James E.J. Bottomley" <James.Bottomley@HansenPartnership.com> > Cc: Helge Deller <deller@gmx.de> > Cc: linux-parisc@vger.kernel.org > Link: https://lore.kernel.org/lkml/202402280912.33AEE7A9CF@keescook/#t > Link: https://lore.kernel.org/lkml/j7bfvig3gew3qruouxrh7z7ehjjafrgkbcmg6tcghhfh3rhmzi@wzlcoecgy5rs/ > --- > v3: > - Fixed spelling errors in log > - Be consistent about field vs member in log > > Hi, > > This patch was split and refactored out of a tree-wide change [0] to just > zero-init each struct vm_unmapped_area_info. The overall goal of the > series is to help shadow stack guard gaps. Currently, there is only one > arch with shadow stacks, but two more are in progress. It is compile tested > only. > > There was further discussion that this method of initializing the structs > while nice in some ways has a greater risk of introducing bugs in some of > the more complicated callers. Since this version was reviewed my arch > maintainers already, leave it as was already acknowledged. > > Thanks, > > Rick > > [0] https://lore.kernel.org/lkml/20240226190951.3240433-6-rick.p.edgecombe@intel.com/ > --- > arch/parisc/kernel/sys_parisc.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/arch/parisc/kernel/sys_parisc.c b/arch/parisc/kernel/sys_parisc.c > index 98af719d5f85..f7722451276e 100644 > --- a/arch/parisc/kernel/sys_parisc.c > +++ b/arch/parisc/kernel/sys_parisc.c > @@ -104,7 +104,9 @@ static unsigned long arch_get_unmapped_area_common(struct file *filp, > struct vm_area_struct *vma, *prev; > unsigned long filp_pgoff; > int do_color_align; > - struct vm_unmapped_area_info info; > + struct vm_unmapped_area_info info = { > + .length = len > + }; > > if (unlikely(len > TASK_SIZE)) > return -ENOMEM; > @@ -139,7 +141,6 @@ static unsigned long arch_get_unmapped_area_common(struct file *filp, > return addr; > } > > - info.length = len; > info.align_mask = do_color_align ? (PAGE_MASK & (SHM_COLOUR - 1)) : 0; > info.align_offset = shared_align_offset(filp_pgoff, pgoff); > > @@ -160,7 +161,6 @@ static unsigned long arch_get_unmapped_area_common(struct file *filp, > */ > } > > - info.flags = 0; > info.low_limit = mm->mmap_base; > info.high_limit = mmap_upper_limit(NULL); > return vm_unmapped_area(&info);
Thanks! Acked-by: Dan Carpenter <dan.carpenter@linaro.org> regards, dan carpenter
On 03/13/24 at 06:58am, Jiri Slaby wrote: > Hi, > > > On 13. 03. 24, 1:48, Baoquan He wrote: > > Hi Jiri, > > > > On 03/12/24 at 10:58am, Jiri Slaby wrote: > > > On 13. 12. 23, 6:57, Baoquan He wrote: > > ... snip... > > > > --- a/include/linux/kexec.h > > > > +++ b/include/linux/kexec.h > > > ... > > > > @@ -500,6 +500,13 @@ static inline int crash_hotplug_memory_support(void) { return 0; } > > > > static inline unsigned int crash_get_elfcorehdr_size(void) { return 0; } > > > > #endif > > > > +extern bool kexec_file_dbg_print; > > > > + > > > > +#define kexec_dprintk(fmt, ...) \ > > > > + printk("%s" fmt, \ > > > > + kexec_file_dbg_print ? KERN_INFO : KERN_DEBUG, \ > > > > + ##__VA_ARGS__) > > > > > > This means you dump it _always_. Only with different levels. > > > > It dumped always too with pr_debug() before, I just add a switch to > > control it's pr_info() or pr_debug(). > > Not really, see below. > > > > > > > And without any prefix whatsoever, so people see bloat like this in their > > > log now: > > > [ +0.000001] 0000000000001000-000000000009ffff (1) > > > [ +0.000002] 000000007f96d000-000000007f97efff (3) > > > [ +0.000002] 0000000000800000-0000000000807fff (4) > > > [ +0.000001] 000000000080b000-000000000080bfff (4) > > > [ +0.000002] 0000000000810000-00000000008fffff (4) > > > [ +0.000001] 000000007f97f000-000000007f9fefff (4) > > > [ +0.000001] 000000007ff00000-000000007fffffff (4) > > > [ +0.000002] 0000000000000000-0000000000000fff (2) > > > > On which arch are you seeing this? There should be one line above these > > range printing to tell what they are, like: > > > > E820 memmap: > > Ah this is there too. It's a lot of output, so I took it out of context, > apparently. > > > 0000000000000000-000000000009a3ff (1) > > 000000000009a400-000000000009ffff (2) > > 00000000000e0000-00000000000fffff (2) > > 0000000000100000-000000006ff83fff (1) > > 000000006ff84000-000000007ac50fff (2) > > It should all be prefixed like kdump: or kexec: in any way. I can reproduce it now on fedora. OK, I will add kexec or something similar to prefix. Thanks. > > > > without actually knowing what that is. > > > > > > There should be nothing logged if that is not asked for and especially if > > > kexec load went fine, right? > > > > Right. Before this patch, those pr_debug() were already there. You need > > enable them to print out like add '#define DEBUG' in *.c file, or enable > > the dynamic debugging of the file or function. > > I think it's perfectly fine for DEBUG builds to print this out. And many > (all major?) distros use dyndbg, so it used to print nothing by default. > > > With this patch applied, > > you only need specify '-d' when you execute kexec command with > > kexec_file load interface, like: > > > > kexec -s -l -d /boot/vmlinuz-xxxx.img --initrd xxx.img --reuse-cmdline > > Perhaps our (SUSE) tooling passes -d? But I am seeing this every time I > boot. > > No, it does not seem so: > load.sh[915]: Starting kdump kernel load; kexec cmdline: /sbin/kexec -p > /var/lib/kdump/kernel --append=" loglevel=7 console=tty0 console=ttyS0 > video=1920x1080,1024x768,800x600 oops=panic > lsm=lockdown,capability,integrity,selinux sysrq=yes reset_devices > acpi_no_memhotplug cgroup_disable=memory nokaslr numa=off irqpoll nr_cpus=1 > root=kdump rootflags=bind rd.udev.children-max=8 disable_cpu_apicid=0 > panic=1" --initrd=/var/lib/kdump/initrd -a > > > For kexec_file load, it is not logging if not specifying '-d', unless > > you take way to make pr_debug() work in that file. > > So is -d detection malfunctioning under some circumstances? > > > > Can this be redesigned, please? > > > > Sure, after making clear what's going on with this, I will try. > > > > > > > > Actually what was wrong on the pr_debug()s? Can you simply turn them on from > > > the kernel when -d is passed to kexec instead of all this? > > > > Joe suggested this during v1 reviewing: > > https://lore.kernel.org/all/1e7863ec4e4ab10b84fd0e64f30f8464d2e484a3.camel@perches.com/T/#u > > > > > > > > ... > > > > --- a/kernel/kexec_core.c > > > > +++ b/kernel/kexec_core.c > > > > @@ -52,6 +52,8 @@ atomic_t __kexec_lock = ATOMIC_INIT(0); > > > > /* Flag to indicate we are going to kexec a new kernel */ > > > > bool kexec_in_progress = false; > > > > +bool kexec_file_dbg_print; > > > > > > Ugh, and a global flag for this? > > > > Yeah, kexec_file_dbg_print records if '-d' is specified when 'kexec' > > command executed. Anything wrong with the global flag? > > Global variables are frowned upon. To cite coding style: unless you > **really** need them. Here, it looks like you do not. I see your point, will consider and change. Thanks again.
Hi, On 13. 03. 24, 1:48, Baoquan He wrote: > Hi Jiri, > > On 03/12/24 at 10:58am, Jiri Slaby wrote: >> On 13. 12. 23, 6:57, Baoquan He wrote: > ... snip... >>> --- a/include/linux/kexec.h >>> +++ b/include/linux/kexec.h >> ... >>> @@ -500,6 +500,13 @@ static inline int crash_hotplug_memory_support(void) { return 0; } >>> static inline unsigned int crash_get_elfcorehdr_size(void) { return 0; } >>> #endif >>> +extern bool kexec_file_dbg_print; >>> + >>> +#define kexec_dprintk(fmt, ...) \ >>> + printk("%s" fmt, \ >>> + kexec_file_dbg_print ? KERN_INFO : KERN_DEBUG, \ >>> + ##__VA_ARGS__) >> >> This means you dump it _always_. Only with different levels. > > It dumped always too with pr_debug() before, I just add a switch to > control it's pr_info() or pr_debug(). Not really, see below. >> >> And without any prefix whatsoever, so people see bloat like this in their >> log now: >> [ +0.000001] 0000000000001000-000000000009ffff (1) >> [ +0.000002] 000000007f96d000-000000007f97efff (3) >> [ +0.000002] 0000000000800000-0000000000807fff (4) >> [ +0.000001] 000000000080b000-000000000080bfff (4) >> [ +0.000002] 0000000000810000-00000000008fffff (4) >> [ +0.000001] 000000007f97f000-000000007f9fefff (4) >> [ +0.000001] 000000007ff00000-000000007fffffff (4) >> [ +0.000002] 0000000000000000-0000000000000fff (2) > > On which arch are you seeing this? There should be one line above these > range printing to tell what they are, like: > > E820 memmap: Ah this is there too. It's a lot of output, so I took it out of context, apparently. > 0000000000000000-000000000009a3ff (1) > 000000000009a400-000000000009ffff (2) > 00000000000e0000-00000000000fffff (2) > 0000000000100000-000000006ff83fff (1) > 000000006ff84000-000000007ac50fff (2) It should all be prefixed like kdump: or kexec: in any way. >> without actually knowing what that is. >> >> There should be nothing logged if that is not asked for and especially if >> kexec load went fine, right? > > Right. Before this patch, those pr_debug() were already there. You need > enable them to print out like add '#define DEBUG' in *.c file, or enable > the dynamic debugging of the file or function. I think it's perfectly fine for DEBUG builds to print this out. And many (all major?) distros use dyndbg, so it used to print nothing by default. > With this patch applied, > you only need specify '-d' when you execute kexec command with > kexec_file load interface, like: > > kexec -s -l -d /boot/vmlinuz-xxxx.img --initrd xxx.img --reuse-cmdline Perhaps our (SUSE) tooling passes -d? But I am seeing this every time I boot. No, it does not seem so: load.sh[915]: Starting kdump kernel load; kexec cmdline: /sbin/kexec -p /var/lib/kdump/kernel --append=" loglevel=7 console=tty0 console=ttyS0 video=1920x1080,1024x768,800x600 oops=panic lsm=lockdown,capability,integrity,selinux sysrq=yes reset_devices acpi_no_memhotplug cgroup_disable=memory nokaslr numa=off irqpoll nr_cpus=1 root=kdump rootflags=bind rd.udev.children-max=8 disable_cpu_apicid=0 panic=1" --initrd=/var/lib/kdump/initrd -a > For kexec_file load, it is not logging if not specifying '-d', unless > you take way to make pr_debug() work in that file. So is -d detection malfunctioning under some circumstances? >> Can this be redesigned, please? > > Sure, after making clear what's going on with this, I will try. > >> >> Actually what was wrong on the pr_debug()s? Can you simply turn them on from >> the kernel when -d is passed to kexec instead of all this? > > Joe suggested this during v1 reviewing: > https://lore.kernel.org/all/1e7863ec4e4ab10b84fd0e64f30f8464d2e484a3.camel@perches.com/T/#u > >> >> ... >>> --- a/kernel/kexec_core.c >>> +++ b/kernel/kexec_core.c >>> @@ -52,6 +52,8 @@ atomic_t __kexec_lock = ATOMIC_INIT(0); >>> /* Flag to indicate we are going to kexec a new kernel */ >>> bool kexec_in_progress = false; >>> +bool kexec_file_dbg_print; >> >> Ugh, and a global flag for this? > > Yeah, kexec_file_dbg_print records if '-d' is specified when 'kexec' > command executed. Anything wrong with the global flag? Global variables are frowned upon. To cite coding style: unless you **really** need them. Here, it looks like you do not. thanks, -- js suse labs
Hi Jiri, On 03/12/24 at 10:58am, Jiri Slaby wrote: > On 13. 12. 23, 6:57, Baoquan He wrote: ... snip... > > --- a/include/linux/kexec.h > > +++ b/include/linux/kexec.h > ... > > @@ -500,6 +500,13 @@ static inline int crash_hotplug_memory_support(void) { return 0; } > > static inline unsigned int crash_get_elfcorehdr_size(void) { return 0; } > > #endif > > +extern bool kexec_file_dbg_print; > > + > > +#define kexec_dprintk(fmt, ...) \ > > + printk("%s" fmt, \ > > + kexec_file_dbg_print ? KERN_INFO : KERN_DEBUG, \ > > + ##__VA_ARGS__) > > This means you dump it _always_. Only with different levels. It dumped always too with pr_debug() before, I just add a switch to control it's pr_info() or pr_debug(). > > And without any prefix whatsoever, so people see bloat like this in their > log now: > [ +0.000001] 0000000000001000-000000000009ffff (1) > [ +0.000002] 000000007f96d000-000000007f97efff (3) > [ +0.000002] 0000000000800000-0000000000807fff (4) > [ +0.000001] 000000000080b000-000000000080bfff (4) > [ +0.000002] 0000000000810000-00000000008fffff (4) > [ +0.000001] 000000007f97f000-000000007f9fefff (4) > [ +0.000001] 000000007ff00000-000000007fffffff (4) > [ +0.000002] 0000000000000000-0000000000000fff (2) On which arch are you seeing this? There should be one line above these range printing to tell what they are, like: E820 memmap: 0000000000000000-000000000009a3ff (1) 000000000009a400-000000000009ffff (2) 00000000000e0000-00000000000fffff (2) 0000000000100000-000000006ff83fff (1) 000000006ff84000-000000007ac50fff (2) > > without actually knowing what that is. > > There should be nothing logged if that is not asked for and especially if > kexec load went fine, right? Right. Before this patch, those pr_debug() were already there. You need enable them to print out like add '#define DEBUG' in *.c file, or enable the dynamic debugging of the file or function. With this patch applied, you only need specify '-d' when you execute kexec command with kexec_file load interface, like: kexec -s -l -d /boot/vmlinuz-xxxx.img --initrd xxx.img --reuse-cmdline For kexec_file load, it is not logging if not specifying '-d', unless you take way to make pr_debug() work in that file. > > Can this be redesigned, please? Sure, after making clear what's going on with this, I will try. > > Actually what was wrong on the pr_debug()s? Can you simply turn them on from > the kernel when -d is passed to kexec instead of all this? Joe suggested this during v1 reviewing: https://lore.kernel.org/all/1e7863ec4e4ab10b84fd0e64f30f8464d2e484a3.camel@perches.com/T/#u > > ... > > --- a/kernel/kexec_core.c > > +++ b/kernel/kexec_core.c > > @@ -52,6 +52,8 @@ atomic_t __kexec_lock = ATOMIC_INIT(0); > > /* Flag to indicate we are going to kexec a new kernel */ > > bool kexec_in_progress = false; > > +bool kexec_file_dbg_print; > > Ugh, and a global flag for this? Yeah, kexec_file_dbg_print records if '-d' is specified when 'kexec' command executed. Anything wrong with the global flag? Thanks Baoquan
Future changes will need to add a new member to struct vm_unmapped_area_info. This would cause trouble for any call site that doesn't initialize the struct. Currently every caller sets each member manually, so if new members are added they will be uninitialized and the core code parsing the struct will see garbage in the new member. It could be possible to initialize the new member manually to 0 at each call site. This and a couple other options were discussed, and a working consensus (see links) was that in general the best way to accomplish this would be via static initialization with designated member initiators. Having some struct vm_unmapped_area_info instances not zero initialized will put those sites at risk of feeding garbage into vm_unmapped_area() if the convention is to zero initialize the struct and any new member addition misses a call site that initializes each member manually. It could be possible to leave the code mostly untouched, and just change the line: struct vm_unmapped_area_info info to: struct vm_unmapped_area_info info = {}; However, that would leave cleanup for the members that are manually set to zero, as it would no longer be required. So to be reduce the chance of bugs via uninitialized members, instead simply continue the process to initialize the struct this way tree wide. This will zero any unspecified members. Move the member initializers to the struct declaration when they are known at that time. Leave the members out that were manually initialized to zero, as this would be redundant for designated initializers. Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com> Acked-by: Helge Deller <deller@gmx.de> Cc: "James E.J. Bottomley" <James.Bottomley@HansenPartnership.com> Cc: Helge Deller <deller@gmx.de> Cc: linux-parisc@vger.kernel.org Link: https://lore.kernel.org/lkml/202402280912.33AEE7A9CF@keescook/#t Link: https://lore.kernel.org/lkml/j7bfvig3gew3qruouxrh7z7ehjjafrgkbcmg6tcghhfh3rhmzi@wzlcoecgy5rs/ --- v3: - Fixed spelling errors in log - Be consistent about field vs member in log Hi, This patch was split and refactored out of a tree-wide change [0] to just zero-init each struct vm_unmapped_area_info. The overall goal of the series is to help shadow stack guard gaps. Currently, there is only one arch with shadow stacks, but two more are in progress. It is compile tested only. There was further discussion that this method of initializing the structs while nice in some ways has a greater risk of introducing bugs in some of the more complicated callers. Since this version was reviewed my arch maintainers already, leave it as was already acknowledged. Thanks, Rick [0] https://lore.kernel.org/lkml/20240226190951.3240433-6-rick.p.edgecombe@intel.com/ --- arch/parisc/kernel/sys_parisc.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/arch/parisc/kernel/sys_parisc.c b/arch/parisc/kernel/sys_parisc.c index 98af719d5f85..f7722451276e 100644 --- a/arch/parisc/kernel/sys_parisc.c +++ b/arch/parisc/kernel/sys_parisc.c @@ -104,7 +104,9 @@ static unsigned long arch_get_unmapped_area_common(struct file *filp, struct vm_area_struct *vma, *prev; unsigned long filp_pgoff; int do_color_align; - struct vm_unmapped_area_info info; + struct vm_unmapped_area_info info = { + .length = len + }; if (unlikely(len > TASK_SIZE)) return -ENOMEM; @@ -139,7 +141,6 @@ static unsigned long arch_get_unmapped_area_common(struct file *filp, return addr; } - info.length = len; info.align_mask = do_color_align ? (PAGE_MASK & (SHM_COLOUR - 1)) : 0; info.align_offset = shared_align_offset(filp_pgoff, pgoff); @@ -160,7 +161,6 @@ static unsigned long arch_get_unmapped_area_common(struct file *filp, */ } - info.flags = 0; info.low_limit = mm->mmap_base; info.high_limit = mmap_upper_limit(NULL); return vm_unmapped_area(&info); -- 2.34.1
On Tue, Mar 12, 2024 at 10:02:59AM -0700, Guenter Roeck wrote:
> Document API functions for suppressing warning backtraces.
>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
Reviewed-by: Kees Cook <keescook@chromium.org>
--
Kees Cook
On Tue, Mar 12, 2024 at 10:02:58AM -0700, Guenter Roeck wrote:
> Add unit tests to verify that warning backtrace suppression works.
>
> If backtrace suppression does _not_ work, the unit tests will likely
> trigger unsuppressed backtraces, which should actually help to get
> the affected architectures / platforms fixed.
>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
Reviewed-by: Kees Cook <keescook@chromium.org>
--
Kees Cook
On Tue, Mar 12, 2024 at 10:02:57AM -0700, Guenter Roeck wrote: > Count suppressed warning backtraces to enable code which suppresses > warning backtraces to check if the expected backtrace(s) have been > observed. > > Using atomics for the backtrace count resulted in build errors on some > architectures due to include file recursion, so use a plain integer > for now. > > Signed-off-by: Guenter Roeck <linux@roeck-us.net> > --- > include/kunit/bug.h | 7 ++++++- > lib/kunit/bug.c | 4 +++- > 2 files changed, 9 insertions(+), 2 deletions(-) > > diff --git a/include/kunit/bug.h b/include/kunit/bug.h > index 1e34da961599..2097a854ac8c 100644 > --- a/include/kunit/bug.h > +++ b/include/kunit/bug.h > @@ -20,6 +20,7 @@ > struct __suppressed_warning { > struct list_head node; > const char *function; > + int counter; Thanks for adding a counter! > }; > > void __start_suppress_warning(struct __suppressed_warning *warning); > @@ -28,7 +29,7 @@ bool __is_suppressed_warning(const char *function); > > #define DEFINE_SUPPRESSED_WARNING(func) \ > struct __suppressed_warning __kunit_suppress_##func = \ > - { .function = __stringify(func) } > + { .function = __stringify(func), .counter = 0 } > > #define START_SUPPRESSED_WARNING(func) \ > __start_suppress_warning(&__kunit_suppress_##func) > @@ -39,12 +40,16 @@ bool __is_suppressed_warning(const char *function); > #define IS_SUPPRESSED_WARNING(func) \ > __is_suppressed_warning(func) > > +#define SUPPRESSED_WARNING_COUNT(func) \ > + (__kunit_suppress_##func.counter) > + > #else /* CONFIG_KUNIT */ > > #define DEFINE_SUPPRESSED_WARNING(func) > #define START_SUPPRESSED_WARNING(func) > #define END_SUPPRESSED_WARNING(func) > #define IS_SUPPRESSED_WARNING(func) (false) > +#define SUPPRESSED_WARNING_COUNT(func) (0) > > #endif /* CONFIG_KUNIT */ > #endif /* __ASSEMBLY__ */ > diff --git a/lib/kunit/bug.c b/lib/kunit/bug.c > index f93544d7a9d1..13b3d896c114 100644 > --- a/lib/kunit/bug.c > +++ b/lib/kunit/bug.c > @@ -32,8 +32,10 @@ bool __is_suppressed_warning(const char *function) > return false; > > list_for_each_entry(warning, &suppressed_warnings, node) { > - if (!strcmp(function, warning->function)) > + if (!strcmp(function, warning->function)) { > + warning->counter++; > return true; > + } > } > return false; > } > -- > 2.39.2 > Reviewed-by: Kees Cook <keescook@chromium.org> -- Kees Cook