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

    --
    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:02    [W:4.668 / U:0.000 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site