Re: [PATCH 8/9] block: add configurable error injection

[Keith Busch <kbusch@kernel.org>]

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;
> +}