On Mon, Dec 02 2013, Andrzej Pietrasiewicz wrote:
> - ffs_dev_lock();
> for (i = 0; i < func_num; i++) {
> - ffs_tab[i] = ffs_alloc_dev();
> - if (IS_ERR(ffs_tab[i])) {
> - ret = PTR_ERR(ffs_tab[i]);
> - --i;
> + /*
> + * usb_get_function_instance() takes ffs_lock,
> + * so we must not take it here...
> + */
> + fi_ffs[i] = usb_get_function_instance("ffs");
Technically, usb_get_function_instance doesn't take ffs_lock, but
perhaps some function called from it does. Could you explain in the
comment what *actually* takes the lock and how it is called from
usb_get_function_instance?
> + if (IS_ERR(fi_ffs[i])) {
> + ret = PTR_ERR(fi_ffs[i]);
> goto no_dev;
> }
> - ret = ffs_single_dev(ffs_tab[i], gfs_single_func);
> + /*
> + * ... but we need it below
> + *
> + * What happens if some thread of execution kicks in between
> + * usb_get_function_instance() above and ffs_dev_lock() below
> + * and (after taking the lock) accesses the devices list?
> + *
> + * The other thread can be a configfs-based gadget only,
> + * since we cannot load this (g_ffs) module twice.
> + *
> + * For the device to be used it needs so be acquired first.
> + * ffs_acquire_dev() finds the device by name and it will get
> + * some string to match against as a device name given for
> + * mount by the user. So devices which don't have their name set
> + * will never be matched, so our device just created above will
> + * not be abused.
> + *
> + * There is an exception to the matching rule: the g_ffs, if
> + * no functions= parameter is given on module load, will assume
> + * there is only one "device" and will accept any "device" name
> + * given for mount. If this is the case here, then the other
> + * thread potentially creates another device and the device list
> + * consists no more of a single device. Then setting the device
> + * created above as "single" will not succeed and g_ffs will
> + * refuse loading with EBUSY.
> + */
> + ffs_dev_lock();
I'm wondering whether it would be feasible to change
usb_get_function_instance's path so that ffs_lock is not taken. This
would save us this long comment… ;)
> + opts = to_f_fs_opts(fi_ffs[i]);
> + ret = ffs_single_dev(opts->dev, gfs_single_func);
> if (ret)
> - goto no_dev;
> + goto no_dev_unlock;
> if (!gfs_single_func) {
> - ret = ffs_name_dev(ffs_tab[i], func_names[i]);
> + ret = ffs_name_dev(opts->dev, func_names[i]);
> if (ret)
> - goto no_dev;
> + goto no_dev_unlock;
> }
> - ffs_tab[i]->ffs_ready_callback = functionfs_ready_callback;
> - ffs_tab[i]->ffs_closed_callback = functionfs_closed_callback;
> + opts->dev->ffs_ready_callback = functionfs_ready_callback;
> + opts->dev->ffs_closed_callback = functionfs_closed_callback;
> + opts->dev->ffs_acquire_dev_callback = functionfs_acquire_dev;
> + opts->dev->ffs_release_dev_callback = functionfs_release_dev;
> + opts->no_configfs = true;
> + ffs_dev_unlock();
> }
> - ffs_dev_unlock();
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michał “mina86” Nazarewicz (o o)
ooo +--<mpn@xxxxxxxxxx>--<xmpp:mina86@xxxxxxxxxx>--ooO--(_)--Ooo--
Attachment:
signature.asc
Description: PGP signature
