linux-kernel - Re: [PATCH 0/3] cleanup: add if_not_cond

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] [day] [month] [year] [list]

Message-ID: <66fcac2dbde60_964f229426@dwillia2-xfh.jf.intel.com.notmuch>
Date: Tue, 1 Oct 2024 19:13:01 -0700
From: Dan Williams <dan.j.williams@...el.com>
To: David Lechner <dlechner@...libre.com>, Peter Zijlstra
	<peterz@...radead.org>, Dan Williams <dan.j.williams@...el.com>, "Linus
 Torvalds" <torvalds@...ux-foundation.org>, Jonathan Cameron
	<jic23@...nel.org>
CC: Nuno Sá <nuno.sa@...log.com>, Michael Hennerich
	<michael.hennerich@...log.com>, <linux-kernel@...r.kernel.org>,
	<linux-iio@...r.kernel.org>, <linux-cxl@...r.kernel.org>, David Lechner
	<dlechner@...libre.com>, Ira Weiny <ira.weiny@...el.com>, "Fabio M. De
 Francesco" <fabio.maria.de.francesco@...ux.intel.com>
Subject: Re: [PATCH 0/3] cleanup: add if_not_cond_guard macro

David Lechner wrote:
> So far, I have not found scoped_cond_guard() to be nice to work with.
> We have been using it quite a bit in the IIO subsystem via the
> iio_device_claim_direct_scoped() macro.
> 
> The main thing I don't like is that scoped_cond_guard() uses a for loop
> internally. In the IIO subsystem, we usually try to return as early as
> possible, so often we are returning from all paths from withing this
> hidden for loop. However, since it is a for loop, the compiler thinks
> that it possible to exit the for loop and so we end up having to use
> unreachable() after the end of the scope to avoid a compiler warning.
> This is illustrated in the ad7380 patch in this series and there are 36
> more instance of unreachable() already introduced in the IIO subsystem
> because of this.
> 
> Also, scoped_cond_guard() is they only macro for conditional guards in
> cleanup.h currently. This means that so far, patches adopting this are
> generally converting something that wasn't scoped to be scoped. This
> results in changing the indentation of a lot of lines of code, which is
> just noise in the patches.
> 
> To avoid these issues, the natural thing to do would be to have a
> non-scoped version of the scoped_cond_guard() macro. There was was a
> rejected attempt to do this in [1], where one of the complaints was:
> 
> > > -       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.
> 
> [1]: https://lore.kernel.org/all/170905252721.2268463.6714121678946763402.stgit@dwillia2-xfh.jf.intel.com/
> 
> I couldn't find a way to make a cond_guard() macro that would work like
> exactly as suggested (the problem is that you can't declare a variable
> in the condition expression of an if statement in C). So I am proposing
> a macro that reads basically the same as the above so it still reads
> almost like normal C code even though it hides the if statement a bit.
> 
>         if_not_cond_guard(rwsem_read_intr, &cxl_region_rwsem)
>                 return -EINTR;
> 
> The "not" is baked into the macro because in most cases, failing to
> obtain the lock is the abnormal condition and generally we want to have
> the abnormal path be the indented one.

I think you could also take the "cond" out of the name because that is
implied by the fact it's an 'if'.

So, calling this "if_not_guard ()", for the series, you can add:

Reviewed-by: Dan Williams <dan.j.williams@...el.com>

...but it's ultimately up to Peter and Linus if they find this "if ()"
rename acceptable. If it is I would suggest the style should be treat it
as an "if ()" statement and add this to .clang-format:

diff --git a/.clang-format b/.clang-format
index 252820d9c80a..ae3511a69896 100644
--- a/.clang-format
+++ b/.clang-format
@@ -63,6 +63,8 @@ DerivePointerAlignment: false
 DisableFormat: false
 ExperimentalAutoDetectBinPacking: false
 FixNamespaceComments: false
+IfMacros:
+  - 'if_not_guard'
 
 # Taken from:
 #   git grep -h '^#define [^[:space:]]*for_each[^[:space:]]*(' include/ tools/ \


Last note, while the iio conversion looks correct to me, I would feel
more comfortable if there was a way to have the compiler catch that
plain "guard(iio_claim_direct)" usage is broken. Perhaps declaring
iio_device_claim_direct_mode() as __must_check achieves that effect?