lkml.org 
[lkml]   [2023]   [Apr]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: BUG FIX: [PATCH RFC v3] memstick_check() memleak in kernel 6.1.0+ introduced pre 4.17
    From
    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?"

    \
     
     \ /
      Last update: 2023-04-01 12:39    [W:3.294 / U:0.040 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site