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.
