[PATCH] target: allow changing dbroot if there are no registered devices

[PATCH] target: allow changing dbroot if there are no registered devices

[Maurizio Lombardi]

The target driver prevents the users from changing
the database root directory if a target module like ib_srpt has
been registered, this makes it difficult for users to
set their preferred database directory if the module
gets loaded during the system boot.

Let the users modify dbroot if there are no registered devices

Signed-off-by: Maurizio Lombardi <mlombard@redhat.com>
---
 drivers/target/target_core_configfs.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c
index 023bd4516a68..cba10829e62f 100644
--- a/drivers/target/target_core_configfs.c
+++ b/drivers/target/target_core_configfs.c
@@ -72,6 +72,8 @@ static struct config_group target_core_hbagroup;
 static struct config_group alua_group;
 static struct config_group alua_lu_gps_group;
 
+static unsigned int target_devices;
+
 static inline struct se_hba *
 item_to_hba(struct config_item *item)
 {
@@ -106,38 +108,32 @@ static ssize_t target_core_item_dbroot_store(struct config_item *item,
 	ssize_t read_bytes;
 	struct file *fp;
 
-	mutex_lock(&g_tf_lock);
-	if (!list_empty(&g_tf_list)) {
-		mutex_unlock(&g_tf_lock);
-		pr_err("db_root: cannot be changed: target drivers registered");
+	if (target_devices) {
+		pr_err("db_root: cannot be changed because it's in use\n");
 		return -EINVAL;
 	}
 
 	if (count > (DB_ROOT_LEN - 1)) {
-		mutex_unlock(&g_tf_lock);
 		pr_err("db_root: count %d exceeds DB_ROOT_LEN-1: %u\n",
 		       (int)count, DB_ROOT_LEN - 1);
 		return -EINVAL;
 	}
 
 	read_bytes = snprintf(db_root_stage, DB_ROOT_LEN, "%s", page);
-	if (!read_bytes) {
-		mutex_unlock(&g_tf_lock);
+	if (!read_bytes)
 		return -EINVAL;
-	}
+
 	if (db_root_stage[read_bytes - 1] == '\n')
 		db_root_stage[read_bytes - 1] = '\0';
 
 	/* validate new db root before accepting it */
 	fp = filp_open(db_root_stage, O_RDONLY, 0);
 	if (IS_ERR(fp)) {
-		mutex_unlock(&g_tf_lock);
 		pr_err("db_root: cannot open: %s\n", db_root_stage);
 		return -EINVAL;
 	}
 	if (!S_ISDIR(file_inode(fp)->i_mode)) {
 		filp_close(fp, NULL);
-		mutex_unlock(&g_tf_lock);
 		pr_err("db_root: not a directory: %s\n", db_root_stage);
 		return -EINVAL;
 	}
@@ -145,8 +141,6 @@ static ssize_t target_core_item_dbroot_store(struct config_item *item,
 
 	strncpy(db_root, db_root_stage, read_bytes);
 
-	mutex_unlock(&g_tf_lock);
-
 	pr_debug("Target_Core_ConfigFS: db_root set to %s\n", db_root);
 
 	return read_bytes;
@@ -3315,6 +3309,7 @@ static struct config_group *target_core_make_subdev(
 	 */
 	target_stat_setup_dev_default_groups(dev);
 
+	target_devices++;
 	mutex_unlock(&hba->hba_access_mutex);
 	return &dev->dev_group;
 
@@ -3353,6 +3348,7 @@ static void target_core_drop_subdev(
 	 * se_dev is released from target_core_dev_item_ops->release()
 	 */
 	config_item_put(item);
+	target_devices--;
 	mutex_unlock(&hba->hba_access_mutex);
 }
 
-- 
2.27.0

Re: [PATCH] target: allow changing dbroot if there are no registered devices

[Mike Christie]

On 1/7/22 9:10 AM, Maurizio Lombardi wrote:
> The target driver prevents the users from changing
> the database root directory if a target module like ib_srpt has
> been registered, this makes it difficult for users to
> set their preferred database directory if the module
> gets loaded during the system boot.
> 
> Let the users modify dbroot if there are no registered devices
> 
> Signed-off-by: Maurizio Lombardi <mlombard@redhat.com>
> ---
>  drivers/target/target_core_configfs.c | 20 ++++++++------------
>  1 file changed, 8 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c
> index 023bd4516a68..cba10829e62f 100644
> --- a/drivers/target/target_core_configfs.c
> +++ b/drivers/target/target_core_configfs.c
> @@ -72,6 +72,8 @@ static struct config_group target_core_hbagroup;
>  static struct config_group alua_group;
>  static struct config_group alua_lu_gps_group;
>  
> +static unsigned int target_devices;
> +
>  static inline struct se_hba *
>  item_to_hba(struct config_item *item)
>  {
> @@ -106,38 +108,32 @@ static ssize_t target_core_item_dbroot_store(struct config_item *item,
>  	ssize_t read_bytes;
>  	struct file *fp;
>  
> -	mutex_lock(&g_tf_lock);
> -	if (!list_empty(&g_tf_list)) {
> -		mutex_unlock(&g_tf_lock);
> -		pr_err("db_root: cannot be changed: target drivers registered");
> +	if (target_devices) {
> +		pr_err("db_root: cannot be changed because it's in use\n");
>  		return -EINVAL;
>  	}
>  

How does the locking work for this patch?

The configfs subsys mutex handles the make and drop callouts below.

For this part though, it didn't look like we are holding the same mutex,
so can target_devices increase after we've passed that check? If so, was
the idea that it's one of those things where if it races then it doesn't
really matter because it's rare and nothing is really hurt?

>  	if (count > (DB_ROOT_LEN - 1)) {
> -		mutex_unlock(&g_tf_lock);
>  		pr_err("db_root: count %d exceeds DB_ROOT_LEN-1: %u\n",
>  		       (int)count, DB_ROOT_LEN - 1);
>  		return -EINVAL;
>  	}
>  
>  	read_bytes = snprintf(db_root_stage, DB_ROOT_LEN, "%s", page);
> -	if (!read_bytes) {
> -		mutex_unlock(&g_tf_lock);
> +	if (!read_bytes)
>  		return -EINVAL;
> -	}
> +
>  	if (db_root_stage[read_bytes - 1] == '\n')
>  		db_root_stage[read_bytes - 1] = '\0';
>  
>  	/* validate new db root before accepting it */
>  	fp = filp_open(db_root_stage, O_RDONLY, 0);
>  	if (IS_ERR(fp)) {
> -		mutex_unlock(&g_tf_lock);
>  		pr_err("db_root: cannot open: %s\n", db_root_stage);
>  		return -EINVAL;
>  	}
>  	if (!S_ISDIR(file_inode(fp)->i_mode)) {
>  		filp_close(fp, NULL);
> -		mutex_unlock(&g_tf_lock);
>  		pr_err("db_root: not a directory: %s\n", db_root_stage);
>  		return -EINVAL;
>  	}
> @@ -145,8 +141,6 @@ static ssize_t target_core_item_dbroot_store(struct config_item *item,
>  
>  	strncpy(db_root, db_root_stage, read_bytes);
>  
> -	mutex_unlock(&g_tf_lock);
> -
>  	pr_debug("Target_Core_ConfigFS: db_root set to %s\n", db_root);
>  
>  	return read_bytes;
> @@ -3315,6 +3309,7 @@ static struct config_group *target_core_make_subdev(
>  	 */
>  	target_stat_setup_dev_default_groups(dev);
>  
> +	target_devices++;
>  	mutex_unlock(&hba->hba_access_mutex);
>  	return &dev->dev_group;
>  
> @@ -3353,6 +3348,7 @@ static void target_core_drop_subdev(
>  	 * se_dev is released from target_core_dev_item_ops->release()
>  	 */
>  	config_item_put(item);
> +	target_devices--;
>  	mutex_unlock(&hba->hba_access_mutex);
>  }
>  

Re: [PATCH] target: allow changing dbroot if there are no registered devices

[Maurizio Lombardi]

On Tue, Feb 22, 2022 at 02:57:30PM -0600, Mike Christie wrote:
> On 1/7/22 9:10 AM, Maurizio Lombardi wrote:
> > The target driver prevents the users from changing
> > the database root directory if a target module like ib_srpt has
> > been registered, this makes it difficult for users to
> > set their preferred database directory if the module
> > gets loaded during the system boot.
> > 
> > Let the users modify dbroot if there are no registered devices
> > 
> > Signed-off-by: Maurizio Lombardi <mlombard@redhat.com>
> > ---
> >  drivers/target/target_core_configfs.c | 20 ++++++++------------
> >  1 file changed, 8 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c
> > index 023bd4516a68..cba10829e62f 100644
> > --- a/drivers/target/target_core_configfs.c
> > +++ b/drivers/target/target_core_configfs.c
> > @@ -72,6 +72,8 @@ static struct config_group target_core_hbagroup;
> >  static struct config_group alua_group;
> >  static struct config_group alua_lu_gps_group;
> >  
> > +static unsigned int target_devices;
> > +
> >  static inline struct se_hba *
> >  item_to_hba(struct config_item *item)
> >  {
> > @@ -106,38 +108,32 @@ static ssize_t target_core_item_dbroot_store(struct config_item *item,
> >  	ssize_t read_bytes;
> >  	struct file *fp;
> >  
> > -	mutex_lock(&g_tf_lock);
> > -	if (!list_empty(&g_tf_list)) {
> > -		mutex_unlock(&g_tf_lock);
> > -		pr_err("db_root: cannot be changed: target drivers registered");
> > +	if (target_devices) {
> > +		pr_err("db_root: cannot be changed because it's in use\n");
> >  		return -EINVAL;
> >  	}
> >  
> 
> How does the locking work for this patch?
> 
> The configfs subsys mutex handles the make and drop callouts below.
> 
> For this part though, it didn't look like we are holding the same mutex,
> so can target_devices increase after we've passed that check? If so, was
> the idea that it's one of those things where if it races then it doesn't
> really matter because it's rare and nothing is really hurt?

Thanks for the review, Mike.

There is, indeed, a small window where a race condition is possible;
when "target_devices" is 0, a module gets loaded by the kernel and at the same time a userspace process
writes to dbroot, before the "target_devices" variable gets incremented to 1.

I guess it's extremely rare but maybe it's better to simply add a "target_devices_lock" mutex
to be safe.

Maurizio