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

[PATCH v3 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

Footnotes:
1 - http://www.usb.org/developers/docs/devclass_docs/usbmassbulk_10.pdf

--
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")

Changes since v2:
- replace BUG_ON() with WARN_ON()
- s/arabic numeral/deciaml value
- add more verbose commit messages
- fix some typos
- move out label in more suitable place

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       |   34 +++++++++++++++++---
 drivers/usb/gadget/function/f_mass_storage.h       |    1 +
 fs/configfs/dir.c                                  |   29 +++++++++++++++++
 include/linux/configfs.h                           |    9 ++++++
 5 files changed, 75 insertions(+), 5 deletions(-)

-- 
1.7.9.5

[PATCH v3 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-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
---
 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 v3 2/4] usb: gadget: mass_storage: Store lun_opts in fsg_opts

[Krzysztof Opasiak]

Currently lun_opts are stored only in configfs and
accessed using container_of() on config group. This
means that in configfs callbacks we can easily access
only current lun (config_group).

This commit adds an additinal array to fsg_opts which
allows to access not only current lun but also all other
luns using their id.

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..67a67b5 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;
+	WARN_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

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

[Krzysztof Opasiak]

According to mass storage specification:

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

This commit fix configfs interface adding this restriction.
Now user can create luns only with contignous ids and
cannot remove lun from the middle of id space.

Example:

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

After this commit:
$ 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

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

diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c
index 67a67b5..f4b2de4 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 contiguous\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;
 	WARN_ON(fsg_opts->lun_opts[num]);
@@ -3379,6 +3390,12 @@ static struct config_group *fsg_lun_make(struct config_group *group,
 	config_group_init_type_name(&opts->group, name, &fsg_lun_type);
 
 	return &opts->group;
+
+err_undepend_item:
+	configfs_undepend_item_unlocked(&(fsg_opts->lun_opts
+					  [num - 1]->group.cg_item));
+err_free_opts:
+	kfree(opts);
 out:
 	mutex_unlock(&fsg_opts->lock);
 	return ERR_PTR(ret);
@@ -3400,6 +3417,10 @@ 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;
-- 
1.7.9.5

[PATCH v3 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 decimal value from 1 to FSG_MAX_LUNS.

Moreover, LUNs ids should be contiguous. This means that user
may remove only lun with max id and can create new lun
only if its id equals to max id + 1.

Signed-off-by: Krzysztof Opasiak <k.opasiak-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
---
 .../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..2bf085d 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 - decimal value 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

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Felipe Balbi]

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

Hi,

On Thu, Apr 09, 2015 at 06:18:07PM +0200, Krzysztof Opasiak wrote:
> 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-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>

Joel, any comments to this patch ?

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

-- 
balbi

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