If allocation fails, the ida_simple_get() will return error number.
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 <[email protected]>
---
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
On Tue, Jan 11, 2022 at 03:17:51PM +0800, Jiasheng Jiang wrote:
> If allocation fails, the ida_simple_get() will return error number.
How can allocation fail? Have you been able to trigger this?
> 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 <[email protected]>
> ---
> 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?
As I have said before, I'm not going to take changes from you unless you
can prove they have been tested, due to all of the previous problems in
your submitted patches.
greg k-h
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 <[email protected]>
>> ---
>> 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