Message ID | 2219830.iZASKD2KPV@kreacher (mailing list archive) |
---|---|
State | Superseded, archived |
Headers | show |
Series | rtc: rtc-cmos: Assorted ACPI-related cleanups and fixes | expand |
On Mon, Nov 07, 2022 at 09:03:06PM +0100, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Make cmos_do_remove() drop the ACPI RTC fixed event handler so as to > prevent it from operating on stale data in case the event triggers > after driver removal. > > While at it, make cmos_do_remove() also clear the driver data pointer > of the device and make cmos_acpi_wake_setup() do that in the error path > too. ... > + dev_set_drvdata(dev, NULL); > + dev_set_drvdata(dev, NULL); Maybe I'm missing something, but the cmos_do_remove() is called by ->remove() callback of the real drivers (pnp and platform) and device core is already doing this. So, don't know why you need these calls to be explicit.
On Mon, Nov 07, 2022 at 09:03:06PM +0100, Rafael J. Wysocki wrote: ... > +static inline void acpi_rtc_event_cleanup(void) > +{ > + if (!acpi_disabled) Btw, other functions look like using if (acpi_disabled) return; pattern. Maybe here the same for the sake of consistency? > + acpi_remove_fixed_event_handler(ACPI_EVENT_RTC, rtc_handler); > +}
On Mon, Nov 7, 2022 at 10:21 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Mon, Nov 07, 2022 at 09:03:06PM +0100, Rafael J. Wysocki wrote: > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > Make cmos_do_remove() drop the ACPI RTC fixed event handler so as to > > prevent it from operating on stale data in case the event triggers > > after driver removal. > > > > While at it, make cmos_do_remove() also clear the driver data pointer > > of the device and make cmos_acpi_wake_setup() do that in the error path > > too. > > ... > > > + dev_set_drvdata(dev, NULL); > > > + dev_set_drvdata(dev, NULL); > > Maybe I'm missing something, but the cmos_do_remove() is called by ->remove() > callback of the real drivers (pnp and platform) and device core is already > doing this. So, don't know why you need these calls to be explicit. Good point, but then I guess I should move this patch to the front, because the issue fixed by it may trigger a use-after-free in rtc_handler() already.
On Mon, Nov 7, 2022 at 10:28 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Mon, Nov 07, 2022 at 09:03:06PM +0100, Rafael J. Wysocki wrote: > > ... > > > +static inline void acpi_rtc_event_cleanup(void) > > +{ > > + if (!acpi_disabled) > > Btw, other functions look like using > > if (acpi_disabled) > return; > > pattern. Maybe here the same for the sake of consistency? > > > + acpi_remove_fixed_event_handler(ACPI_EVENT_RTC, rtc_handler); > > +} > Well, it is more lines of code, but whatever.
Index: linux-pm/drivers/rtc/rtc-cmos.c =================================================================== --- linux-pm.orig/drivers/rtc/rtc-cmos.c +++ linux-pm/drivers/rtc/rtc-cmos.c @@ -798,6 +798,12 @@ static inline void acpi_rtc_event_setup( acpi_disable_event(ACPI_EVENT_RTC, 0); } +static inline void acpi_rtc_event_cleanup(void) +{ + if (!acpi_disabled) + acpi_remove_fixed_event_handler(ACPI_EVENT_RTC, rtc_handler); +} + static void rtc_wake_on(struct device *dev) { acpi_clear_event(ACPI_EVENT_RTC); @@ -884,6 +890,10 @@ static inline void acpi_rtc_event_setup( { } +static inline void acpi_rtc_event_cleanup(void) +{ +} + static inline void cmos_acpi_wake_setup(struct device *dev) { } @@ -1109,6 +1119,7 @@ cleanup2: free_irq(rtc_irq, cmos_rtc.rtc); cleanup1: cmos_rtc.dev = NULL; + dev_set_drvdata(dev, NULL); cleanup0: if (RTC_IOMAPPED) release_region(ports->start, resource_size(ports)); @@ -1138,6 +1149,9 @@ static void cmos_do_remove(struct device hpet_unregister_irq_handler(cmos_interrupt); } + if (!dev_get_platdata(dev)) + acpi_rtc_event_cleanup(); + cmos->rtc = NULL; ports = cmos->iomem; @@ -1148,6 +1162,7 @@ static void cmos_do_remove(struct device cmos->iomem = NULL; cmos->dev = NULL; + dev_set_drvdata(dev, NULL); } static int cmos_aie_poweroff(struct device *dev)