[PATCH 0/5] Ensure that lun ids are contiguous

[PATCH 0/5] 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

Krzysztof Opasiak (5):
  fs: configfs: Fix typo in comment
  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                                  |   31 ++++++++++++++-
 include/linux/configfs.h                           |    9 +++++
 5 files changed, 82 insertions(+), 6 deletions(-)

-- 
1.7.9.5

[PATCH 1/5] fs: configfs: Fix typo in comment

[Krzysztof Opasiak]

Signed-off-by: Krzysztof Opasiak <k.opasiak@samsung.com>
---
 fs/configfs/dir.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
index cf0db00..dee1cb5 100644
--- a/fs/configfs/dir.c
+++ b/fs/configfs/dir.c
@@ -325,7 +325,7 @@ static void configfs_dir_set_ready(struct configfs_dirent *sd)
  * attached and not validated yet.
  * @sd		configfs_dirent of the directory to check
  *
- * @return	non-zero iff the directory was validated
+ * @return	non-zero if the directory was validated
  *
  * Note: takes configfs_dirent_lock, so the result may change from false to true
  * in two consecutive calls, but never from true to false.
-- 
1.7.9.5

Re: [PATCH 1/5] fs: configfs: Fix typo in comment

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 912 bytes --]

On Wed, Apr 08, 2015 at 01:28:44PM +0200, Krzysztof Opasiak wrote:
> Signed-off-by: Krzysztof Opasiak <k.opasiak@samsung.com>
> ---
>  fs/configfs/dir.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
> index cf0db00..dee1cb5 100644
> --- a/fs/configfs/dir.c
> +++ b/fs/configfs/dir.c
> @@ -325,7 +325,7 @@ static void configfs_dir_set_ready(struct configfs_dirent *sd)
>   * attached and not validated yet.
>   * @sd		configfs_dirent of the directory to check
>   *
> - * @return	non-zero iff the directory was validated
> + * @return	non-zero if the directory was validated

iff is not a typo, it's short-hand for "if, and only if"

>   *
>   * Note: takes configfs_dirent_lock, so the result may change from false to true
>   * in two consecutive calls, but never from true to false.
> -- 
> 1.7.9.5
> 

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/5] fs: configfs: Fix typo in comment

[Krzysztof Opasiak]

On 04/09/2015 03:46 PM, Felipe Balbi wrote:
> On Wed, Apr 08, 2015 at 01:28:44PM +0200, Krzysztof Opasiak wrote:
>> Signed-off-by: Krzysztof Opasiak <k.opasiak@samsung.com>
>> ---
>>   fs/configfs/dir.c |    2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
>> index cf0db00..dee1cb5 100644
>> --- a/fs/configfs/dir.c
>> +++ b/fs/configfs/dir.c
>> @@ -325,7 +325,7 @@ static void configfs_dir_set_ready(struct configfs_dirent *sd)
>>    * attached and not validated yet.
>>    * @sd		configfs_dirent of the directory to check
>>    *
>> - * @return	non-zero iff the directory was validated
>> + * @return	non-zero if the directory was validated
>
> iff is not a typo, it's short-hand for "if, and only if"
>

I've already dropped this patch in v2, but thank you for your remark.

Best regards,

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

[PATCH 2/5] 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 dee1cb5..ef51594 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 3/5] 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 3/5] usb: gadget: mass_storage: Store lun_opts in fsg_opts

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 159 bytes --]

On Wed, Apr 08, 2015 at 01:28:46PM +0200, Krzysztof Opasiak wrote:
> Signed-off-by: Krzysztof Opasiak <k.opasiak@samsung.com>

no commit log

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH 4/5] 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

Re: [PATCH 4/5] usb: gadget: mass_storage: Ensure that lun ids are contiguous

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 159 bytes --]

On Wed, Apr 08, 2015 at 01:28:47PM +0200, Krzysztof Opasiak wrote:
> Signed-off-by: Krzysztof Opasiak <k.opasiak@samsung.com>

no commit log

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH 5/5] 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