Re: [v3 for-next 1/3] block: Reject unknown status tags in error injection rules

[Haris Iqbal]

On 11/5/26 21:18, Mohamed Khalfella wrote:
> On Thu 2026-11-05 21:00:27 +0100, Haris Iqbal wrote:
> > On 10/21/26 00:34, Mohamed Khalfella wrote:
> > > On Mon 2026-10-05 10:49:17 +0200, Christoph Hellwig wrote:
> > > > On Tue, Sep 29, 2026 at 12:16:32AM +0200, Md Haris Iqbal wrote:
> > > > > tag_to_blk_status() returns BLK_STS_OK both for the "OK" tag and for a
> > > > > tag it does not recognize, so a caller cannot tell the two apart. The
> > > > > block error injection code (which is the sole user of this function
> > > > > currently) relies on error_inject_add() later to reject adding a rule with
> > > > > status=BLK_STS_OK. This is okay for the current state of error injection.
> > > > >
> > > > > The delay option added in the next commit makes the rule with status=OK
> > > > > valid, meaning the function tag_to_blk_status() now needs to explicitly
> > > > > match BLK_STS_OK for it, and fail for a tag it does not recognize. Hence
> > > > > make tag_to_blk_status() take a second param to update the matched status,
> > > > > and return true in case of a successful match. If a match is not found,
> > > > > the function tag_to_blk_status() returns false and *status remains
> > > > > unchanged.
> > > > >
> > > > > A repeated status= where an invalid tag comes first is now rejected
> > > > > instead of being overridden by the later one.
> > > > >
> > > > > Cc: Christoph Hellwig <hch@xxxxxx>
> > > > > Assisted-by: Claude:claude-opus-5
> > > > > Signed-off-by: Md Haris Iqbal <haris.iqbal@xxxxxxxxx>
> > > > > ---
> > > > >    block/blk-core.c        | 14 ++++++--------
> > > > >    block/blk.h             |  2 +-
> > > > >    block/error-injection.c |  7 ++++---
> > > > >    3 files changed, 11 insertions(+), 12 deletions(-)
> > > > >
> > > > > diff --git a/block/blk-core.c b/block/blk-core.c
> > > > > index 13dc70e8f55d..c45846b9c50d 100644
> > > > > --- a/block/blk-core.c
> > > > > +++ b/block/blk-core.c
> > > > > @@ -225,21 +225,19 @@ const char *blk_status_to_tag(blk_status_t status)
> > > > >    	return blk_errors[idx].tag;
> > > > >    }
> > > > >    
> > > > > -blk_status_t tag_to_blk_status(const char *tag)
> > > > > +bool tag_to_blk_status(const char *tag, blk_status_t *status)
> > > > >    {
> > > > >    	int i;
> > > > >    
> > > > >    	for (i = 0; i < ARRAY_SIZE(blk_errors); i++) {
> > > > >    		if (blk_errors[i].tag &&
> > > > > -		    !strcmp(blk_errors[i].tag, tag))
> > > > > -			return (__force blk_status_t)i;
> > > > > +		    !strcmp(blk_errors[i].tag, tag)) {
> > > > > +			*status = (__force blk_status_t)i;
> > > > > +			return true;
> > > > > +		}
> > > > >    	}
> > > > >    
> > > > > -	/*
> > > > > -	 * Return BLK_STS_OK for mismatches as this function is intended to
> > > > > -	 * parse error status values.
> > > > > -	 */
> > > > > -	return BLK_STS_OK;
> > > > > +	return false;
> > > > >    }
> > > > >    
> > > > >    /**
> > > > > diff --git a/block/blk.h b/block/blk.h
> > > > > index 2cc03aa54c53..5fa54162c686 100644
> > > > > --- a/block/blk.h
> > > > > +++ b/block/blk.h
> > > > > @@ -52,7 +52,7 @@ void blk_free_flush_queue(struct blk_flush_queue *q);
> > > > >    
> > > > >    const char *blk_status_to_str(blk_status_t status);
> > > > >    const char *blk_status_to_tag(blk_status_t status);
> > > > > -blk_status_t tag_to_blk_status(const char *tag);
> > > > > +bool tag_to_blk_status(const char *tag, blk_status_t *status);
> > > > >    enum req_op str_to_blk_op(const char *op);
> > > > >    
> > > > >    bool __blk_mq_unfreeze_queue(struct request_queue *q, bool force_atomic);
> > > > > diff --git a/block/error-injection.c b/block/error-injection.c
> > > > > index e14bc4b723ef..db543fe27630 100644
> > > > > --- a/block/error-injection.c
> > > > > +++ b/block/error-injection.c
> > > > > @@ -171,15 +171,16 @@ static int match_op(substring_t *args, enum req_op *op)
> > > > >    static int match_status(substring_t *args, blk_status_t *status)
> > > > >    {
> > > > >    	const char *tag;
> > > > > +	bool found;
> > > > >    
> > > > >    	tag = match_strdup(args);
> > > > >    	if (!tag)
> > > > >    		return -ENOMEM;
> > > > > -	*status = tag_to_blk_status(tag);
> > > > > -	if (!*status)
> > > > > +	found = tag_to_blk_status(tag, status);
> > > > > +	if (!found)
> > > > >    		pr_warn("invalid status '%s'\n", tag);
> > > > >    	kfree(tag);
> > > > > -	return 0;
> > > > > +	return found ? 0 : -EINVAL;
> > > >
> > > > 	if (!found) {
> > > >     		pr_warn("invalid status '%s'\n", tag);
> > > > 		return -EINVAL;
> > > > 	}
> > > > 	return 0;
> >
> > As Mohamed mentioned below, "tag" would leak then.
> >
> > > Doing it this way will result in use-after-free. "tag" has already been
> > > freed above.
> > >
> > > Maybe change tag_to_blk_status() to return an int (0 on success and
> > > -EINVAL on failure) and use its return value without mapping it?
> >
> > I am not sure I get your suggestion completely. Even if we change the
> > return type of tag_to_blk_status(), we would have to free the "tag"
> > before returning.
>
> Changing tag_to_blk_status() to return an int will not have an effect or
> "tag" like you mentioned. But you will not need to map the return from
> that function.
>
> return found ? 0 : -EINVAL;
>
> The line above just returns what tag_to_blk_status() returned.

I would prefer to keep tag_to_blk_status() as a bool lookup. It's the 
reverse of blk_status_to_tag(), and which errno to return is 
match_status()'s call.

> > What can make it simpler is allocating "tag" on stack, and then use
> > match_strlcpy() instead of match_strdup().
>
> That can work too. Just make sure we limit the string size somehow.

This I can do in v4. It should effectively remove the ternary operator too.