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

[Christoph Hellwig <hch@lst.de>]

On Tue, Jun 02, 2026 at 10:42:35AM +0100, Keith Busch wrote:
> When nr_sectors is 0, it is reset to U64_MAX so overflows if start > 1.

Yeah.

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

I ended up ordering a bit differently for better readability, but
yes.

> > +	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.

Yes.

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

Yes.

> > +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?

lib/parser.c doesn't use those prefixes, it's a bit weird.

> > +	if (!options)
> > +		return -ENOMEM;
> > +
> 
> On failure, memdup_user_nul returns an ERR_PTR rather than NULL.
> 
> 	if (IS_ERR(options))
> 		return PTR_ERR(options);

Aarg, annoying.  Because memdup_user does return NULL :(

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

Yes.