netdev - Re: [PATCH v21 01/23] cxl/mem: refactor memdev allocation

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: <692e54808af8d_261c11001@dwillia2-mobl4.notmuch>
Date: Mon, 1 Dec 2025 18:52:48 -0800
From: <dan.j.williams@...el.com>
To: <alejandro.lucero-palau@....com>, <linux-cxl@...r.kernel.org>,
	<netdev@...r.kernel.org>, <dan.j.williams@...el.com>, <edward.cree@....com>,
	<davem@...emloft.net>, <kuba@...nel.org>, <pabeni@...hat.com>,
	<edumazet@...gle.com>, <dave.jiang@...el.com>
CC: Alejandro Lucero <alucerop@....com>
Subject: Re: [PATCH v21 01/23] cxl/mem: refactor memdev allocation

alejandro.lucero-palau@ wrote:
> From: Alejandro Lucero <alucerop@....com>
> 
> In preparation for always-synchronous memdev attach, refactor memdev
> allocation and fix release bug in devm_cxl_add_memdev() when error after
> a successful allocation.

Never do "refactor and fix". Always do "fix" then "refactor" separately.
In this case though I wonder what release bug you are referring to?

If cxl_memdev_alloc() fails, nothing to free.

If dev_set_name() fails, it puts the device which calls
cxl_memdev_release() which undoes cxl_memdev_alloc().  (Now, that weird
and busted devm_cxl_memdev_edac_release() somehow snuck into
cxl_memdev_release() when I was not looking. I will fix that separately,
but no leak there that I can see.)

If cdev_device_add() fails we need to shutdown the ioctl path, but
otherwise put_device() cleans everything up.

If the devm_add_action_or_reset() fails the device needs to be both
unregistered and final put. It does not use device_unregister() because
the cdev also needs to be deleted. So cdev_device_del() handles the
device_del() and the caller is responsible for the final put_device().

What bug are you referring to?

> The diff is busy as this moves cxl_memdev_alloc() down below the definition
> of cxl_memdev_fops and introduces devm_cxl_memdev_add_or_reset() to
> preclude needing to export more symbols from the cxl_core.

Will need to read the code to figure out what this patch is trying to do
because this changelog is not orienting me to the problem that is being
solved.

> Fixes: 1c3333a28d45 ("cxl/mem: Do not rely on device_add() side effects for dev_set_name() failures")

Maybe this Fixes: tag is wrong and this is instead a bug introduced by
my probe order RFC? At least Jonathan pinged me about a bug there that I
will go look at next.

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

Why does this have my Sign-off?

> Signed-off-by: Alejandro Lucero <alucerop@....com>
> ---
>  drivers/cxl/core/memdev.c | 134 +++++++++++++++++++++-----------------
>  drivers/cxl/private.h     |  10 +++
>  2 files changed, 86 insertions(+), 58 deletions(-)
>  create mode 100644 drivers/cxl/private.h
> 
> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> index e370d733e440..8de19807ac7b 100644
> --- a/drivers/cxl/core/memdev.c
> +++ b/drivers/cxl/core/memdev.c
> @@ -8,6 +8,7 @@
>  #include <linux/idr.h>
>  #include <linux/pci.h>
>  #include <cxlmem.h>
> +#include "private.h"
>  #include "trace.h"
>  #include "core.h"
>  
> @@ -648,42 +649,25 @@ static void detach_memdev(struct work_struct *work)
>  
>  static struct lock_class_key cxl_memdev_key;
>  
> -static struct cxl_memdev *cxl_memdev_alloc(struct cxl_dev_state *cxlds,
> -					   const struct file_operations *fops)
> +int devm_cxl_memdev_add_or_reset(struct device *host, struct cxl_memdev *cxlmd)

Can you say more why Type-2 drivers need an "_or_reset()" export? If a
Type-2 driver is calling devm_cxl_add_memdev() from its ->probe()
routine, then just return on failure. Confused.