linux-kernel - Re: [PATCH 3/3] cxl/region: Use cond_guard() in show

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: <65de5725824ba_1138c72948c@dwillia2-xfh.jf.intel.com.notmuch>
Date: Tue, 27 Feb 2024 13:41:57 -0800
From: Dan Williams <dan.j.williams@...el.com>
To: Linus Torvalds <torvalds@...ux-foundation.org>, Dan Williams
	<dan.j.williams@...el.com>
CC: <peterz@...radead.org>, <gregkh@...uxfoundation.org>, Ira Weiny
	<ira.weiny@...el.com>, Dave Jiang <dave.jiang@...el.com>, Jonathan Cameron
	<Jonathan.Cameron@...wei.com>, "Fabio M. De Francesco"
	<fabio.maria.de.francesco@...ux.intel.com>, <linux-kernel@...r.kernel.org>,
	<linux-cxl@...r.kernel.org>
Subject: Re: [PATCH 3/3] cxl/region: Use cond_guard() in show_targetN()

Linus Torvalds wrote:
> On Tue, 27 Feb 2024 at 08:49, Dan Williams <dan.j.williams@...el.com> wrote:
> >
> > -       rc = down_read_interruptible(&cxl_region_rwsem);
> > -       if (rc)
> > -               return rc;
> > +       cond_guard(rwsem_read_intr, return -EINTR, &cxl_region_rwsem);
> 
> Yeah, this is an example of how NOT to do things.
> 
> If you can't make the syntax be something clean and sane like
> 
>         if (!cond_guard(rwsem_read_intr, &cxl_region_rwsem))
>                 return -EINTR;
> 
> then this code should simply not be converted to guards AT ALL.
> 
> Note that we have a perfectly fine way to do conditional lock guarding
> by simply using helper functions, which actually makes code MORE
> READABLE:
> 
>         if (!down_read_interruptible(&cxl_region_rwsem))
>                 return -EINTR;
>         rc = do_locked_function();
>         up_read(&cxl_region_rwsem);
>         return rc;
> 
> and notice how there are no special cases, no multiple unlocks, no
> NOTHING. And the syntax is clean.

Ok, I took the wrong lessons from scoped_cond_guard(). Consider it
dropped.

> Honestly, if people are going to use 'guard' to write crap code, we
> need to really stop that in its tracks.
> 
> There is no upside to making up new interfaces that only generate garbage.
> 
> This is final. I'm not willing to even entertain this kind of crap.

I will also note that these last 3 statements, nuking the proposal from
space, I find excessive. Yes, on the internet no one can hear you being
subtle, but the "MORE READABLE" and "NOTHING" were pretty darn
unequivocal, especially coming from the person who has absolute final
say on what enters his project.

I have been around a while so I take this as par for the course, and I
appreciate blunt feedback. I have had dance teachers tell me my "dancing
is shit", and sometimes that level of bluntness is needed, but that was
also from somebody I have worked with for years. Fabio has not been
around that long, and nothing about what Fabio did was crap, he carried
through on an idea that I asked him to explore and it did not work out.

So Fabio, keep going, thank you for patiently working through this
investigation and my takeaway is that we have successfully discovered
one way this mission to cleanup usage of goto in the CXL subsystem can
not proceed. Back to the drawing board.