2015-04-08 11:29:23

by Krzysztof Opasiak

[permalink] [raw]
Subject: [PATCH 0/5] Ensure that lun ids are contiguous

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


2015-04-08 11:29:29

by Krzysztof Opasiak

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

Signed-off-by: Krzysztof Opasiak <[email protected]>
---
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

2015-04-08 11:30:40

by Krzysztof Opasiak

[permalink] [raw]
Subject: [PATCH 2/5] fs: configfs: Add unlocked version of configfs_depend_item()

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 <[email protected]>
---
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

2015-04-08 11:29:35

by Krzysztof Opasiak

[permalink] [raw]
Subject: [PATCH 3/5] usb: gadget: mass_storage: Store lun_opts in fsg_opts

Signed-off-by: Krzysztof Opasiak <[email protected]>
---
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

2015-04-08 11:29:47

by Krzysztof Opasiak

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

Signed-off-by: Krzysztof Opasiak <[email protected]>
---
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

2015-04-08 11:29:44

by Krzysztof Opasiak

[permalink] [raw]
Subject: [PATCH 5/5] Documentation: ABI: Fix documentation for mass_storage function

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 <[email protected]>
---
.../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

2015-04-09 13:48:21

by Felipe Balbi

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

On Wed, Apr 08, 2015 at 01:28:44PM +0200, Krzysztof Opasiak wrote:
> Signed-off-by: Krzysztof Opasiak <[email protected]>
> ---
> 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


Attachments:
(No filename) (884.00 B)
signature.asc (819.00 B)
Digital signature
Download all attachments

2015-04-09 13:49:14

by Felipe Balbi

[permalink] [raw]
Subject: Re: [PATCH 3/5] usb: gadget: mass_storage: Store lun_opts in fsg_opts

On Wed, Apr 08, 2015 at 01:28:46PM +0200, Krzysztof Opasiak wrote:
> Signed-off-by: Krzysztof Opasiak <[email protected]>

no commit log

--
balbi


Attachments:
(No filename) (152.00 B)
signature.asc (819.00 B)
Digital signature
Download all attachments

2015-04-09 13:49:21

by Felipe Balbi

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

On Wed, Apr 08, 2015 at 01:28:47PM +0200, Krzysztof Opasiak wrote:
> Signed-off-by: Krzysztof Opasiak <[email protected]>

no commit log

--
balbi


Attachments:
(No filename) (152.00 B)
signature.asc (819.00 B)
Digital signature
Download all attachments

2015-04-09 14:50:45

by Krzysztof Opasiak

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



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 <[email protected]>
>> ---
>> 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