linux-kernel - Re: [PATCH v1 1/1] platform/x86: ideapad-laptop: Make the scope

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240904012242.GA1110859@thelio-3990X>
Date: Tue, 3 Sep 2024 18:22:42 -0700
From: Nathan Chancellor <nathan@...nel.org>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc: Gergo Koteles <soyer@....hu>,
	Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
	Hans de Goede <hdegoede@...hat.com>,
	platform-driver-x86@...r.kernel.org, linux-kernel@...r.kernel.org,
	Ike Panhc <ike.pan@...onical.com>,
	Peter Zijlstra <peterz@...radead.org>,
	Josh Poimboeuf <jpoimboe@...nel.org>,
	kernel test robot <lkp@...el.com>
Subject: Re: [PATCH v1 1/1] platform/x86: ideapad-laptop: Make the
 scope_guard() clear of its scope

Hi Andy,

On Tue, Sep 03, 2024 at 06:40:16PM +0300, Andy Shevchenko wrote:
> On Tue, Sep 03, 2024 at 05:29:02PM +0200, Gergo Koteles wrote:
> > On Tue, 2024-09-03 at 18:14 +0300, Andy Shevchenko wrote:
> > > On Tue, Sep 03, 2024 at 05:00:51PM +0200, Gergo Koteles wrote:
> > > > On Thu, 2024-08-29 at 19:50 +0300, Andy Shevchenko wrote:
> > > > > First of all, it's a bit counterintuitive to have something like
> > > > > 
> > > > > 	int err;
> > > > > 	...
> > > > > 	scoped_guard(...)
> > > > > 		err = foo(...);
> > > > > 	if (err)
> > > > > 		return err;
> > > > > 
> > > > > Second, with a particular kernel configuration and compiler version in
> > > > > one of such cases the objtool is not happy:
> > > > > 
> > > > >   ideapad-laptop.o: warning: objtool: .text.fan_mode_show: unexpected end of section
> > > > > 
> > > > > I'm not an expert on all this, but the theory is that compiler and
> > > > > linker in this case can't understand that 'result' variable will be
> > > > > always initialized as long as no error has been returned. Assigning
> > > > > 'result' to a dummy value helps with this. Note, that fixing the
> > > > > scoped_guard() scope (as per above) does not make issue gone.
> > > > > 
> > > > > That said, assign dummy value and make the scope_guard() clear of its scope.
> > > > > For the sake of consistency do it in the entire file.
> > > > > 
> > > > 
> > > > Interestingly, if I open a scope manually and use the plain guard, the
> > > > warning disappears.
> > > 
> > > Yes, that's what I also have, but I avoid that approach because in that case
> > > the printing will be done inside the lock, widening the critical section for
> > > no benefits.
> > > 
> > 
> > This is intended to be an inner block scope within the function, it
> > does not expand the critical section.
> 
> I'm not sure I understand.
> 
> scoped_guard() has a marked scope (with {} or just a line coupled with it).
> The guard() has a scope starting at it till the end of the function. In the
> latter case the sysfs_emit() becomes part of the critical section.
> 
> > > > 	unsigned long result;
> > > > 	int err;
> > > > 
> > > > 	{
> > > > 		guard(mutex)(&priv->vpc_mutex);
> > > > 		err = read_ec_data(priv->adev->handle, VPCCMD_R_FAN,
> > > > &result);
> > > > 		if (err)
> > > > 			return err;
> > > > 	}
> 
> But looking again into the code above now I got what you meant.
> You have added a nested scope inside the function, like
> 
> 	do {
> 		...
> 	} while (0);
> 
> Yes, this is strange and not what we want to have either. So I prefer to hear
> what objtool / clang people may comment on this.

So this does not appear to happen when CONFIG_KCOV is disabled with the
configuration from the original report. I have spent some time looking
at the disassembly but I am a little out of my element there. If I
remember correctly, the "unexpected end of section" warning from objtool
can appear when optimizations play fast and loose with the presence of
potential undefined behavior (or cannot prove that there is no undefined
behavior through inlining or analysis). In this case, I wonder if KCOV
prevents LLVM from realizing that the for loop that scoped_guard()
results in will run at least once, meaning that err and result would be
potentially used uninitialized? That could explain why this change
resolves the warning, as it ensures that no undefined behavior could
happen regardless of whether or not the loop runs?

Josh and Peter may have more insight.

Cheers,
Nathan