linux-kernel - Re: [PATCH 05/26] cxl/core: Simplify cxl_dpa_set

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]

Date: Fri, 3 May 2024 18:19:41 -0700
From: Dan Williams <dan.j.williams@...el.com>
To: Ira Weiny <ira.weiny@...el.com>, Alison Schofield
	<alison.schofield@...el.com>
CC: Dave Jiang <dave.jiang@...el.com>, Fan Ni <fan.ni@...sung.com>, "Jonathan
 Cameron" <Jonathan.Cameron@...wei.com>, Navneet Singh
	<navneet.singh@...el.com>, Dan Williams <dan.j.williams@...el.com>, Davidlohr
 Bueso <dave@...olabs.net>, Vishal Verma <vishal.l.verma@...el.com>,
	<linux-btrfs@...r.kernel.org>, <linux-cxl@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 05/26] cxl/core: Simplify cxl_dpa_set_mode()

Ira Weiny wrote:
> Alison Schofield wrote:
> > On Sun, Mar 24, 2024 at 04:18:08PM -0700, Ira Weiny wrote:
> > > cxl_dpa_set_mode() checks the mode for validity two times, once outside
> > > of the DPA RW semaphore and again within.
> > 
> > Not true.
> 
> Sorry for not being clear.  It does check the mode 2x but not for
> validity.  I'll clarify.
> 
> > It only checks mode once before the lock. It checks for
> > capacity after the lock. If it didn't check mode before the lock,
> > then unsupported modes would fall through.
> 
> But we can check the mode 1 time and either check the size or fail.
> 
> > 
> > > The function is not in a critical path.
> > 
> > Implying what here?  OK to check twice (even though it wasn't)
> > or OK to expand scope of locking.
> 
> Implying that checking the mode outside the lock is not required.

The @mode check outside the lock is there to taking the lock when not
necessary because the passed in mode is already bogus.

The lock is about making sure the write of cxled->mode relative to the
state of the dpa partitions is an atomic check-and-set.

So this makes the function unconditionally take the lock when it might
be bogus to do so. The value of reorganizing this is questionable.