On Tue, Jun 03, 2026 at 07:45:40AM +0200, Christoph Hellwig wrote:
> +static int error_inject_add(struct gendisk *disk, enum req_op op,
> + sector_t start, u64 nr_sectors, blk_status_t status,
> + unsigned int chance)
> +{
> + struct blk_error_inject *inj;
> +
> + if (op == REQ_OP_LAST)
> + return -EINVAL;
> + if (status == BLK_STS_OK)
> + return -EINVAL;
> + if (U64_MAX - nr_sectors < start)
> + return -EINVAL;
> +
> + if (!nr_sectors)
> + nr_sectors = U64_MAX;
> +
...
> +
> + inj->op = op;
> + inj->start = start;
> + inj->end = start + nr_sectors - 1;
When nr_sectors is 0, it is reset to U64_MAX so overflows if start > 1.
I think you want to remove overriding nr_sectors to U64_MAX and do:
if (!nr_sectors)
inj->end = U64_MAX;
else if (U64_MAX - nr_sectors < start )
return -EINVAL;
else
inj->end = start + nr_sectors - 1;
> + inj->status = status;
> + inj->chance = chance;
> +
> + /*
> + * Add to the front of the list so that newer entries can partially
> + * override other entries. This also intentional allows duplicate
> + * entries as there is no real reason to reject them.
> + */
> + mutex_lock(&disk->error_injection_lock);
> + if (!disk_live(disk)) {
> + mutex_unlock(&disk->error_injection_lock);
> + return -EINVAL;
I think we've leaked 'inj' in this error case.
> + }
> + list_add(&inj->entry, &disk->error_injection_list);
The __blk_error_inject interates this list with
"list_for_each_entry_rcu", so shouldn't this be list_add_rcu to match?
> + mutex_unlock(&disk->error_injection_lock);
> +
> + bdev_set_flag(disk->part0, BD_MAKE_IT_FAIL);
> + return 0;
> +}
<snip>
> +static const match_table_t opt_tokens = {
> + { Opt_add, "add", },
> + { Opt_removeall, "removeall", },
> + { Opt_op, "op=%s", },
> + { Opt_start, "start=%u" },
> + { Opt_nr_sectors, "nr_sectors=%u" },
Shouldn't start and nr_sectors use %llu?
> +static ssize_t blk_error_injection_write(struct file *file,
> + const char __user *ubuf, size_t count, loff_t *pos)
> +{
...
> + options = memdup_user_nul(ubuf, count);
> + if (!options)
> + return -ENOMEM;
> +
On failure, memdup_user_nul returns an ERR_PTR rather than NULL.
if (IS_ERR(options))
return PTR_ERR(options);
> + case Removeall:
> + if (option_mask & ~Opt_removeall)
> + return -EINVAL;
Leaking "options"? Should this be:
if (option_mask & ~Opt_removeall) {
ret = -EINVAL;
goto out_free_options;
}
?
> + error_inject_removall(disk);
> + break;
> + default:
> + ret = -EINVAL;
> + }
> +
> + if (!ret)
> + ret = count;
> +out_free_options:
> + kfree(options);
> + return ret;
> +}
