netdev - Re: [RFC PATCH] cleanup: make scoped

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: <ZvqgVeOe9jE02b1r@google.com>
Date: Mon, 30 Sep 2024 05:57:57 -0700
From: Dmitry Torokhov <dmitry.torokhov@...il.com>
To: Przemek Kitszel <przemyslaw.kitszel@...el.com>
Cc: Dan Carpenter <dan.carpenter@...aro.org>, linux-kernel@...r.kernel.org,
	Peter Zijlstra <peterz@...radead.org>,
	amadeuszx.slawinski@...ux.intel.com,
	Tony Nguyen <anthony.l.nguyen@...el.com>,
	nex.sw.ncis.osdt.itp.upstreaming@...el.com, netdev@...r.kernel.org,
	Andy Shevchenko <andriy.shevchenko@...el.com>
Subject: Re: [RFC PATCH] cleanup: make scoped_guard() to be return-friendly

On Mon, Sep 30, 2024 at 01:30:58PM +0200, Przemek Kitszel wrote:
> On 9/30/24 13:08, Dan Carpenter wrote:
> > On Mon, Sep 30, 2024 at 12:21:44PM +0200, Przemek Kitszel wrote:
> > > 
> > > Most of the time it is just easier to bend your driver than change or
> > > extend the core of the kernel.
> > > 
> > > There is actually scoped_cond_guard() which is a trylock variant.
> > > 
> > > scoped_guard(mutex_try, &ts->mutex) you have found is semantically
> > > wrong and must be fixed.
> > 
> > What?  I'm so puzzled by this conversation.
> 
> there are two variants of scoped_guard() and you have found a place
> where the wrong one is used

"Yeah? Well, you know, that's just like uh, your opinion, man."

>From include/linux/cleanup.h:

 * scoped_guard (name, args...) { }:
 *	similar to CLASS(name, scope)(args), except the variable (with the
 *	explicit name 'scope') is declard in a for-loop such that its scope is
 *	bound to the next (compound) statement.
 *
 *	for conditional locks the loop body is skipped when the lock is not
 *	acquired.

Please note the 2nd paragraph that explains this particular usage and
that it was done this way on purpose.

> 
> > 
> > Anyway, I don't have a problem with your goal, but your macro is wrong and will
> > need to be re-written.  You will need to update any drivers which use the
> > scoped_guard() for try locks.  I don't care how you do that.  Use
> > scoped_cond_guard() if you want or invent a new macro.  But that work always
> > falls on the person changing the API.  Plus, it's only the one tsc200x-core.c
> > driver so I don't understand why you're making a big deal about it.

I think if you also count uses of "scoped_guard(mutex_intr, ...)" you
will find more of such examples.

> 
> apologies for upsetting you
> I will send next iteration of this series with additional patches fixing
> current code (thanks you for finding it for me in this case!)

No, please do not. Your "fix" it looks like will prevent writing
code like:

	scoped_guard(mutex_intr, &some_mutex) {
		do_stuff();

		return 0;
	}

	return -EINTR;

You might not like it, but it is a valid pattern.

> 
> I didn't said so in prev mail to leave you an option to send the fix for
> the usage bug you have reported, just confirmed it. But by all means I'm
> happy to fix current code myself.
> 
> > but your macro is wrong and will need to be re-written
> 
> could you please elaborate here?
i
Dan explained that you are changing the behavior of the guards, in an
undesirable way, breaking users. Please re-read what was written before.

Thanks.

-- 
Dmitry