Message-ID: <8ca08fba-1120-ca86-6129-0b33afb4a1da@akamai.com>
Date:   Thu, 16 Feb 2023 12:21:06 -0500
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.4.2
Subject: Re: [PATCH v2 20/20] jump_label: RFC - tolerate toggled state
To:     Peter Zijlstra <peterz@infradead.org>,
        Jim Cromie <jim.cromie@gmail.com>
Cc:     linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org,
        amd-gfx@lists.freedesktop.org, intel-gvt-dev@lists.freedesktop.org,
        intel-gfx@lists.freedesktop.org, jani.nikula@intel.com,
        ville.syrjala@linux.intel.com, daniel.vetter@ffwll.ch,
        seanpaul@chromium.org, robdclark@gmail.com,
        gregkh@linuxfoundation.org
References: <20230113193016.749791-1-jim.cromie@gmail.com>
 <20230113193016.749791-21-jim.cromie@gmail.com>
 <Y8aNMxHpvZ8qecSc@hirez.programming.kicks-ass.net>
Content-Language: en-US
From:   Jason Baron <jbaron@akamai.com>
In-Reply-To: <Y8aNMxHpvZ8qecSc@hirez.programming.kicks-ass.net>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 1/17/23 6:57 AM, Peter Zijlstra wrote:
> On Fri, Jan 13, 2023 at 12:30:16PM -0700, Jim Cromie wrote:
>> __jump_label_patch currently will "crash the box" if it finds a
>> jump_entry not as expected.  ISTM this overly harsh; it doesn't
>> distinguish between "alternate/opposite" state, and truly
>> "insane/corrupted".
>>
>> The "opposite" (but well-formed) state is a milder mis-initialization
>> problem, and some less severe mitigation seems practical.  ATM this
>> just warns about it; a range/enum of outcomes: warn, crash, silence,
>> ok, fixup-continue, etc, are possible on a case-by-case basis.
>>
>> Ive managed to create this mis-initialization condition with
>> test_dynamic_debug.ko & _submod.ko.  These replicate DRM's regression
>> on DRM_USE_DYNAMIC_DEBUG=y; drm.debug callsites in drivers/helpers
>> (dependent modules) are not enabled along with those in drm.ko itself.
>>
> 
>> Ive hit this case a few times, but havent been able to isolate the
>> when and why.
>>
>> warn-only is something of a punt, and I'm still left with remaining
>> bugs which are likely related; I'm able to toggle the p-flag on
>> callsites in the submod, but their enablement still doesn't yield
>> logging activity.
> 
> Right; having been in this is state is bad since it will generate
> inconsistent code-flow. Full on panic *might* not be warranted (as it
> does for corrupted text) but it is still a fairly bad situation -- so
> I'm not convinced we want to warn and carry on.
> 
> It would be really good to figure out why the site was skipped over and
> got out of skew.
> 
> Given it's all module stuff, the 'obvious' case would be something like
> a race between adding the new sites and flipping it, but I'm not seeing
> how -- things are rather crudely serialized by jump_label_mutex.

Indeed, looks like dynamic debug introduces a new path in this series to 
that tries to toggle the jump label sites before they have been 
initialized, which ends up with the jump label key enabled but only some 
of the sites in the correct state. Then when the key is subsequently 
disabled it finds some in the wrong state. I just posted a patch for 
dynamic debug to use the module callback notifiers so it's ordered 
properly against jump label.

Note this isn't an issue in the current tree b/c there is no path to 
toggle the sites currently before they are initialized, but Jim's series 
here adds such a path.

Thanks,

-Jason


> 
> The only other option I can come up with is that somehow the update
> condition in jump_label_add_module() is somehow wrong.
>