Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 10 Sep 2021 09:00:26 -0500
From:   Andrew Halaney <ahalaney@redhat.com>
To:     Jason Baron <jbaron@akamai.com>
Cc:     linux-kernel@vger.kernel.org, Jim Cromie <jim.cromie@gmail.com>
Subject: Re: [PATCH] dyndbg: make dyndbg a known cli param
Message-ID: <20210910140026.tufn25xf5j4tzbtx@halaneylaptop>
References: <20210909161755.61743-1-ahalaney@redhat.com>
 <0849504a-b7ed-f448-6d66-1a9fd5ad70ce@akamai.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <0849504a-b7ed-f448-6d66-1a9fd5ad70ce@akamai.com>
Precedence: bulk

On Thu, Sep 09, 2021 at 05:34:51PM -0400, Jason Baron wrote:
> 
> 
> On 9/9/21 12:17 PM, Andrew Halaney wrote:
> > Right now dyndbg shows up as an unknown parameter if used on boot:
> > 
> >     Unknown command line parameters: dyndbg=module params +p ; module sys +p
> > 
> > That's because it is unknown, it doesn't sit in the __param
> > section, so the processing done to warn users supplying an unknown
> > parameter doesn't think it is legitimate.
> > 
> > Install a dummy handler to register it. This was chosen instead of the
> > approach the (deprecated) ddebug_query param takes, which is to
> > have a real handler that copies the string taking up a KiB memory.
> > 
> > Fixes: 86d1919a4fb0 ("init: print out unknown kernel parameters")
> > Signed-off-by: Andrew Halaney <ahalaney@redhat.com>
> > ---
> > 
> > This is the last valid param I know of that was getting flagged on boot
> > if used correctly. Please let me know if the alternative approach of
> > actually copying the string is preferred and I'll spin that up instead.
> > 
> 
> Hi Andrew,
> 
> So I think you are referring to the string copying that ddebug_query= does.
> I don't think that works for 'dyndbg=' b/c its actually looking through
> the entire command line for stuff like <module_name>.dyndbg="blah".
> 
> So I think what you prposed below makes sense, we could perhaps add a note
> as to why it's a noop. As I mentioned it needs to look through the entire
> string.
> 

I think that the ddebug_query= style works fine for dyndbg=, and that
things like <module_name>.dyndbg= go through a different path to
actually have the work done.

When dyndbg= is processed through dynamic_debug_init() all of the
<module_name>.dyndbg= style queries don't do anything ultimately.

When the module is actually loaded unknown_module_param_cb() checks for
the module's dyndbg query and calls ddebug_dyndbg_module_param_cb()
directly. It is at this point that I believe the query is really
applied.

For example, this is what I see in dmesg:

    [    0.045553] Kernel command line: BOOT_IMAGE=(hd0,msdos1)/vmlinuz-5.14.0+ root=UUID=9a0e5b84-1190-465f-a587-f2f2a00a8e91 ro rhgb quiet "dyndbg=module params +p ; module sys +p" dynamic_debug.verbose=2 kvm.dyndbg=+pflmt

    # Initial dynamic_debug processing..
    [    0.114308] dyndbg:   6 debug prints in module main
    [    0.114308] dyndbg:   1 debug prints in module initramfs
    <snip (no kvm debug prints... since it is a module and not loaded)>

    # Processing normal dyndbg= for builtins, param has matches but sys
    # does not
    [    0.114308] dyndbg: dyndbg="module params +p ; module sys +p"
    [    0.114308] dyndbg: query 0: "module params +p "
    <snip>
    [    0.114308] dyndbg: applied: func="" file="" module="params" format="" lineno=0-0
    [    0.114308] dyndbg: query 1: "module sys +p"
    [    0.114308] dyndbg: no-match: func="" file="" module="sys" format="" lineno=0-0
    [    0.114308] dyndbg: processed 2 queries, with 4 matches, 0 errs

    # Trying to process kvm.dyndbg=, but no effect as module isn't loaded
    [    0.114308] doing dyndbg params: kvm.dyndbg='+pflmt'
    [    0.114308] dyndbg: kvm.dyndbg="+pflmt"
    <snip>
    [    0.114308] dyndbg: processed 1 queries, with 0 matches, 0 errs

    # kvm module is loaded, kernel/module.c calls ddebug_dyndbg_module_param_cb()
    # this time it does something
    [    3.651764] doing kvm, parsing ARGS: 'dyndbg=+pflmt '
    [    3.651767] doing kvm: dyndbg='+pflmt'
    [    3.651768] dyndbg: module: kvm dyndbg="+pflmt"
    [    3.651769] dyndbg: query 0: "+pflmt"
    <snip>
    [    3.652414] dyndbg: applied: func="" file="" module="kvm" format="" lineno=0-0
    [    3.652416] dyndbg: processed 1 queries, with 10 matches, 0 errs

With that being said, I think ddebug_query= style of copying the
dyndbg="string" is all that is really needed for processing builtins,
and the module loading method that's already in place shouldn't be
affected. Does that change you opinion of the approach (or am I totally
misunderstanding)? Processing the whole cli seems unnecessary, but I'm
also a (unjustified?) stickler for adding additional memory usage in the
patch.

> 
> > Sort of an aside, but what's the policy for removing deprecated cli
> > params? ddebug_query has been deprecated for a very long time now, but I
> > am not sure if removing params like that is considered almost as bad as
> > breaking userspace considering some systems might update their kernels
> > but not the bootloader supplying the param.
> 
> I think it's probably ok to remove at this point, especially now that
> we are going to flag it as unknown, right? So I feel like that change
> can logically go with this series if you want as a separate patch.

ddebug_query actually _is_ known (it registers with __setup() so there's
no warning like the one I listed in the commit description (although the
deprecated message in lib/dynamic_debug.c is present if you actually use
it). So I sort of feel like the removal would be separate to this
patchset. Let me know when your thoughts when you answer the prior
question and I'll react with a v2 accordingly!

> 
> Thanks,
> 
> -Jason
> 
> 
> > 
> >  lib/dynamic_debug.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> > index cb5abb42c16a..84c16309cc63 100644
> > --- a/lib/dynamic_debug.c
> > +++ b/lib/dynamic_debug.c
> > @@ -761,6 +761,18 @@ static __init int ddebug_setup_query(char *str)
> >  
> >  __setup("ddebug_query=", ddebug_setup_query);
> >  
> > +/*
> > + * Install a noop handler to make dyndbg look like a normal kernel cli param.
> > + * This avoids warnings about dyndbg being an unknown cli param when supplied
> > + * by a user.
> > + */
> > +static __init int dyndbg_setup(char *str)
> > +{
> > +	return 1;
> > +}
> > +
> > +__setup("dyndbg=", dyndbg_setup);
> > +
> >  /*
> >   * File_ops->write method for <debugfs>/dynamic_debug/control.  Gathers the
> >   * command text from userspace, parses and executes it.
> > 
>