On Fri, Jul 19 2013, Andrzej Pietrasiewicz wrote: > When configfs is in place, the things related to intialization > of struct fsg_common will be split over a number of places. > This patch adds several functions which together cover the former > intialization routine fsg_common_init. > > When configfs is in place, the luns will not be represented in sysfs, > so there will be no struct device associated with a lun. > To prepare for this some debug macros need to be adjusted. Two new > fields are added to struct fsg_lun: name and name_pfx. > The "name" is for storing a string which is presented to the user > instead of the dev_name. The "name_pfx", if non-NULL, is prepended > to the "name" at printing time. > > The name_pfx is for a future lun.0, which will be a default group in > mass_storage.<name>. By design at USB function configfs group's creation > time its name is not known (but instead set a bit later in > drivers/usb/gadget/configfs.c:function_make) and it is this name that > serves the purpose of the said name prefix. So instead of copying > a yet-unknown string a pointer to it is stored in struct fsg_lun. > > Signed-off-by: Andrzej Pietrasiewicz <andrze...@samsung.com> > Signed-off-by: Kyungmin Park <kyungmin.p...@samsung.com>
> +int fsg_common_set_num_buffers(struct fsg_common *common, unsigned int n)
> +{
> + struct fsg_buffhd *bh, *new_buffhds;
> + int i, rc;
> +
> + rc = fsg_num_buffers_validate(n);
> + if (rc != 0)
> + return rc;
> +
> + new_buffhds = kcalloc(n, sizeof *(new_buffhds), GFP_KERNEL);
> + if (!new_buffhds)
> + return -ENOMEM;
> +
> + /* Data buffers cyclic list */
> + bh = new_buffhds;
> + i = n;
> + goto buffhds_first_it;
> + do {
> + bh->next = bh + 1;
> + ++bh;
> +buffhds_first_it:
> + bh->buf = kmalloc(FSG_BUFLEN, GFP_KERNEL);
> + if (unlikely(!bh->buf))
> + goto error_release;
> + } while (--i);
> + bh->next = new_buffhds;
> +
> + common->fsg_num_buffers = n;
> + kfree(common->buffhds);
Instead of kfree, fsg_common_free_buffers must be called here or
otherwise bh->buf won't get freed. In fact, I'd create an internal
implementation:
static void _fsg_common_free_buffers(struct fsg_buffhd *buffhds,
unsigned n) {
if (buffhds) {
struct fsg_buffhd *bh = buffhds;
while (n--) {
kfree(bh->buf);
++bh;
}
kfree(buffhds);
}
}
which could be used here, in error recovery below (error_release label),
and fsg_common_free_buffers().
> + common->buffhds = new_buffhds;
> +
> + return 0;
> +
> +error_release:
> + bh = new_buffhds;
> + i = n - i;
> + while (i--) {
> + kfree(bh->buf);
> + bh++;
> + };
> +
> + kfree(new_buffhds);
> +
> + return -ENOMEM;
> +}
> +
> +void fsg_common_free_buffers(struct fsg_common *common)
> +{
> + struct fsg_buffhd *bh;
> + int i;
> +
> + bh = common->buffhds;
> + i = common->fsg_num_buffers;
> + while (i--) {
> + kfree(bh->buf);
> + bh++;
> + };
> +
> + kfree(common->buffhds);
> + common->buffhds = NULL;
> +}
> +int fsg_common_set_nluns(struct fsg_common *common, int nluns)
> +{
> + struct fsg_lun **curlun;
> +
> + /* Find out how many LUNs there should be */
> + if (nluns < 1 || nluns > FSG_MAX_LUNS) {
> + pr_err("invalid number of LUNs: %u\n", nluns);
> + return -EINVAL;
> + }
> +
> + curlun = kcalloc(nluns, sizeof(*curlun), GFP_KERNEL);
> + if (unlikely(!curlun))
> + return -ENOMEM;
> +
> + common->luns = curlun;
> + common->nluns = nluns;
> +
> + pr_info("Number of LUNs=%d\n", common->nluns);
> +
> + return 0;
This function is fishy. What if someone calls it after bunch of LUNs is
created? If that happens, those luns are lost. At the very least, it
should check if common->luns is NULL and if it is, return -EBUSY or
something.
> +}
> +
> +void fsg_common_free_luns(struct fsg_common *common)
> +{
> + kfree(common->luns);
> + common->luns = NULL;
Hmm... This should at least check if any of the common->luns[i] is not
NULL, no? To be on the safe side, I'd just call fsg_common_remove_luns
at the beginning and change fsg_common_remove_luns to set the pointers
to NULL and check whether they are NULL before removing.
> +}
> +void fsg_common_remove_luns(struct fsg_common *common)
> +{
> + int i;
> +
> + for (i = 0; i < common->nluns; ++i)
> + fsg_common_remove_lun(common->luns[i], common->sysfs);
> +}
> +int fsg_common_create_lun(struct fsg_common *common, struct fsg_lun_config
> *cfg,
> + unsigned int id, const char *name,
> + const char **name_pfx)
> +{
> + struct fsg_lun *lun;
> + char *pathbuf;
> + int rc = -ENOMEM;
> + int name_len;
> +
> + if (!common->nluns || !common->luns)
> + return -ENODEV;
> +
> + if (common->luns[id])
> + return -EBUSY;
> +
> + name_len = strlen(name) + 1;
> + if (name_len > MAX_LUN_NAME_LEN)
> + return -ENAMETOOLONG;
> +
> + lun = kzalloc(sizeof(*lun), GFP_KERNEL);
> + if (!lun)
> + return -ENOMEM;
> +
> + lun->name = kzalloc(name_len, GFP_KERNEL);
lun->name = kstrndup(name, name_len, GFP_KERNEL);
> + if (!lun->name)
> + goto error_name;
> + lun->name_pfx = name_pfx;
> +
> + lun->cdrom = !!cfg->cdrom;
> + lun->ro = cfg->cdrom || cfg->ro;
> + lun->initially_ro = lun->ro;
> + lun->removable = cfg->removable;
To be on the safe side:
+ lun->removable = !!cfg->removable;
> +
> + common->luns[id] = lun;
> +
> + if (common->sysfs) {
> + lun->dev.release = fsg_lun_release;
> + lun->dev.parent = &common->gadget->dev;
> + dev_set_drvdata(&lun->dev, &common->filesem);
> + dev_set_name(&lun->dev, name);
> +
> + rc = fsg_common_add_sysfs(common, lun);
> + if (rc) {
> + pr_info("failed to register LUN%d: %d\n", id, rc);
> + goto error_sysfs;
> + }
> + }
> + sprintf(lun->name, name);
NO! sprintf(foo, bar) where bar is not a literal string is in vast
majority of cases a security bug. If bar is a literal string, it's
usually a waste of cycles.
And yes, this is not needed if you use kstrndup() like I pointed
earlier.
> +
> + if (cfg->filename) {
> + rc = fsg_lun_open(lun, cfg->filename);
> + if (rc)
> + goto error_lun;
> + } else if (!lun->removable) {
> + pr_err("no file given for LUN%d\n", id);
> + rc = -EINVAL;
> + goto error_lun;
> + }
> +
> + pathbuf = kmalloc(PATH_MAX, GFP_KERNEL);
> + {
> + char *p = "(no medium)";
> + if (fsg_lun_is_open(lun)) {
> + p = "(error)";
> + if (pathbuf) {
> + p = d_path(&lun->filp->f_path,
> + pathbuf, PATH_MAX);
> + if (IS_ERR(p))
> + p = "(error)";
> + }
> + }
> + pr_info("LUN: %s%s%sfile: %s\n",
> + lun->removable ? "removable " : "",
> + lun->ro ? "read only " : "",
> + lun->cdrom ? "CD-ROM " : "",
> + p);
> + }
> + kfree(pathbuf);
> +
> + return 0;
> +
> +error_lun:
> + if (common->sysfs) {
> + device_remove_file(&lun->dev, &dev_attr_nofua);
> + device_remove_file(&lun->dev,
> + lun->cdrom
> + ? &dev_attr_ro_cdrom
> + : &dev_attr_ro);
> + device_remove_file(&lun->dev,
> + lun->removable
> + ? &dev_attr_file
> + : &dev_attr_file_nonremovable);
> + device_unregister(&lun->dev);
> + }
> + fsg_lun_close(lun);
> +error_sysfs:
> + common->luns[id] = NULL;
> + kfree(lun->name);
> +error_name:
> + kfree(lun);
> + return rc;
> +}
> +
> +int fsg_common_create_luns(struct fsg_common *common, struct fsg_config *cfg)
> +{
> + char buf[40]; /* enough for 2^128 decimal */
> + int i, rc;
> +
> + for (i = 0; i < common->nluns; ++i) {
> + snprintf(buf, 40, "lun%d", i);
+ snprintf(buf, sizeof buf, "lun%d", i);
> + rc = fsg_common_create_lun(common, &cfg->luns[i], i, buf, NULL);
> + if (rc)
> + goto fail;
> + }
> +
> + pr_info("Number of LUNs=%d\n", common->nluns);
> +
> + return 0;
> +
> +fail:
> + while (--i >= 0)
> + fsg_common_remove_lun(common->luns[i], true);
If fsg_common_remove_lun checked whether a pointer is NULL, this could
be just a call to it instead of a loop.
> + return rc;
> +}
> +
> @@ -2697,6 +3095,8 @@ struct fsg_common *fsg_common_init(struct fsg_common
> *common,
> /* curlun->dev.driver = &fsg_driver.driver; XXX */
> dev_set_drvdata(&curlun->dev, &common->filesem);
> dev_set_name(&curlun->dev, "lun%d", i);
> + snprintf(curlun->name, MAX_LUN_NAME_LEN,
> + dev_name(&curlun->dev));
Like before, don't use snprintf() to copy strings. Use strlcpy().
>
> rc = device_register(&curlun->dev);
> if (rc) {
> @@ -2824,6 +3224,15 @@ error_release:
> return ERR_PTR(rc);
> }
>
> + if (likely(common->buffhds)) {
Use _fsg_common_free_buffers() described above.
> struct fsg_buffhd *bh = common->buffhds;
> unsigned i = common->fsg_num_buffers;
> do {
> kfree(bh->buf);
> } while (++bh, --i);
> - }
>
> - kfree(common->buffhds);
> + kfree(common->buffhds);
> + }
> if (common->free_storage_on_release)
> kfree(common);
> }
> diff --git a/drivers/usb/gadget/storage_common.h
> b/drivers/usb/gadget/storage_common.h
> index 1fcda2b..b2af2aa 100644
> --- a/drivers/usb/gadget/storage_common.h
> +++ b/drivers/usb/gadget/storage_common.h
> @@ -17,10 +17,42 @@
> #define VLDBG(lun, fmt, args...) do { } while (0)
> #endif /* VERBOSE_DEBUG */
>
> -#define LDBG(lun, fmt, args...) dev_dbg(&(lun)->dev, fmt, ## args)
> -#define LERROR(lun, fmt, args...) dev_err(&(lun)->dev, fmt, ## args)
> -#define LWARN(lun, fmt, args...) dev_warn(&(lun)->dev, fmt, ## args)
> -#define LINFO(lun, fmt, args...) dev_info(&(lun)->dev, fmt, ## args)
> +#define LDBG(lun, fmt, args...)
> \
> + do { \
> + if ((lun)->name_pfx && *(lun)->name_pfx) \
> + pr_debug("%s/%s: " fmt, *(lun)->name_pfx, \
> + (lun)->name, ## args); \
> + else \
> + pr_debug("%s: " fmt, (lun)->name, ## args); \
> + } while (0)
> +
> +#define LERROR(lun, fmt, args...) \
> + do { \
> + if ((lun)->name_pfx && *(lun)->name_pfx) \
> + pr_err("%s/%s: " fmt, *(lun)->name_pfx, \
> + (lun)->name, ## args); \
> + else \
> + pr_err("%s: " fmt, (lun)->name, ## args); \
> + } while (0)
> +
> +
> +#define LWARN(lun, fmt, args...) \
> + do { \
> + if ((lun)->name_pfx && *(lun)->name_pfx) \
> + pr_warn("%s/%s: " fmt, *(lun)->name_pfx, \
> + (lun)->name, ## args); \
> + else \
> + pr_warn("%s: " fmt, (lun)->name, ## args); \
> + } while (0)
> +
> +#define LINFO(lun, fmt, args...) \
> + do { \
> + if ((lun)->name_pfx && *(lun)->name_pfx) \
> + pr_info("%s/%s: " fmt, *(lun)->name_pfx, \
> + (lun)->name, ## args); \
> + else \
> + pr_info("%s: " fmt, (lun)->name, ## args); \
> + } while (0)
+#define _LMSG(func, lun, fmt, args...) do { \
+ if ((lun)->name_pfx && *(lun)->name_pfx) \
+ func("%s/%s: " fmt, *(lun)->name_pfx, \
+ (lun)->name, ## args); \
+ else \
+ func("%s: " fmt, (lun)->name, ## args); \
+} while (0)
+#define LDBG(lun, fmt, args...) _LMSG(pr_debug, lun, fmt, ## args)
+#define LERROR(lun, fmt, args...) _LMSG(pr_err, lun, fmt, ## args)
+#define LWARN(lun, fmt, args...) _LMSG(pr_warn, lun, fmt, ## args)
+#define LINFO(lun, fmt, args...) _LMSG(pr_info, lun, fmt, ## args)
>
> #ifdef DUMP_MSGS
>
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michał “mina86” Nazarewicz (o o)
ooo +----<email/xmpp: m...@google.com>--------------ooO--(_)--Ooo--
signature.asc
Description: PGP signature
