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

[Maurizio Lombardi <mlombard@redhat.com>]

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