Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Message-ID: <07aba584-ce62-ed58-29b5-0a2df78b7f1d@akamai.com>
Date:   Fri, 11 Mar 2022 13:03:37 -0500
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.0
Subject: Re: [PATCH 1/5] dyndbg: fix static_branch manipulation
Content-Language: en-US
To:     Jim Cromie <jim.cromie@gmail.com>, gregkh@linuxfoundation.org,
        linux-kernel@vger.kernel.org
Cc:     daniel.vetter@ffwll.ch, seanpaul@chromium.org, robdclark@gmail.com,
        linux@rasmusvillemoes.dk, joe@perches.com,
        dri-devel@lists.freedesktop.org, amd-gfx@lists.freedesktop.org,
        intel-gvt-dev@lists.freedesktop.org,
        intel-gfx@lists.freedesktop.org, vincent.whitchurch@axis.com
References: <20220311044756.425777-1-jim.cromie@gmail.com>
 <20220311044756.425777-2-jim.cromie@gmail.com>
From:   Jason Baron <jbaron@akamai.com>
In-Reply-To: <20220311044756.425777-2-jim.cromie@gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 3/10/22 23:47, Jim Cromie wrote:
> In https://urldefense.com/v3/__https://lore.kernel.org/lkml/20211209150910.GA23668@axis.com/__;!!GjvTz_vk!HGKKoni4RVdEBgv_V0zPSNSX428bpf02zkCy2WbeQkBdVtp1QJqGX-lJYlRDGg$ 
> 
> Vincent's patch commented on, and worked around, a bug toggling
> static_branch's, when a 2nd PRINTK-ish flag was added.  The bug
> results in a premature static_branch_disable when the 1st of 2 flags
> was disabled.
> 
> The cited commit computed newflags, but then in the JUMP_LABEL block,
> failed to use that result, instead using just one of the terms in it.
> Using newflags instead made the code work properly.
> 
> This is Vincents test-case, reduced.  It needs the 2nd flag to work
> properly, but it's explanatory here.
> 
> pt_test() {
>     echo 5 > /sys/module/dynamic_debug/verbose
> 
>     site="module tcp" # just one callsite
>     echo " $site =_ " > /proc/dynamic_debug/control # clear it
> 
>     # A B ~A ~B
>     for flg in +T +p "-T #broke here" -p; do
> 	echo " $site $flg " > /proc/dynamic_debug/control
>     done;
> 
>     # A B ~B ~A
>     for flg in +T +p "-p #broke here" -T; do
> 	echo " $site $flg " > /proc/dynamic_debug/control
>     done
> }
> pt_test
> 
> Fixes: 84da83a6ffc0 dyndbg: combine flags & mask into a struct, simplify with it
> CC: vincent.whitchurch@axis.com
> Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
> 
> --
> .drop @stable, no exposed bug.
> ---
>  lib/dynamic_debug.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> index dd7f56af9aed..a56c1286ffa4 100644
> --- a/lib/dynamic_debug.c
> +++ b/lib/dynamic_debug.c
> @@ -211,10 +211,11 @@ static int ddebug_change(const struct ddebug_query *query,
>  				continue;
>  #ifdef CONFIG_JUMP_LABEL
>  			if (dp->flags & _DPRINTK_FLAGS_PRINT) {
> -				if (!(modifiers->flags & _DPRINTK_FLAGS_PRINT))
> +				if (!(newflags & _DPRINTK_FLAGS_PRINT))
>  					static_branch_disable(&dp->key.dd_key_true);
> -			} else if (modifiers->flags & _DPRINTK_FLAGS_PRINT)
> +			} else if (newflags & _DPRINTK_FLAGS_PRINT) {
>  				static_branch_enable(&dp->key.dd_key_true);
> +			}
>  #endif
>  			dp->flags = newflags;
>  			v4pr_info("changed %s:%d [%s]%s =%s\n",


Hi Jim,

If iiuc this is currently a bug but could be if we add a second 'print' bit
such as for printing to the tracing logs. That said I agree that using 'newflags'
here makes the code more straightforward/readable. So this one is fine with
me.

Acked-by: Jason Baron <jbaron@akamai.com>

Thanks,

-Jason