[PATCH v2 0/4] Ensure that lun ids are contiguous

[PATCH v2 0/4] Ensure that lun ids are contiguous

[Krzysztof Opasiak]

Dear list,

This series fix configfs interface for mass storage function.
According to mass storage specification[1]:

"Logical Unit Numbers on the device shall be numbered contiguously
 starting from LUN 0 to a maximum LUN of 15 (Fh)."

Currently configfs interface allows to create LUNs with
arbitrary ids what causes problems with some host side
mass storage drivers.

This patch extends configfs interface with unlocked version
of configfs_depend_item() which should be used only in configfs
callbacks. This allows to protect from
removing lun directory from the middle of id space.

Example:

as is:
$ mkdir mass_storage.name
$ mkdir lun.3
$ mkdir lun.5
$ rmdir lun.3

After this series:
$ mkdir mass_storage.name
$ mkdir lun.3
mkdir: cannot create directory 'lun.3': Invalid argument
$ mkdir lun.1
$ mkdir lun.2
$ rmdir lun.1
rmdir: failed to remove 'lun.1': Device or resource busy
$ rmdir lun.2
$ rmdir lun.1

--
Best regards,
Krzysztof Opasiak
Samsung R&D Institute Poland
Samsung Electronics

---
Changes since v1:
- drop incorrect typo fix
   ("iff" is not a typo but shorten version of "if and only if")

Krzysztof Opasiak (4):
  fs: configfs: Add unlocked version of configfs_depend_item()
  usb: gadget: mass_storage: Store lun_opts in fsg_opts
  usb: gadget: mass_storage: Ensure that lun ids are contiguous
  Documentation: ABI: Fix documentation for mass_storage function

 .../ABI/testing/configfs-usb-gadget-mass-storage   |    7 +++-
 drivers/usb/gadget/function/f_mass_storage.c       |   40 ++++++++++++++++++--
 drivers/usb/gadget/function/f_mass_storage.h       |    1 +
 fs/configfs/dir.c                                  |   29 ++++++++++++++
 include/linux/configfs.h                           |    9 +++++
 5 files changed, 81 insertions(+), 5 deletions(-)

-- 
1.7.9.5

[PATCH v2 1/4] fs: configfs: Add unlocked version of configfs_depend_item()

[Krzysztof Opasiak]

Sometimes it might be desirable to prohibit removing a directory
in configfs. One example is USB gadget (mass_storage function):
when gadget is already bound, if lun directory is removed,
the gadget must be thrown away, too. A better solution would be
to fail with e.g. -EBUSY.

Currently configfs has configfs_depend/undepend_item() methods
but they cannot be used in configfs callbacks. This commit
adds unlocked version of this methods which can be used
only in configfs callbacks.

Signed-off-by: Krzysztof Opasiak <k.opasiak@samsung.com>
---
 fs/configfs/dir.c        |   29 +++++++++++++++++++++++++++++
 include/linux/configfs.h |    9 +++++++++
 2 files changed, 38 insertions(+)

diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
index cf0db00..7875a5e 100644
--- a/fs/configfs/dir.c
+++ b/fs/configfs/dir.c
@@ -1152,6 +1152,35 @@ void configfs_undepend_item(struct configfs_subsystem *subsys,
 }
 EXPORT_SYMBOL(configfs_undepend_item);
 
