On 01. 04. 2023. 12:14, Greg KH wrote:
> On Sat, Apr 01, 2023 at 12:01:43PM +0200, Mirsad Goran Todorovac wrote:
>> On 01. 04. 2023. 11:52, Mirsad Goran Todorovac wrote:
>>> On 01. 04. 2023. 11:23, Greg KH wrote:
>>>> On Sat, Apr 01, 2023 at 11:18:19AM +0200, Greg KH wrote:
>>>>> On Sat, Apr 01, 2023 at 08:33:36AM +0200, Greg KH wrote:
>>>>>> On Sat, Apr 01, 2023 at 08:28:07AM +0200, Greg KH wrote:
>>>>>>> On Sat, Apr 01, 2023 at 08:23:26AM +0200, Mirsad Goran Todorovac wrote:
>>>>>>>>> This patch is implying that anyone who calls "dev_set_name()" also has
>>>>>>>>> to do this hack, which shouldn't be the case at all.
>>>>>>>>>
>>>>>>>>> thanks,
>>>>>>>>>
>>>>>>>>> greg k-h
>>>>>>>>
>>>>>>>> This is my best guess. Unless there is dev_free_name() or kobject_free_name(), I don't
>>>>>>>> see a more sensible way to patch this up.
>>>>>>>
>>>>>>> In sleeping on this, I think this has to move to the driver core. I
>>>>>>> don't understand why we haven't seen this before, except maybe no one
>>>>>>> has really noticed before (i.e. we haven't had good leak detection tools
>>>>>>> that run with removable devices?)
>>>>>>>
>>>>>>> Anyway, let me see if I can come up with something this weekend, give me
>>>>>>> a chance...
>>>>>>
>>>>>> Wait, no, this already should be handled by the kobject core, look at
>>>>>> kobject_cleanup(), at the bottom. So your change should be merely
>>>>>> duplicating the logic there that already runs when the struct device is
>>>>>> freed, right?
>>>>>>
>>>>>> So I don't understand why your change works, odd. I need more coffee...
>>>>>
>>>>> I think you got half of the change correctly. This init code is a maze
>>>>> of twisty passages, let me take your patch and tweak it a bit into
>>>>> something that I think should work. This looks to be only a memstick
>>>>> issue, not a driver core issue (which makes me feel better.)
>>>>
>>>> Oops, forgot the patch. Can you try this change here and let me know if
>>>> that solves the problem or not? I have compile-tested it only, so I
>>>> have no idea if it works.
>>>>
>>>> If this does work, I'll make up a "real" function to replace the
>>>> horrible dev.kobj.name mess that a driver would have to do here as it
>>>> shouldn't be required that a driver author knows the internals of the
>>>> driver core that well...
>>>>
>>>> thanks,
>>>>
>>>> greg k-h
>>>>
>>>> --------------------
>>>>
>>>>
>>>> diff --git a/drivers/memstick/core/memstick.c b/drivers/memstick/core/memstick.c
>>>> index bf7667845459..bbfaf6536903 100644
>>>> --- a/drivers/memstick/core/memstick.c
>>>> +++ b/drivers/memstick/core/memstick.c
>>>> @@ -410,6 +410,7 @@ static struct memstick_dev *memstick_alloc_card(struct memstick_host *host)
>>>> return card;
>>>> err_out:
>>>> host->card = old_card;
>>>> + kfree_const(card->dev.kobj.name);
>>>> kfree(card);
>>>> return NULL;
>>>> }
>>>> @@ -468,8 +469,10 @@ static void memstick_check(struct work_struct *work)
>>>> put_device(&card->dev);
>>>> host->card = NULL;
>>>> }
>>>> - } else
>>>> + } else {
>>>> + kfree_const(card->dev.kobj.name);
>>>> kfree(card);
>>>> + }
>>>> }
>>>>
>>>> out_power_off:
>>>
>>> I thought of this version, but I am not sure about tracking the device_register() and
>>> device_unregister() calls?
>>>
>>> put_device() calls put_kobject() which frees the const char *kobj.name ...
>>>
>>> I thought how host cannot just be kfree()d when host->card is still allocated.
>>> And it is a pointer. That also seems to me like a bug :-/
>>>
>>> Kind regards,
>>> Mirsad
>>>
>>> ---
>>> diff --git a/drivers/memstick/core/memstick.c b/drivers/memstick/core/memstick.c
>>> index bf7667845459..46c7bda9715d 100644
>>> --- a/drivers/memstick/core/memstick.c
>>> +++ b/drivers/memstick/core/memstick.c
>>> @@ -179,6 +179,8 @@ static void memstick_free(struct device *dev)
>>> {
>>> struct memstick_host *host = container_of(dev, struct memstick_host,
>>> dev);
>>> + if (host->card && host->card->dev)
>>> + put_device(&host->card->dev);
>>> kfree(host);
>>> }
>>>
>>> @@ -410,7 +412,7 @@ static struct memstick_dev *memstick_alloc_card(struct memstick_host *host)
>>> return card;
>>> err_out:
>>> host->card = old_card;
>>> - kfree(card);
>>> + put_device(&card->dev);
>>> return NULL;
>>> }
>>>
>>> @@ -468,8 +470,9 @@ static void memstick_check(struct work_struct *work)
>>> put_device(&card->dev);
>>> host->card = NULL;
>>> }
>>> - } else
>>> - kfree(card);
>>> + } else {
>>> + put_device(&card->dev);
>>> + }
>>> }
>>>
>>> out_power_off:
>>
>> Thousand apologies, the previous version had a compilation error. I've sent the untested
>> version.
>>
>> I must have become over-confident. But they say that a mistake that makes you humbled
>> is better than success that makes you arrogant :-|
>>
>> I would like your opinion on the patch before I actually start the kernel, for I won't
>> be able to reboot clean that machine if it hangs in kernel until Tuesday :-(
>>
>> It seems that put_device() would call the release method of the device and kfree() in
>> it, but I cannot say anything about the side effects, for I do not know the source so
>> well ...
>>
>> Kind regards,
>> Mirsad
>>
>> ---
>> diff --git a/drivers/memstick/core/memstick.c b/drivers/memstick/core/memstick.c
>> index bf7667845459..c63250322e26 100644
>> --- a/drivers/memstick/core/memstick.c
>> +++ b/drivers/memstick/core/memstick.c
>> @@ -179,6 +179,8 @@ static void memstick_free(struct device *dev)
>> {
>> struct memstick_host *host = container_of(dev, struct memstick_host,
>> dev);
>> + if (host->card)
>> + put_device(&host->card->dev);
>
> This isn't going to work as at this moment in time, the last reference
> count has already happened, causing this release callback to be called,
> so that the bus driver can free the memory for the device.
>
> So you would be calling put_device() on a device already has 0 for a
> reference count :)
>
>> kfree(host);
>> }
>>
>> @@ -410,7 +412,7 @@ static struct memstick_dev *memstick_alloc_card(struct memstick_host *host)
>> return card;
>> err_out:
>> host->card = old_card;
>> - kfree(card);
>> + put_device(&card->dev);
>
> No, the device was not registered here yet, right? That would be
> required _IFF_ there was a call to device_register().
>
>> return NULL;
>> }
>>
>> @@ -468,8 +470,9 @@ static void memstick_check(struct work_struct *work)
>> put_device(&card->dev);
>> host->card = NULL;
>> }
>> - } else
>> - kfree(card);
>> + } else {
>> + put_device(&card->dev);
>
> Same here, unless I'm reading this wrong, device_register() had not been
> called yet, which is why the kfree was required (same for the above
> call).
>
> But hey, this driver really is a maze of twisty callbacks and workqueues
> and complexity, for no obvious reason to me (maybe because of some async
> requirement for memstick devices? Thankfully I no longer have this
> hardware...) So I might be totally wrong...
>
> I would recommend trying my version first, it "shouldn't" cause anything
> worse to happen from what you have today, but hey, that's just my guess.
>
> thanks,
>
> greg k-h
Hi Mr. Greg,
Thank you for the additional insight.
I will build your patch ASAP and give feedback.
Kind regards,
Mirsad
--
Mirsad Goran Todorovac
Sistem inženjer
Grafički fakultet | Akademija likovnih umjetnosti
Sveučilište u Zagrebu
System engineer
Faculty of Graphic Arts | Academy of Fine Arts
University of Zagreb, Republic of Croatia
The European Union
"I see something approaching fast ... Will it be friends with me?"
