Re: [PATCH v6 4/4] scsi: target: pin db_root for metadata writes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] target: use-after-free and torn read of db_root_path during concurrent configfs write
- [Medium] target: target_core_item_dbroot_store returns 0 on error
--

commit c0795bea7761bae07f37ffbb11cf3b1c3410422d
Author: Runyu Xiao <runyu.xiao@seu.edu.cn>

scsi: target: pin db_root for metadata writes

This commit addresses metadata file path resolution in the target subsystem
by resolving db_root once and pinning the resulting path. It aims to
prevent re-entering configfs while holding frag_sem and bypasses via
retargeted symlinks.

Link: https://lore.kernel.org/r/20260818051442.1523210-1-runyu.xiao@seu.edu.cn

> diff --git a/drivers/target/target_core_alua.c b/drivers/target/target_core_alua.c
> --- a/drivers/target/target_core_alua.c
> +++ b/drivers/target/target_core_alua.c
> @@ -862,20 +861,23 @@ static int core_alua_write_tpg_metadata(
>  	loff_t pos = 0;
>  	int ret;
>  
> -	if (tsk_is_kthread(current)) {
> -		scoped_with_init_fs()
> -			file = filp_open(path, O_RDWR | O_CREAT | O_TRUNC, 0600);
> -	} else {
> -		file = filp_open(path, O_RDWR | O_CREAT | O_TRUNC, 0600);
> +	if (!db_root_path.dentry) {

[Severity: High]
Is it possible for this lockless read of db_root_path to race with a
concurrent configfs store operation?

Since ALUA configuration state can be updated for a LUN even when there are
no backend devices present (meaning target_devices evaluates to 0), a
concurrent write to the dbroot configfs attribute wouldn't be blocked.
Could this lead to a torn read of the path structure?

> +		pr_err("db_root is not initialized for ALUA metadata path: %s/%s\n",
> +		       db_root, path);
> +		return -ENODEV;
>  	}

[ ... ]

> diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c
> --- a/drivers/target/target_core_configfs.c
> +++ b/drivers/target/target_core_configfs.c
[ ... ]
> @@ -107,46 +141,70 @@ static ssize_t target_core_item_dbroot_show(struct config_item *item,
>  static ssize_t target_core_item_dbroot_store(struct config_item *item,
>  					const char *page, size_t count)
>  {
[ ... ]
> +	r = target_validate_db_root(db_root_stage, &path);
> +	if (r)
> +		goto free_stage;
> +
> +	mutex_lock(&target_devices_lock);
> +	if (target_devices) {
> +		pr_err("db_root: cannot be changed because it's in use\n");
> +		goto unlock_put;
>  	}

[Severity: Medium]
At this point in the execution flow, r is 0 because the call to
target_validate_db_root() succeeded. If we take this goto unlock_put path
due to target_devices being non-zero, doesn't this result in returning 0 to
userspace instead of an error code like -EINVAL?

> -	path_put(&path);
>  
> +	have_old_path = db_root_path.dentry;
> +	if (have_old_path)
> +		old_path = db_root_path;
> +	db_root_path = path;
> +	path = (struct path){};

[Severity: High]
Can this non-atomic update of db_root_path race with the lockless readers in
the metadata write paths?

Since the target_devices check above allows this write to proceed when there
are no backend devices, could this result in a use-after-free or invalid
pointer dereference if the old path is freed while being concurrently
accessed by ALUA or virtual_lun0 PR metadata writers?

>  	strscpy(db_root, db_root_stage);
>  	pr_debug("Target_Core_ConfigFS: db_root set to %s\n", db_root);
>  
>  	r = read_bytes;
>  
> -unlock:
> +unlock_put:
>  	mutex_unlock(&target_devices_lock);
> +	if (path.dentry)
> +		path_put(&path);
> +	if (have_old_path)
> +		path_put(&old_path);
> +	kfree(db_root_stage);
> +	return r;

[ ... ]

> diff --git a/drivers/target/target_core_pr.c b/drivers/target/target_core_pr.c
> --- a/drivers/target/target_core_pr.c
> +++ b/drivers/target/target_core_pr.c
> @@ -1965,16 +1964,21 @@ static int __core_scsi3_write_aptpl_to_file(
>  	int ret;
>  	loff_t pos = 0;
>  
> -	path = kasprintf(GFP_KERNEL, "%s/pr/aptpl_%s", db_root,
> -			&wwn->unit_serial[0]);
> +	path = kasprintf(GFP_KERNEL, "pr/aptpl_%s", &wwn->unit_serial[0]);
>  	if (!path)
>  		return -ENOMEM;
>  
> -	scoped_with_init_fs()
> -		file = filp_open(path, flags, 0600);
> +	if (!db_root_path.dentry) {

[Severity: High]
Similar to the ALUA metadata path, could this locklessly access db_root_path
while it is being updated?

For virtual_lun0, this function can execute when target_devices is 0,
potentially colliding with the configfs writer and leading to a torn read.

> +		pr_err("db_root is not initialized for APTPL metadata path: %s/%s\n",
> +		       db_root, path);
> +		kfree(path);
> +		return -ENODEV;
> +	}
> +
> +	file = configfs_open_root(&db_root_path, path, flags, 0600);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260926092148.128937-1-runyu.xiao@seu.edu.cn?part=4