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

[Haris Iqbal <haris.iqbal@linux.dev>]

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@lst.de>
>>> Assisted-by: Claude:claude-opus-5
>>> Signed-off-by: Md Haris Iqbal <haris.iqbal@linux.dev>
>>> ---
>>>   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.

What can make it simpler is allocating "tag" on stack, and then use 
match_strlcpy() instead of match_strdup().

> 
>>
>> to keep the code a bit more readable.
>>
>> Otherwise looks good:
>>
>> Reviewed-by: Christoph Hellwig <hch@lst.de>