Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Jiasheng Jiang <jiasheng@iscas.ac.cn>
To:     gregkh@linuxfoundation.org
Cc:     heikki.krogerus@linux.intel.com, linux-usb@vger.kernel.org,
        linux-kernel@vger.kernel.org, Jiasheng Jiang <jiasheng@iscas.ac.cn>
Subject: Re: Re: [PATCH] usb: typec: Check error number after calling ida_simple_get
Date:   Thu, 20 Jan 2022 16:51:13 +0800
Message-Id: <20220120085113.1854945-1-jiasheng@iscas.ac.cn>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

On Tue, Jan 11, 2022 at 04:39:13PM +0800, Greg KH wrote:
>> If allocation fails, the ida_simple_get() will return error number.
>
> How can allocation fail?  Have you been able to trigger this?

Thanks for your advice.
I have already tested the altmode_id_get() by kunit and qemu-system-x86_64.
Firstly I used qemu to create a virtual machine with only 32M memory.
And then I wrote a kunit test that trying to make altmode_id_get() fail.
The test continually allocates memory until the memory doesn't have
enough space allocated for 8 bytes at first.
Then it will call the altmode_id_get() and use KUNIT_EXPECT_EQ() to
test whether the return value is -ENOMEM.
Because it is tested under extremely harsh conditions, I close the OOM
killer, in order to prevent the automatically killing for the process
which will make the memory deadlock.
To be convenient, I used '__GFP_NORETRY' when allocation instead of
the above setting, as it can achieve the same goal that the allocation
will not retry that causing the OOM killer.
As a result, the test passed which means that altmode_id_get()
really returned -ENOMEM.

>> So altmode_id_get() may return error number.
>> And then id will be used in altmode_id_remove, causing the BUG_ON().
>> Or it will be assigned to alt->id.
>> Therefore, it should be better to check it and return error if fails,
>> like the ida_simple_get() in typec_register_port().
>> 
>> Fixes: 8a37d87d72f0 ("usb: typec: Bus type for alternate modes")
>> Signed-off-by: Jiasheng Jiang <jiasheng@iscas.ac.cn>
>> ---
>>  drivers/usb/typec/class.c | 3 +++
>>  1 file changed, 3 insertions(+)
>> 
>> diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
>> index aeef453aa658..67b3670ede99 100644
>> --- a/drivers/usb/typec/class.c
>> +++ b/drivers/usb/typec/class.c
>> @@ -516,6 +516,9 @@ typec_register_altmode(struct device *parent,
>>  	struct altmode *alt;
>>  	int ret;
>>  
>> +	if (id < 0)
>> +		return ERR_PTR(id);
>> +
>>  	alt = kzalloc(sizeof(*alt), GFP_KERNEL);
>>  	if (!alt) {
>>  		altmode_id_remove(parent, id);
>> -- 
>> 2.25.1
>> 
>
> How did you test that this change will work properly?

And this time I also use the kunit to test the patch.
I added the check 'if (id < 0) return;' between the altmode_id_get()
and KUNIT_FAIL().
As a result the kunit test passed, which means that the patch works properly.

Therefore, I think it is true that the allocation can fail and
altmode_id_get() will return -ENOMEM under the conditions that
the freed memory is lower that 8 bytes and OOM killer is closed.

Moreover, there is a check for the allocation of 'alt' in the
same function.
To maintain the code consistency, it should be better to add the
check for 'id' too.

Sincerely thanks,
Jiang