linux-kernel - Re: [PATCH v3] cxl/region: Remove lock from memory notifier callback

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: <66e48742afe0f_e45da29448@iweiny-mobl.notmuch>
Date: Fri, 13 Sep 2024 13:41:06 -0500
From: Ira Weiny <ira.weiny@...el.com>
To: Jonathan Cameron <Jonathan.Cameron@...wei.com>, Ira Weiny
	<ira.weiny@...el.com>
CC: Ying Huang <ying.huang@...el.com>, Dave Jiang <dave.jiang@...el.com>, "Dan
 Williams" <dan.j.williams@...el.com>, Davidlohr Bueso <dave@...olabs.net>,
	Alison Schofield <alison.schofield@...el.com>, Vishal Verma
	<vishal.l.verma@...el.com>, <linux-cxl@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3] cxl/region: Remove lock from memory notifier callback

Jonathan Cameron wrote:
> On Wed, 04 Sep 2024 09:47:54 -0500
> Ira Weiny <ira.weiny@...el.com> wrote:
> 

[snip]

> > 
> > Link: https://lore.kernel.org/all/66b4cf539a79b_a36e829416@iweiny-mobl.notmuch/ [0]
> > Cc: Ying Huang <ying.huang@...el.com>
> > Suggested-by: Dan Williams <dan.j.williams@...el.com>
> > Reviewed-by: Davidlohr Bueso <dave@...olabs.net>
> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@...wei.com>
> > Reviewed-by: Ying Huang <ying.huang@...el.com>
> > Signed-off-by: Ira Weiny <ira.weiny@...el.com>
> A few comments on looking at this again, but all things that apply equally to old
> code so maybe things for another day.

Yea this was solely a move of existing code to fix the locking issue.  I
did not evaluate the original code.  However...

[snip]

> >  }
> >  
> > +static void shutdown_notifiers(void *_cxlr)
> > +{
> > +	struct cxl_region *cxlr = _cxlr;
> > +
> > +	unregister_memory_notifier(&cxlr->memory_notifier);
> > +	unregister_mt_adistance_algorithm(&cxlr->adist_notifier);
> Flip order.
> 
> Makes zero real difference, but if we later end up with more to do
> here for some reason there may be ordering requirements that will
> care that this doesn't tear down in reverse of setup.

Generally I agree with you however, the memory and adist notifiers are
unrelated.  So failing to unwind in reverse order is a matter of taste and
is not required even if some other logic was introduced between the
registrations I don't see how this backwards order would be an issue.

> 
> Mind you, see below.
> 
> > +}
> > +
> >  static int cxl_region_probe(struct device *dev)
> >  {
> >  	struct cxl_region *cxlr = to_cxl_region(dev);
> > @@ -3418,6 +3412,18 @@ static int cxl_region_probe(struct device *dev)
> >  out:
> >  	up_read(&cxl_region_rwsem);
> >  
> > +	if (rc)
> > +		return rc;
> > +
> > +	cxlr->memory_notifier.notifier_call = cxl_region_perf_attrs_callback;
> > +	cxlr->memory_notifier.priority = CXL_CALLBACK_PRI;
> > +	register_memory_notifier(&cxlr->memory_notifier);
> Can in theory fail.  Today that is EEXIST only but who knows in future.
> I think we should handle that and do two devm_add_action_or_reset() perhaps?
> 

First we should not fail the probe if this fails.

Second, nothing bad happens in unregister if the registration failed.
Therefore, register failing is benign and I don't see a need for the extra
action callback.

Ira

[snip]