| Message ID | 5629262.DvuYhMxLoT@kreacher |
|---|---|
| State | New |
| Headers | show |
| Series | rtc: rtc-cmos: Fix event handler registration ordering issue | expand |
On Wed, Oct 12, 2022 at 08:07:01PM +0200, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Because acpi_install_fixed_event_handler() enables the event > automatically on success, it is incorrect to call it before the > handler routine passed to it is ready to handle events. > > Unfortunately, the rtc-cmos driver does exactly the incorrect thing > by calling cmos_wake_setup(), which passes rtc_handler() to > acpi_install_fixed_event_handler(), before cmos_do_probe(), because > rtc_handler() uses dev_get_drvdata() to get to the cmos object > pointer and the driver data pointer is only populated in > cmos_do_probe(). > > This leads to a NULL pointer dereference in rtc_handler() on boot > if the RTC fixed event happens to be active at the init time. > > To address this issue, change the initialization ordering of the > driver so that cmos_wake_setup() is always called after a successful > cmos_do_probe() call. > > While at it, change cmos_pnp_probe() to call cmos_do_probe() after > the initial if () statement used for computing the IRQ argument to > be passed to cmos_do_probe() which is cleaner than calling it in > each branch of that if () (local variable "irq" can be of type int, > because it is passed to that function as an argument of type int). > > Note that commit 6492fed7d8c9 ("rtc: rtc-cmos: Do not check > ACPI_FADT_LOW_POWER_S0") caused this issue to affect a larger number > of systems, because previously it only affected systems with > ACPI_FADT_LOW_POWER_S0 set, but it is present regardless of that > commit. > > Fixes: 6492fed7d8c9 ("rtc: rtc-cmos: Do not check ACPI_FADT_LOW_POWER_S0") > Fixes: a474aaedac99 ("rtc-cmos: move wake setup from ACPI glue into RTC driver") > Link: https://lore.kernel.org/linux-acpi/20221010141630.zfzi7mk7zvnmclzy@techsingularity.net/ > Reported-by: Mel Gorman <mgorman@techsingularity.net> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Works for me so; Tested-by: Mel Gorman <mgorman@techsingularity.net>
On Wed, Oct 12, 2022 at 08:07:01PM +0200, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Because acpi_install_fixed_event_handler() enables the event > automatically on success, it is incorrect to call it before the > handler routine passed to it is ready to handle events. > > Unfortunately, the rtc-cmos driver does exactly the incorrect thing > by calling cmos_wake_setup(), which passes rtc_handler() to > acpi_install_fixed_event_handler(), before cmos_do_probe(), because > rtc_handler() uses dev_get_drvdata() to get to the cmos object > pointer and the driver data pointer is only populated in > cmos_do_probe(). > > This leads to a NULL pointer dereference in rtc_handler() on boot > if the RTC fixed event happens to be active at the init time. > > To address this issue, change the initialization ordering of the > driver so that cmos_wake_setup() is always called after a successful > cmos_do_probe() call. > > While at it, change cmos_pnp_probe() to call cmos_do_probe() after > the initial if () statement used for computing the IRQ argument to > be passed to cmos_do_probe() which is cleaner than calling it in > each branch of that if () (local variable "irq" can be of type int, > because it is passed to that function as an argument of type int). > > Note that commit 6492fed7d8c9 ("rtc: rtc-cmos: Do not check > ACPI_FADT_LOW_POWER_S0") caused this issue to affect a larger number > of systems, because previously it only affected systems with > ACPI_FADT_LOW_POWER_S0 set, but it is present regardless of that > commit. > > Fixes: 6492fed7d8c9 ("rtc: rtc-cmos: Do not check ACPI_FADT_LOW_POWER_S0") > Fixes: a474aaedac99 ("rtc-cmos: move wake setup from ACPI glue into RTC driver") > Link: https://lore.kernel.org/linux-acpi/20221010141630.zfzi7mk7zvnmclzy@techsingularity.net/ > Reported-by: Mel Gorman <mgorman@techsingularity.net> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Reviewed-by: Bjorn Helgaas <bhelgaas@google.com> Yep, I blew it with a474aaedac99, sorry about that. Possibly could call cmos_wake_setup() from cmos_do_probe() instead of from cmos_pnp_probe() and cmos_platform_probe()? Then there would be a single call site and it would be closer to the actual dependency on dev_set_drvdata(). Either way is fine with me. Unrelated, but I happened to notice that pnp_irq() returns -1 for failure, and this note suggests that possibly returning 0 would be better: https://lore.kernel.org/r/CAHk-=wg2Pkb9kbfbstbB91AJA2SF6cySbsgHG-iQMq56j3VTcA@mail.gmail.com > --- > drivers/rtc/rtc-cmos.c | 29 +++++++++++++++++++---------- > 1 file changed, 19 insertions(+), 10 deletions(-) > > Index: linux-pm/drivers/rtc/rtc-cmos.c > =================================================================== > --- linux-pm.orig/drivers/rtc/rtc-cmos.c > +++ linux-pm/drivers/rtc/rtc-cmos.c > @@ -1352,10 +1352,10 @@ static void cmos_check_acpi_rtc_status(s > > static int cmos_pnp_probe(struct pnp_dev *pnp, const struct pnp_device_id *id) > { > - cmos_wake_setup(&pnp->dev); > + int irq, ret; > > if (pnp_port_start(pnp, 0) == 0x70 && !pnp_irq_valid(pnp, 0)) { > - unsigned int irq = 0; > + irq = 0; > #ifdef CONFIG_X86 > /* Some machines contain a PNP entry for the RTC, but > * don't define the IRQ. It should always be safe to > @@ -1364,13 +1364,17 @@ static int cmos_pnp_probe(struct pnp_dev > if (nr_legacy_irqs()) > irq = RTC_IRQ; > #endif > - return cmos_do_probe(&pnp->dev, > - pnp_get_resource(pnp, IORESOURCE_IO, 0), irq); > } else { > - return cmos_do_probe(&pnp->dev, > - pnp_get_resource(pnp, IORESOURCE_IO, 0), > - pnp_irq(pnp, 0)); > + irq = pnp_irq(pnp, 0); > } > + > + ret = cmos_do_probe(&pnp->dev, pnp_get_resource(pnp, IORESOURCE_IO, 0), irq); > + if (ret) > + return ret; > + > + cmos_wake_setup(&pnp->dev); > + > + return 0; > } > > static void cmos_pnp_remove(struct pnp_dev *pnp) > @@ -1454,10 +1458,9 @@ static inline void cmos_of_init(struct p > static int __init cmos_platform_probe(struct platform_device *pdev) > { > struct resource *resource; > - int irq; > + int irq, ret; > > cmos_of_init(pdev); > - cmos_wake_setup(&pdev->dev); > > if (RTC_IOMAPPED) > resource = platform_get_resource(pdev, IORESOURCE_IO, 0); > @@ -1467,7 +1470,13 @@ static int __init cmos_platform_probe(st > if (irq < 0) > irq = -1; > > - return cmos_do_probe(&pdev->dev, resource, irq); > + ret = cmos_do_probe(&pdev->dev, resource, irq); > + if (ret) > + return ret; > + > + cmos_wake_setup(&pdev->dev); > + > + return 0; > } > > static int cmos_platform_remove(struct platform_device *pdev) > > >
On Wed, Oct 12, 2022 at 11:00 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Wed, Oct 12, 2022 at 08:07:01PM +0200, Rafael J. Wysocki wrote: > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > Because acpi_install_fixed_event_handler() enables the event > > automatically on success, it is incorrect to call it before the > > handler routine passed to it is ready to handle events. > > > > Unfortunately, the rtc-cmos driver does exactly the incorrect thing > > by calling cmos_wake_setup(), which passes rtc_handler() to > > acpi_install_fixed_event_handler(), before cmos_do_probe(), because > > rtc_handler() uses dev_get_drvdata() to get to the cmos object > > pointer and the driver data pointer is only populated in > > cmos_do_probe(). > > > > This leads to a NULL pointer dereference in rtc_handler() on boot > > if the RTC fixed event happens to be active at the init time. > > > > To address this issue, change the initialization ordering of the > > driver so that cmos_wake_setup() is always called after a successful > > cmos_do_probe() call. > > > > While at it, change cmos_pnp_probe() to call cmos_do_probe() after > > the initial if () statement used for computing the IRQ argument to > > be passed to cmos_do_probe() which is cleaner than calling it in > > each branch of that if () (local variable "irq" can be of type int, > > because it is passed to that function as an argument of type int). > > > > Note that commit 6492fed7d8c9 ("rtc: rtc-cmos: Do not check > > ACPI_FADT_LOW_POWER_S0") caused this issue to affect a larger number > > of systems, because previously it only affected systems with > > ACPI_FADT_LOW_POWER_S0 set, but it is present regardless of that > > commit. > > > > Fixes: 6492fed7d8c9 ("rtc: rtc-cmos: Do not check ACPI_FADT_LOW_POWER_S0") > > Fixes: a474aaedac99 ("rtc-cmos: move wake setup from ACPI glue into RTC driver") > > Link: https://lore.kernel.org/linux-acpi/20221010141630.zfzi7mk7zvnmclzy@techsingularity.net/ > > Reported-by: Mel Gorman <mgorman@techsingularity.net> > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Reviewed-by: Bjorn Helgaas <bhelgaas@google.com> > > Yep, I blew it with a474aaedac99, sorry about that. > > Possibly could call cmos_wake_setup() from cmos_do_probe() instead of > from cmos_pnp_probe() and cmos_platform_probe()? Sounds good. I would prefer to send a separate patch for this on top of the $subject one, unless Alexandre wants me to do it all in one go. Alexandre, what's your preference here? Or would you prefer if I pushed this forward? > Then there would be a single call site and it would be closer to the actual dependency on > dev_set_drvdata(). Either way is fine with me. OK > Unrelated, but I happened to notice that pnp_irq() returns -1 for > failure, and this note suggests that possibly returning 0 would be > better: > > https://lore.kernel.org/r/CAHk-=wg2Pkb9kbfbstbB91AJA2SF6cySbsgHG-iQMq56j3VTcA@mail.gmail.com Probably. In that case, though, it would be prudent to also explicitly discard IRQ resources where start is equal to 0. > > > --- > > drivers/rtc/rtc-cmos.c | 29 +++++++++++++++++++---------- > > 1 file changed, 19 insertions(+), 10 deletions(-) > > > > Index: linux-pm/drivers/rtc/rtc-cmos.c > > =================================================================== > > --- linux-pm.orig/drivers/rtc/rtc-cmos.c > > +++ linux-pm/drivers/rtc/rtc-cmos.c > > @@ -1352,10 +1352,10 @@ static void cmos_check_acpi_rtc_status(s > > > > static int cmos_pnp_probe(struct pnp_dev *pnp, const struct pnp_device_id *id) > > { > > - cmos_wake_setup(&pnp->dev); > > + int irq, ret; > > > > if (pnp_port_start(pnp, 0) == 0x70 && !pnp_irq_valid(pnp, 0)) { > > - unsigned int irq = 0; > > + irq = 0; > > #ifdef CONFIG_X86 > > /* Some machines contain a PNP entry for the RTC, but > > * don't define the IRQ. It should always be safe to > > @@ -1364,13 +1364,17 @@ static int cmos_pnp_probe(struct pnp_dev > > if (nr_legacy_irqs()) > > irq = RTC_IRQ; > > #endif > > - return cmos_do_probe(&pnp->dev, > > - pnp_get_resource(pnp, IORESOURCE_IO, 0), irq); > > } else { > > - return cmos_do_probe(&pnp->dev, > > - pnp_get_resource(pnp, IORESOURCE_IO, 0), > > - pnp_irq(pnp, 0)); > > + irq = pnp_irq(pnp, 0); > > } > > + > > + ret = cmos_do_probe(&pnp->dev, pnp_get_resource(pnp, IORESOURCE_IO, 0), irq); > > + if (ret) > > + return ret; > > + > > + cmos_wake_setup(&pnp->dev); > > + > > + return 0; > > } > > > > static void cmos_pnp_remove(struct pnp_dev *pnp) > > @@ -1454,10 +1458,9 @@ static inline void cmos_of_init(struct p > > static int __init cmos_platform_probe(struct platform_device *pdev) > > { > > struct resource *resource; > > - int irq; > > + int irq, ret; > > > > cmos_of_init(pdev); > > - cmos_wake_setup(&pdev->dev); > > > > if (RTC_IOMAPPED) > > resource = platform_get_resource(pdev, IORESOURCE_IO, 0); > > @@ -1467,7 +1470,13 @@ static int __init cmos_platform_probe(st > > if (irq < 0) > > irq = -1; > > > > - return cmos_do_probe(&pdev->dev, resource, irq); > > + ret = cmos_do_probe(&pdev->dev, resource, irq); > > + if (ret) > > + return ret; > > + > > + cmos_wake_setup(&pdev->dev); > > + > > + return 0; > > } > > > > static int cmos_platform_remove(struct platform_device *pdev) > > > > > >
On Wed, 12 Oct 2022 20:07:01 +0200, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Because acpi_install_fixed_event_handler() enables the event > automatically on success, it is incorrect to call it before the > handler routine passed to it is ready to handle events. > > Unfortunately, the rtc-cmos driver does exactly the incorrect thing > by calling cmos_wake_setup(), which passes rtc_handler() to > acpi_install_fixed_event_handler(), before cmos_do_probe(), because > rtc_handler() uses dev_get_drvdata() to get to the cmos object > pointer and the driver data pointer is only populated in > cmos_do_probe(). > > [...] Applied, thanks! [1/1] rtc: rtc-cmos: Fix event handler registration ordering issue commit: 4919d3eb2ec0ee364f7e3cf2d99646c1b224fae8 Best regards,
On 13/10/2022 13:38:31+0200, Rafael J. Wysocki wrote: > On Wed, Oct 12, 2022 at 11:00 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > > > On Wed, Oct 12, 2022 at 08:07:01PM +0200, Rafael J. Wysocki wrote: > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > Because acpi_install_fixed_event_handler() enables the event > > > automatically on success, it is incorrect to call it before the > > > handler routine passed to it is ready to handle events. > > > > > > Unfortunately, the rtc-cmos driver does exactly the incorrect thing > > > by calling cmos_wake_setup(), which passes rtc_handler() to > > > acpi_install_fixed_event_handler(), before cmos_do_probe(), because > > > rtc_handler() uses dev_get_drvdata() to get to the cmos object > > > pointer and the driver data pointer is only populated in > > > cmos_do_probe(). > > > > > > This leads to a NULL pointer dereference in rtc_handler() on boot > > > if the RTC fixed event happens to be active at the init time. > > > > > > To address this issue, change the initialization ordering of the > > > driver so that cmos_wake_setup() is always called after a successful > > > cmos_do_probe() call. > > > > > > While at it, change cmos_pnp_probe() to call cmos_do_probe() after > > > the initial if () statement used for computing the IRQ argument to > > > be passed to cmos_do_probe() which is cleaner than calling it in > > > each branch of that if () (local variable "irq" can be of type int, > > > because it is passed to that function as an argument of type int). > > > > > > Note that commit 6492fed7d8c9 ("rtc: rtc-cmos: Do not check > > > ACPI_FADT_LOW_POWER_S0") caused this issue to affect a larger number > > > of systems, because previously it only affected systems with > > > ACPI_FADT_LOW_POWER_S0 set, but it is present regardless of that > > > commit. > > > > > > Fixes: 6492fed7d8c9 ("rtc: rtc-cmos: Do not check ACPI_FADT_LOW_POWER_S0") > > > Fixes: a474aaedac99 ("rtc-cmos: move wake setup from ACPI glue into RTC driver") > > > Link: https://lore.kernel.org/linux-acpi/20221010141630.zfzi7mk7zvnmclzy@techsingularity.net/ > > > Reported-by: Mel Gorman <mgorman@techsingularity.net> > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > Reviewed-by: Bjorn Helgaas <bhelgaas@google.com> > > > > Yep, I blew it with a474aaedac99, sorry about that. > > > > Possibly could call cmos_wake_setup() from cmos_do_probe() instead of > > from cmos_pnp_probe() and cmos_platform_probe()? > > Sounds good. > > I would prefer to send a separate patch for this on top of the > $subject one, unless Alexandre wants me to do it all in one go. > > Alexandre, what's your preference here? Or would you prefer if I > pushed this forward? > I applied your patch, feel free to improve on top of that ;) > > Then there would be a single call site and it would be closer to the actual dependency on > > dev_set_drvdata(). Either way is fine with me. > > OK > > > Unrelated, but I happened to notice that pnp_irq() returns -1 for > > failure, and this note suggests that possibly returning 0 would be > > better: > > > > https://lore.kernel.org/r/CAHk-=wg2Pkb9kbfbstbB91AJA2SF6cySbsgHG-iQMq56j3VTcA@mail.gmail.com > > Probably. > > In that case, though, it would be prudent to also explicitly discard > IRQ resources where start is equal to 0. > > > > > > --- > > > drivers/rtc/rtc-cmos.c | 29 +++++++++++++++++++---------- > > > 1 file changed, 19 insertions(+), 10 deletions(-) > > > > > > Index: linux-pm/drivers/rtc/rtc-cmos.c > > > =================================================================== > > > --- linux-pm.orig/drivers/rtc/rtc-cmos.c > > > +++ linux-pm/drivers/rtc/rtc-cmos.c > > > @@ -1352,10 +1352,10 @@ static void cmos_check_acpi_rtc_status(s > > > > > > static int cmos_pnp_probe(struct pnp_dev *pnp, const struct pnp_device_id *id) > > > { > > > - cmos_wake_setup(&pnp->dev); > > > + int irq, ret; > > > > > > if (pnp_port_start(pnp, 0) == 0x70 && !pnp_irq_valid(pnp, 0)) { > > > - unsigned int irq = 0; > > > + irq = 0; > > > #ifdef CONFIG_X86 > > > /* Some machines contain a PNP entry for the RTC, but > > > * don't define the IRQ. It should always be safe to > > > @@ -1364,13 +1364,17 @@ static int cmos_pnp_probe(struct pnp_dev > > > if (nr_legacy_irqs()) > > > irq = RTC_IRQ; > > > #endif > > > - return cmos_do_probe(&pnp->dev, > > > - pnp_get_resource(pnp, IORESOURCE_IO, 0), irq); > > > } else { > > > - return cmos_do_probe(&pnp->dev, > > > - pnp_get_resource(pnp, IORESOURCE_IO, 0), > > > - pnp_irq(pnp, 0)); > > > + irq = pnp_irq(pnp, 0); > > > } > > > + > > > + ret = cmos_do_probe(&pnp->dev, pnp_get_resource(pnp, IORESOURCE_IO, 0), irq); > > > + if (ret) > > > + return ret; > > > + > > > + cmos_wake_setup(&pnp->dev); > > > + > > > + return 0; > > > } > > > > > > static void cmos_pnp_remove(struct pnp_dev *pnp) > > > @@ -1454,10 +1458,9 @@ static inline void cmos_of_init(struct p > > > static int __init cmos_platform_probe(struct platform_device *pdev) > > > { > > > struct resource *resource; > > > - int irq; > > > + int irq, ret; > > > > > > cmos_of_init(pdev); > > > - cmos_wake_setup(&pdev->dev); > > > > > > if (RTC_IOMAPPED) > > > resource = platform_get_resource(pdev, IORESOURCE_IO, 0); > > > @@ -1467,7 +1470,13 @@ static int __init cmos_platform_probe(st > > > if (irq < 0) > > > irq = -1; > > > > > > - return cmos_do_probe(&pdev->dev, resource, irq); > > > + ret = cmos_do_probe(&pdev->dev, resource, irq); > > > + if (ret) > > > + return ret; > > > + > > > + cmos_wake_setup(&pdev->dev); > > > + > > > + return 0; > > > } > > > > > > static int cmos_platform_remove(struct platform_device *pdev) > > > > > > > > >
Index: linux-pm/drivers/rtc/rtc-cmos.c =================================================================== --- linux-pm.orig/drivers/rtc/rtc-cmos.c +++ linux-pm/drivers/rtc/rtc-cmos.c @@ -1352,10 +1352,10 @@ static void cmos_check_acpi_rtc_status(s static int cmos_pnp_probe(struct pnp_dev *pnp, const struct pnp_device_id *id) { - cmos_wake_setup(&pnp->dev); + int irq, ret; if (pnp_port_start(pnp, 0) == 0x70 && !pnp_irq_valid(pnp, 0)) { - unsigned int irq = 0; + irq = 0; #ifdef CONFIG_X86 /* Some machines contain a PNP entry for the RTC, but * don't define the IRQ. It should always be safe to @@ -1364,13 +1364,17 @@ static int cmos_pnp_probe(struct pnp_dev if (nr_legacy_irqs()) irq = RTC_IRQ; #endif - return cmos_do_probe(&pnp->dev, - pnp_get_resource(pnp, IORESOURCE_IO, 0), irq); } else { - return cmos_do_probe(&pnp->dev, - pnp_get_resource(pnp, IORESOURCE_IO, 0), - pnp_irq(pnp, 0)); + irq = pnp_irq(pnp, 0); } + + ret = cmos_do_probe(&pnp->dev, pnp_get_resource(pnp, IORESOURCE_IO, 0), irq); + if (ret) + return ret; + + cmos_wake_setup(&pnp->dev); + + return 0; } static void cmos_pnp_remove(struct pnp_dev *pnp) @@ -1454,10 +1458,9 @@ static inline void cmos_of_init(struct p static int __init cmos_platform_probe(struct platform_device *pdev) { struct resource *resource; - int irq; + int irq, ret; cmos_of_init(pdev); - cmos_wake_setup(&pdev->dev); if (RTC_IOMAPPED) resource = platform_get_resource(pdev, IORESOURCE_IO, 0); @@ -1467,7 +1470,13 @@ static int __init cmos_platform_probe(st if (irq < 0) irq = -1; - return cmos_do_probe(&pdev->dev, resource, irq); + ret = cmos_do_probe(&pdev->dev, resource, irq); + if (ret) + return ret; + + cmos_wake_setup(&pdev->dev); + + return 0; } static int cmos_platform_remove(struct platform_device *pdev)