Re: [PATCH V1 for-next 1/2] IB/uverbs: Enable device removal when there are active user space applications

Or Gerlitz
2014-11-18T14:40:42Z

On 11/18/2014 2:11 PM, Yishai Hadas wrote:
> @@ -599,6 +638,8 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
> struct ib_uverbs_file *file = filp->private_data;
> struct ib_uverbs_cmd_hdr hdr;
> __u32 flags;
> + int srcu_key;
> + ssize_t ret;
>
> if (count < sizeof hdr)
> return -EINVAL;
> @@ -606,6 +647,12 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
> if (copy_from_user(&hdr, buf, sizeof hdr))
> return -EFAULT;
>
> + srcu_key = srcu_read_lock(&file->device->disassociate_srcu);
> + if (file->device->disassociated) {
> + ret = -EIO;
> + goto out;
> + }
> +
> flags = (hdr.command &
> IB_USER_VERBS_CMD_FLAGS_MASK) >> IB_USER_VERBS_CMD_FLAGS_SHIFT;
>
> @@ -613,26 +660,36 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
> __u32 command;
>
> if (hdr.command & ~(__u32)(IB_USER_VERBS_CMD_FLAGS_MASK |
> - IB_USER_VERBS_CMD_COMMAND_MASK))
> - return -EINVAL;
> + IB_USER_VERBS_CMD_COMMAND_MASK)) {
> + ret = -EINVAL;
> + goto out;
> + }

put this change and it's such in a pre-patch that does refactoring of
the ib_uverbs_write() code, to make the volume and amount of changes
introduced by this patch smaller

>
> command = hdr.command & IB_USER_VERBS_CMD_COMMAND_MASK;
>
> if (command >= ARRAY_SIZE(uverbs_cmd_table) ||
> - !uverbs_cmd_table[command])
> - return -EINVAL;
> + !uverbs_cmd_table[command]) {
> + ret = -EINVAL;
> + goto out;
> + }
>
> if (!file->ucontext &&
> - command != IB_USER_VERBS_CMD_GET_CONTEXT)
> - return -EINVAL;
> + command != IB_USER_VERBS_CMD_GET_CONTEXT) {
> + ret = -EINVAL;
> + goto out;
> + }
>
> - if (!(file->device->ib_dev->uverbs_cmd_mask & (1ull << command)))
> - return -ENOSYS;
> + if (!(file->device->ib_dev->uverbs_cmd_mask & (1ull << command))) {
> + ret = -ENOSYS;
> + goto out;
> + }
>
> - if (hdr.in_words * 4 != count)
> - return -EINVAL;
> + if (hdr.in_words * 4 != count) {
> + ret = -EINVAL;
> + goto out;
> + }
>
> - return uverbs_cmd_table[command](file,
> + ret = uverbs_cmd_table[command](file,
> buf + sizeof(hdr),
> hdr.in_words * 4,
> hdr.out_words * 4);
> @@ -647,47 +704,69 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
> size_t written_count = count;
>
> if (hdr.command & ~(__u32)(IB_USER_VERBS_CMD_FLAGS_MASK |
> - IB_USER_VERBS_CMD_COMMAND_MASK))
> - return -EINVAL;
> + IB_USER_VERBS_CMD_COMMAND_MASK)) {
> + ret = -EINVAL;
> + goto out;
> + }
>
> command = hdr.command & IB_USER_VERBS_CMD_COMMAND_MASK;
>
> if (command >= ARRAY_SIZE(uverbs_ex_cmd_table) ||
> - !uverbs_ex_cmd_table[command])
> - return -ENOSYS;
> + !uverbs_ex_cmd_table[command]) {
> + ret = -ENOSYS;
> + goto out;
> + }
>
> - if (!file->ucontext)
> - return -EINVAL;
> + if (!file->ucontext) {
> + ret = -EINVAL;
> + goto out;
> + }
>
> - if (!(file->device->ib_dev->uverbs_ex_cmd_mask & (1ull << command)))
> - return -ENOSYS;
> + if (!(file->device->ib_dev->uverbs_ex_cmd_mask & (1ull << command))) {
> + ret = -ENOSYS;
> + goto out;
> + }
>
> - if (count < (sizeof(hdr) + sizeof(ex_hdr)))
> - return -EINVAL;
> + if (count < (sizeof(hdr) + sizeof(ex_hdr))) {
> + ret = -EINVAL;
> + goto out;
> + }
>
> - if (copy_from_user(&ex_hdr, buf + sizeof(hdr), sizeof(ex_hdr)))
> - return -EFAULT;
> + if (copy_from_user(&ex_hdr, buf + sizeof(hdr), sizeof(ex_hdr))) {
> + ret = -EFAULT;
> + goto out;
> + }
>
> count -= sizeof(hdr) + sizeof(ex_hdr);
> buf += sizeof(hdr) + sizeof(ex_hdr);
>
> - if ((hdr.in_words + ex_hdr.provider_in_words) * 8 != count)
> - return -EINVAL;
> + if ((hdr.in_words + ex_hdr.provider_in_words) * 8 != count) {
> + ret = -EINVAL;
> + goto out;
> + }
>
> - if (ex_hdr.cmd_hdr_reserved)
> - return -EINVAL;
> + if (ex_hdr.cmd_hdr_reserved) {
> + ret = -EINVAL;
> + goto out;
> + }
>
> if (ex_hdr.response) {
> - if (!hdr.out_words && !ex_hdr.provider_out_words)
> - return -EINVAL;
> + if (!hdr.out_words && !ex_hdr.provider_out_words) {
> + ret = -EINVAL;
> + goto out;
> + }
>
> if (!access_ok(VERIFY_WRITE,
> (void __user *) (unsigned long) ex_hdr.response,
> - (hdr.out_words + ex_hdr.provider_out_words) * 8))
> - return -EFAULT;
> + (hdr.out_words + ex_hdr.provider_out_words) * 8)) {
> + ret = -EFAULT;
> + goto out;
> + }
> } else {
> - if (hdr.out_words || ex_hdr.provider_out_words)
> - return -EINVAL;
> + if (hdr.out_words || ex_hdr.provider_out_words) {
> + ret = -EINVAL;
> + goto out;
> + }
> }
>
> INIT_UDATA_BUF_OR_NULL(&ucore, buf, (unsigned long) ex_hdr.response,
> @@ -704,22 +783,37 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
> &uhw);
>
> if (err)
> - return err;
> -
> - return written_count;
> + ret = err;
> + else
> + ret = written_count;
> + } else {
> + ret = -ENOSYS;
> }
>
> - return -ENOSYS;
> +out:
> + srcu_read_unlock(&file->device->disassociate_srcu, srcu_key);
> + return ret;
> }