linux-kernel - RE: [PATCH] alarmtimer: Fix rebind failure

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <OS0PR01MB59221716AD7E6A10CDB295D986F9A@OS0PR01MB5922.jpnprd01.prod.outlook.com>
Date:   Wed, 20 Sep 2023 12:47:10 +0000
From:   Biju Das <biju.das.jz@...renesas.com>
To:     Geert Uytterhoeven <geert@...ux-m68k.org>
CC:     Thomas Gleixner <tglx@...utronix.de>,
        Alessandro Zummo <a.zummo@...ertech.it>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        John Stultz <jstultz@...gle.com>,
        Stephen Boyd <sboyd@...nel.org>,
        Geert Uytterhoeven <geert+renesas@...der.be>,
        Biju Das <biju.das.au@...il.com>,
        "linux-rtc@...r.kernel.org" <linux-rtc@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-renesas-soc@...r.kernel.org" 
        <linux-renesas-soc@...r.kernel.org>
Subject: RE: [PATCH] alarmtimer: Fix rebind failure

Hi Geert Uytterhoeven,

Thanks for the feedback.

> Subject: Re: [PATCH] alarmtimer: Fix rebind failure
> 
> Hi Biju,
> 
> On Wed, Sep 20, 2023 at 1:59 PM Biju Das <biju.das.jz@...renesas.com>
> wrote:
> > The resources allocated in alarmtimer_rtc_add_device() are not freed
> > leading to re-bind failure for the endpoint driver. Fix this issue by
> > adding alarmtimer_rtc_remove_device().
> >
> > Signed-off-by: Biju Das <biju.das.jz@...renesas.com>
> 
> Thanks for your patch!
> 
> Does this need a Fixes tag?

I think so, as it breaks unbind/bind on lot of RTC drivers.

There are 2 commits, I will add both as fixes tag.

c79108bd19a8 ("alarmtimer: Make alarmtimer platform device child of RTC device")

7c94caca877b ("alarmtimer: Use wakeup source from alarmtimer platform device"

> 
> > --- a/kernel/time/alarmtimer.c
> > +++ b/kernel/time/alarmtimer.c
> > @@ -61,6 +61,7 @@ static DEFINE_SPINLOCK(freezer_delta_lock);
> >  /* rtc timer and device for setting alarm wakeups at suspend */
> >  static struct rtc_timer                rtctimer;
> >  static struct rtc_device       *rtcdev;
> > +static struct platform_device  *rtc_pdev;
> >  static DEFINE_SPINLOCK(rtcdev_lock);
> >
> >  /**
> > @@ -109,6 +110,7 @@ static int alarmtimer_rtc_add_device(struct device
> *dev)
> >                 }
> >
> >                 rtcdev = rtc;
> > +               rtc_pdev = pdev;
> >                 /* hold a reference so it doesn't go away */
> >                 get_device(dev);
> >                 pdev = NULL;
> > @@ -123,6 +125,23 @@ static int alarmtimer_rtc_add_device(struct device
> *dev)
> >         return ret;
> >  }
> >
> > +static void alarmtimer_rtc_remove_device(struct device *dev) {
> > +       struct rtc_device *rtc = to_rtc_device(dev);
> > +
> > +       if (rtc_pdev) {
> 
> As the return value of class_interface.add_dev() is never checked
> (alarmtimer_rtc_add_device() returns -EBUSY on adding a second alarmtimer),
> multiple timers may have been added, but only one of them will be the real
> alarmtimer.
> Hence this function should check if rtcdev == rtc before unregistering the
> real alarmtimer.  Of course all of this should be protected by rtcdev_lock.

Ok will add lock here and the check.
> 
> > +               module_put(rtc->owner);
> > +               if (device_may_wakeup(rtc->dev.parent))
> > +                       device_init_wakeup(&rtc_pdev->dev, false);
> > +
> > +               platform_device_unregister(rtc_pdev);
> > +               put_device(dev);
> 
> Perhaps use the reverse order of operations as in
> alarmtimer_rtc_add_device()?

Platform device is child of rtc device. So it has to be
at the last as already there is put_device() call in 
devm_rtc_release_device()

Cheers,
Biju


> 
> > +       }
> > +
> > +       rtcdev = NULL;
> > +       rtc_pdev = NULL;
> > +}
> > +
> >  static inline void alarmtimer_rtc_timer_init(void)  {
> >         rtc_timer_init(&rtctimer, NULL, NULL);
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-
> m68k.org
> 
> In personal conversations with technical people, I call myself a hacker.
> But when I'm talking to journalists I just say "programmer" or something
> like that.
>                                 -- Linus Torvalds