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@xxxxxxxxxx>
> > ---
> > 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
