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

Message-Id: <20241001-cleanup-if_not_cond_guard-v1-0-7753810b0f7a@baylibre.com>
Date: Tue, 01 Oct 2024 17:30:17 -0500
From: David Lechner <dlechner@...libre.com>
To: 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: [PATCH 0/3] cleanup: add if_not_cond_guard macro

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.

As example users, I've include a modified version of [2] from the
rejected series and an ADC patch that shows how this avoids the
unreachable() and too much indentation issues in the IIO subsystem.

[2]: https://lore.kernel.org/all/170905254443.2268463.935306988251313983.stgit@dwillia2-xfh.jf.intel.com/

---
David Lechner (3):
      cleanup: add conditional guard helper
      iio: adc: ad7380: use if_not_cond_guard for claim direct
      cxl/region: Use cond_guard() in show_targetN()

 drivers/cxl/core/region.c | 18 ++++--------
 drivers/iio/adc/ad7380.c  | 70 +++++++++++++++++++++++------------------------
 include/linux/cleanup.h   | 11 ++++++++
 3 files changed, 51 insertions(+), 48 deletions(-)
---
base-commit: 431c39f6d3edbab14f48dbf37a58ccdc0ac3be1e
change-id: 20241001-cleanup-if_not_cond_guard-0981d867ddf8

Best regards,
-- 
David Lechner <dlechner@...libre.com>