+int configfs_depend_item_unlocked(struct config_item *target)
+{
+	struct configfs_dirent *sd;
+	int ret = -ENOENT;
+
+	spin_lock(&configfs_dirent_lock);
+	BUG_ON(!target->ci_dentry);
+
+	sd = target->ci_dentry->d_fsdata;
+	if ((sd->s_type & CONFIGFS_DIR) &&
+	    ((sd->s_type & CONFIGFS_USET_DROPPING) ||
+	     (sd->s_type & CONFIGFS_USET_CREATING)))
+		goto out_unlock_dirent_lock;
+
+	sd->s_dependent_count += 1;
+	ret = 0;
+
+out_unlock_dirent_lock:
+	spin_unlock(&configfs_dirent_lock);
+	return ret;
+}
+EXPORT_SYMBOL(configfs_depend_item_unlocked);
+
+void configfs_undepend_item_unlocked(struct config_item *target)
+{
+	configfs_undepend_item(NULL, target);
+}
+EXPORT_SYMBOL(configfs_undepend_item_unlocked);
+
 static int configfs_mkdir(struct inode *dir, struct dentry *dentry, umode_t mode)
 {
 	int ret = 0;
diff --git a/include/linux/configfs.h b/include/linux/configfs.h
index 34025df..e9dbf01 100644
--- a/include/linux/configfs.h
+++ b/include/linux/configfs.h
@@ -257,4 +257,13 @@ void configfs_unregister_subsystem(struct configfs_subsystem *subsys);
 int configfs_depend_item(struct configfs_subsystem *subsys, struct config_item *target);
 void configfs_undepend_item(struct configfs_subsystem *subsys, struct config_item *target);
 
+/*
+ * These functions can sleep and can alloc with GFP_KERNEL
+ * NOTE: These should be called only underneath configfs callbacks.
+ * WARNING: These cannot be called on newly created item
+ *        (in make_group()/make_item callback)
+ */
+int configfs_depend_item_unlocked(struct config_item *target);
+void configfs_undepend_item_unlocked(struct config_item *target);
+
 #endif /* _CONFIGFS_H_ */
-- 
1.7.9.5

[PATCH v2 2/4] usb: gadget: mass_storage: Store lun_opts in fsg_opts

[Krzysztof Opasiak]

Signed-off-by: Krzysztof Opasiak <k.opasiak@samsung.com>
---
 drivers/usb/gadget/function/f_mass_storage.c |    5 +++++
 drivers/usb/gadget/function/f_mass_storage.h |    1 +
 2 files changed, 6 insertions(+)

diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c
index 811929c..095b618 100644
--- a/drivers/usb/gadget/function/f_mass_storage.c
+++ b/drivers/usb/gadget/function/f_mass_storage.c
@@ -3372,6 +3372,8 @@ static struct config_group *fsg_lun_make(struct config_group *group,
 	}
 	opts->lun = fsg_opts->common->luns[num];
 	opts->lun_id = num;
+	BUG_ON(fsg_opts->lun_opts[num]);
+	fsg_opts->lun_opts[num] = opts;
 	mutex_unlock(&fsg_opts->lock);
 
 	config_group_init_type_name(&opts->group, name, &fsg_lun_type);
@@ -3400,6 +3402,7 @@ static void fsg_lun_drop(struct config_group *group, struct config_item *item)
 
 	fsg_common_remove_lun(lun_opts->lun, fsg_opts->common->sysfs);
 	fsg_opts->common->luns[lun_opts->lun_id] = NULL;
+	fsg_opts->lun_opts[lun_opts->lun_id] = NULL;
 	lun_opts->lun_id = 0;
 	mutex_unlock(&fsg_opts->lock);
 
@@ -3546,6 +3549,7 @@ static struct usb_function_instance *fsg_alloc_inst(void)
 	if (!opts)
 		return ERR_PTR(-ENOMEM);
 	mutex_init(&opts->lock);
+	memset(opts->lun_opts, 0, sizeof(opts->lun_opts));
 	opts->func_inst.free_func_inst = fsg_free_inst;
 	opts->common = fsg_common_setup(opts->common);
 	if (IS_ERR(opts->common)) {
@@ -3569,6 +3573,7 @@ static struct usb_function_instance *fsg_alloc_inst(void)
 			(const char **)&opts->func_inst.group.cg_item.ci_name);
 	opts->lun0.lun = opts->common->luns[0];
 	opts->lun0.lun_id = 0;
+	opts->lun_opts[0] = &opts->lun0;
 	config_group_init_type_name(&opts->lun0.group, "lun.0", &fsg_lun_type);
 	opts->default_groups[0] = &opts->lun0.group;
 	opts->func_inst.group.default_groups = opts->default_groups;
diff --git a/drivers/usb/gadget/function/f_mass_storage.h b/drivers/usb/gadget/function/f_mass_storage.h
index b4866fc..0a7c656 100644
--- a/drivers/usb/gadget/function/f_mass_storage.h
+++ b/drivers/usb/gadget/function/f_mass_storage.h
@@ -81,6 +81,7 @@ struct fsg_opts {
 	struct fsg_common *common;
 	struct usb_function_instance func_inst;
 	struct fsg_lun_opts lun0;
+	struct fsg_lun_opts *lun_opts[FSG_MAX_LUNS];
 	struct config_group *default_groups[2];
 	bool no_configfs; /* for legacy gadgets */
 
-- 
1.7.9.5

Re: [PATCH v2 2/4] usb: gadget: mass_storage: Store lun_opts in fsg_opts

[Alan Stern]

On Wed, 8 Apr 2015, Krzysztof Opasiak wrote:

> Signed-off-by: Krzysztof Opasiak <k.opasiak@samsung.com>
> ---
>  drivers/usb/gadget/function/f_mass_storage.c |    5 +++++
>  drivers/usb/gadget/function/f_mass_storage.h |    1 +
>  2 files changed, 6 insertions(+)
> 
> diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c
> index 811929c..095b618 100644
> --- a/drivers/usb/gadget/function/f_mass_storage.c
> +++ b/drivers/usb/gadget/function/f_mass_storage.c
> @@ -3372,6 +3372,8 @@ static struct config_group *fsg_lun_make(struct config_group *group,
>  	}
>  	opts->lun = fsg_opts->common->luns[num];
>  	opts->lun_id = num;
> +	BUG_ON(fsg_opts->lun_opts[num]);

This is not a good idea.  BUG_ON should hardly ever be used.  In fact,
Linus has said that the only time BUG_ON should be used is when things
are so badly messed up that it is better to crash the computer than to
let it continue.

What's wrong with using WARN_ON instead?

> diff --git a/drivers/usb/gadget/function/f_mass_storage.h b/drivers/usb/gadget/function/f_mass_storage.h
> index b4866fc..0a7c656 100644
> --- a/drivers/usb/gadget/function/f_mass_storage.h
> +++ b/drivers/usb/gadget/function/f_mass_storage.h
> @@ -81,6 +81,7 @@ struct fsg_opts {
>  	struct fsg_common *common;
>  	struct usb_function_instance func_inst;
>  	struct fsg_lun_opts lun0;
> +	struct fsg_lun_opts *lun_opts[FSG_MAX_LUNS];

This looks strange.  Why is the entry for LUN 0 duplicated?

Alan Stern

Re: [PATCH v2 2/4] usb: gadget: mass_storage: Store lun_opts in fsg_opts

[Krzysztof Opasiak]

Hi,

On 04/08/2015 04:15 PM, Alan Stern wrote:> On Wed, 8 Apr 2015, Krzysztof 
Opasiak wrote:
 >
 >> Signed-off-by: Krzysztof Opasiak <k.opasiak@samsung.com>
 >> ---
 >>   drivers/usb/gadget/function/f_mass_storage.c |    5 +++++
 >>   drivers/usb/gadget/function/f_mass_storage.h |    1 +
 >>   2 files changed, 6 insertions(+)
 >>
 >> diff --git a/drivers/usb/gadget/function/f_mass_storage.c 
b/drivers/usb/gadget/function/f_mass_storage.c
 >> index 811929c..095b618 100644
 >> --- a/drivers/usb/gadget/function/f_mass_storage.c
 >> +++ b/drivers/usb/gadget/function/f_mass_storage.c
 >> @@ -3372,6 +3372,8 @@ static struct config_group 
*fsg_lun_make(struct config_group *group,
 >>   	}
 >>   	opts->lun = fsg_opts->common->luns[num];
 >>   	opts->lun_id = num;
 >> +	BUG_ON(fsg_opts->lun_opts[num]);
 >
 > This is not a good idea.  BUG_ON should hardly ever be used.  In fact,
 > Linus has said that the only time BUG_ON should be used is when things
 > are so badly messed up that it is better to crash the computer than to
 > let it continue.
 >
 > What's wrong with using WARN_ON instead?

Nothing. I have simply used BUG_ON() because this situation should never 
happen. If it happed then we made some mess in luns and there is no easy 
way to recovery from this point. This is only a little defense point for 
future to make debugging easier. I may change this to WARN_ON() if you like.


 >
 >> diff --git a/drivers/usb/gadget/function/f_mass_storage.h 
b/drivers/usb/gadget/function/f_mass_storage.h
 >> index b4866fc..0a7c656 100644
 >> --- a/drivers/usb/gadget/function/f_mass_storage.h
 >> +++ b/drivers/usb/gadget/function/f_mass_storage.h
 >> @@ -81,6 +81,7 @@ struct fsg_opts {
 >>   	struct fsg_common *common;
 >>   	struct usb_function_instance func_inst;
 >>   	struct fsg_lun_opts lun0;
 >> +	struct fsg_lun_opts *lun_opts[FSG_MAX_LUNS];
 >
 > This looks strange.  Why is the entry for LUN 0 duplicated?

LUN 0 is created on mkdir and cannot be removed by user so the memory 
for it is allocated together with fsg_opts structure. Rest of LUNs are 
being created by explicit user action and malloc() separately. This 
array is used to store their pointers. To make the code easier we simply:

lun_opts[0] = &fsg_opts->lun0;

so later we don't care which lun we are dealing with.

--
Best regards,
Krzysztof Opasiak
Samsung R&D Institute Poland
Samsung Electronics

[PATCH v2 3/4] usb: gadget: mass_storage: Ensure that lun ids are contiguous

[Krzysztof Opasiak]

Signed-off-by: Krzysztof Opasiak <k.opasiak@samsung.com>
---
 drivers/usb/gadget/function/f_mass_storage.c |   35 +++++++++++++++++++++++---
 1 file changed, 31 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c
index 095b618..9d9fafb 100644
--- a/drivers/usb/gadget/function/f_mass_storage.c
+++ b/drivers/usb/gadget/function/f_mass_storage.c
@@ -3355,6 +3355,12 @@ static struct config_group *fsg_lun_make(struct config_group *group,
 		goto out;
 	}
 
+	if (!fsg_opts->common->luns[num - 1]) {
+		ret = -EINVAL;
+		pr_err("LUN ids should be contignous\n");
+		goto out;
+	}
+
 	opts = kzalloc(sizeof(*opts), GFP_KERNEL);
 	if (!opts) {
 		ret = -ENOMEM;
@@ -3364,12 +3370,17 @@ static struct config_group *fsg_lun_make(struct config_group *group,
 	memset(&config, 0, sizeof(config));
 	config.removable = true;
 
+	/* ensure that lun ids are contiguous */
+	ret = configfs_depend_item_unlocked(&(fsg_opts->lun_opts
+					      [num - 1]->group.cg_item));
+	if (ret)
+		goto err_free_opts;
+
 	ret = fsg_common_create_lun(fsg_opts->common, &config, num, name,
 				    (const char **)&group->cg_item.ci_name);
-	if (ret) {
-		kfree(opts);
-		goto out;
-	}
+	if (ret)
+		goto err_undepend_item;
+
 	opts->lun = fsg_opts->common->luns[num];
 	opts->lun_id = num;
 	BUG_ON(fsg_opts->lun_opts[num]);
@@ -3382,6 +3393,16 @@ static struct config_group *fsg_lun_make(struct config_group *group,
 out:
 	mutex_unlock(&fsg_opts->lock);
 	return ERR_PTR(ret);
+
+err_undepend_item:
+	configfs_undepend_item_unlocked(&(fsg_opts->lun_opts
+					  [num - 1]->group.cg_item));
+err_free_opts:
+	kfree(opts);
+
+	mutex_unlock(&fsg_opts->lock);
+	return ERR_PTR(ret);
+
 }
 
 static void fsg_lun_drop(struct config_group *group, struct config_item *item)
@@ -3400,10 +3421,16 @@ static void fsg_lun_drop(struct config_group *group, struct config_item *item)
 		unregister_gadget_item(gadget);
 	}
 
+
+	/* Allow to remove next one */
+	configfs_undepend_item_unlocked(&(fsg_opts->lun_opts
+					  [lun_opts->lun_id - 1]->group.cg_item));
+
 	fsg_common_remove_lun(lun_opts->lun, fsg_opts->common->sysfs);
 	fsg_opts->common->luns[lun_opts->lun_id] = NULL;
 	fsg_opts->lun_opts[lun_opts->lun_id] = NULL;
 	lun_opts->lun_id = 0;
+
 	mutex_unlock(&fsg_opts->lock);
 
 	config_item_put(item);
-- 
1.7.9.5

[PATCH v2 4/4] Documentation: ABI: Fix documentation for mass_storage function

[Krzysztof Opasiak]

Luns in mass storage function are identified using their id.
While creating lun's directory user cannot choose any arbitrary
name other than arabic numeral from 1 to FSG_MAX_LUNS.

Signed-off-by: Krzysztof Opasiak <k.opasiak@samsung.com>
---
 .../ABI/testing/configfs-usb-gadget-mass-storage   |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/Documentation/ABI/testing/configfs-usb-gadget-mass-storage b/Documentation/ABI/testing/configfs-usb-gadget-mass-storage
index 9931fb0..0b54280 100644
--- a/Documentation/ABI/testing/configfs-usb-gadget-mass-storage
+++ b/Documentation/ABI/testing/configfs-usb-gadget-mass-storage
@@ -11,10 +11,15 @@ Description:
 				are 2..4. Available only if
 				CONFIG_USB_GADGET_DEBUG_FILES is set.
 
-What:		/config/usb-gadget/gadget/functions/mass_storage.name/lun.name
+What:		/config/usb-gadget/gadget/functions/mass_storage.name/lun.id
 Date:		Oct 2013
 KernelVersion:	3.13
 Description:
+		id - arabic numeral from 1 to FSG_MAX_LUNS
+		(which is 8 by default) - 1. LUNs should be numbered contiguously.
+		lun.0 is reserved for default lun which appears while creating
+		mass_storage.name directory and cannot be removed by the user.
+
 		The attributes:
 
 		file		- The path to the backing file for the LUN.
-- 
1.7.9.5

Re: [PATCH v2 4/4] Documentation: ABI: Fix documentation for mass_storage function

[Tal Shorer]

On Wed, Apr 8, 2015 at 3:06 PM, Krzysztof Opasiak <k.opasiak@samsung.com> wrote:
> Luns in mass storage function are identified using their id.
> While creating lun's directory user cannot choose any arbitrary
> name other than arabic numeral from 1 to FSG_MAX_LUNS.
>
> Signed-off-by: Krzysztof Opasiak <k.opasiak@samsung.com>
> ---
>  .../ABI/testing/configfs-usb-gadget-mass-storage   |    7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/ABI/testing/configfs-usb-gadget-mass-storage b/Documentation/ABI/testing/configfs-usb-gadget-mass-storage
> index 9931fb0..0b54280 100644
> --- a/Documentation/ABI/testing/configfs-usb-gadget-mass-storage
> +++ b/Documentation/ABI/testing/configfs-usb-gadget-mass-storage
> @@ -11,10 +11,15 @@ Description:
>                                 are 2..4. Available only if
>                                 CONFIG_USB_GADGET_DEBUG_FILES is set.
>
> -What:          /config/usb-gadget/gadget/functions/mass_storage.name/lun.name
> +What:          /config/usb-gadget/gadget/functions/mass_storage.name/lun.id
>  Date:          Oct 2013
>  KernelVersion: 3.13
>  Description:
> +               id - arabic numeral from 1 to FSG_MAX_LUNS
I think "decimal number" or "decimal value" would be easier to understand.
> +               (which is 8 by default) - 1. LUNs should be numbered contiguously.
> +               lun.0 is reserved for default lun which appears while creating
> +               mass_storage.name directory and cannot be removed by the user.
> +
>                 The attributes:
>
>                 file            - The path to the backing file for the LUN.
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 4/4] Documentation: ABI: Fix documentation for mass_storage function

[Krzysztof Opasiak]

On 04/09/2015 10:32 AM, Tal Shorer wrote:
> On Wed, Apr 8, 2015 at 3:06 PM, Krzysztof Opasiak <k.opasiak@samsung.com> wrote:
>> Luns in mass storage function are identified using their id.
>> While creating lun's directory user cannot choose any arbitrary
>> name other than arabic numeral from 1 to FSG_MAX_LUNS.
>>
>> Signed-off-by: Krzysztof Opasiak <k.opasiak@samsung.com>
>> ---
>>   .../ABI/testing/configfs-usb-gadget-mass-storage   |    7 ++++++-
>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/ABI/testing/configfs-usb-gadget-mass-storage b/Documentation/ABI/testing/configfs-usb-gadget-mass-storage
>> index 9931fb0..0b54280 100644
>> --- a/Documentation/ABI/testing/configfs-usb-gadget-mass-storage
>> +++ b/Documentation/ABI/testing/configfs-usb-gadget-mass-storage
>> @@ -11,10 +11,15 @@ Description:
>>                                  are 2..4. Available only if
>>                                  CONFIG_USB_GADGET_DEBUG_FILES is set.
>>
>> -What:          /config/usb-gadget/gadget/functions/mass_storage.name/lun.name
>> +What:          /config/usb-gadget/gadget/functions/mass_storage.name/lun.id
>>   Date:          Oct 2013
>>   KernelVersion: 3.13
>>   Description:
>> +               id - arabic numeral from 1 to FSG_MAX_LUNS
> I think "decimal number" or "decimal value" would be easier to understand.

True. Will fix in v3.

Thanks,

--
Krzysztof Opasiak
Samsung R&D Institute Poland
Samsung Electronics