[PATCH] [media] serial_ir: ensure we're ready to receive interrupts

From: Sean Young
Date: Sat Feb 25 2017 - 06:29:24 EST

When the interrupt requested with devm_request_irq(), serial_ir.rcdev
is still null so will cause null deference if the irq handler is called
early on.

Also ensure that timeout_timer is setup.

Link: http://lkml.kernel.org/r/CA+55aFxsh2uF8gi5sN_guY3Z+tiLv7LpJYKBw+y8vqLzp+TsnQ@xxxxxxxxxxxxxx

Cc: <stable@xxxxxxxxxxxxxxx> # 4.10
Signed-off-by: Sean Young <sean@xxxxxxxx>
---
drivers/media/rc/serial_ir.c | 243 +++++++++++++++++++++----------------------
1 file changed, 118 insertions(+), 125 deletions(-)

diff --git a/drivers/media/rc/serial_ir.c b/drivers/media/rc/serial_ir.c
index 923fb22..22144b4 100644
--- a/drivers/media/rc/serial_ir.c
+++ b/drivers/media/rc/serial_ir.c
@@ -487,74 +487,6 @@ static void serial_ir_timeout(unsigned long arg)
ir_raw_event_handle(serial_ir.rcdev);
}

-static int serial_ir_probe(struct platform_device *dev)
-{
- int i, nlow, nhigh, result;
-
- result = devm_request_irq(&dev->dev, irq, serial_ir_irq_handler,
- share_irq ? IRQF_SHARED : 0,
- KBUILD_MODNAME, &hardware);
- if (result < 0) {
- if (result == -EBUSY)
- dev_err(&dev->dev, "IRQ %d busy\n", irq);
- else if (result == -EINVAL)
- dev_err(&dev->dev, "Bad irq number or handler\n");
- return result;
- }
-
- /* Reserve io region. */
- if ((iommap &&
- (devm_request_mem_region(&dev->dev, iommap, 8 << ioshift,
- KBUILD_MODNAME) == NULL)) ||
- (!iommap && (devm_request_region(&dev->dev, io, 8,
- KBUILD_MODNAME) == NULL))) {
- dev_err(&dev->dev, "port %04x already in use\n", io);
- dev_warn(&dev->dev, "use 'setserial /dev/ttySX uart none'\n");
- dev_warn(&dev->dev,
- "or compile the serial port driver as module and\n");
- dev_warn(&dev->dev, "make sure this module is loaded first\n");
- return -EBUSY;
- }
-
- setup_timer(&serial_ir.timeout_timer, serial_ir_timeout,
- (unsigned long)&serial_ir);
-
- result = hardware_init_port();
- if (result < 0)
- return result;
-
- /* Initialize pulse/space widths */
- init_timing_params(50, 38000);
-
- /* If pin is high, then this must be an active low receiver. */
- if (sense == -1) {
- /* wait 1/2 sec for the power supply */
- msleep(500);
-
- /*
- * probe 9 times every 0.04s, collect "votes" for
- * active high/low
- */
- nlow = 0;
- nhigh = 0;
- for (i = 0; i < 9; i++) {
- if (sinp(UART_MSR) & hardware[type].signal_pin)
- nlow++;
- else
- nhigh++;
- msleep(40);
- }
- sense = nlow >= nhigh ? 1 : 0;
- dev_info(&dev->dev, "auto-detected active %s receiver\n",
- sense ? "low" : "high");
- } else
- dev_info(&dev->dev, "Manually using active %s receiver\n",
- sense ? "low" : "high");
-
- dev_dbg(&dev->dev, "Interrupt %d, port %04x obtained\n", irq, io);
- return 0;
-}
-
static int serial_ir_open(struct rc_dev *rcdev)
{
unsigned long flags;
@@ -679,6 +611,123 @@ static int serial_ir_resume(struct platform_device *dev)
return 0;
}

+static int serial_ir_probe(struct platform_device *dev)
+{
+ struct rc_dev *rcdev;
+ int i, nlow, nhigh, result;
+
+ rcdev = devm_rc_allocate_device(&dev->dev, RC_DRIVER_IR_RAW);
+ if (!rcdev)
+ return -ENOMEM;
+
+ if (hardware[type].send_pulse && hardware[type].send_space)
+ rcdev->tx_ir = serial_ir_tx;
+ if (hardware[type].set_send_carrier)
+ rcdev->s_tx_carrier = serial_ir_tx_carrier;
+ if (hardware[type].set_duty_cycle)
+ rcdev->s_tx_duty_cycle = serial_ir_tx_duty_cycle;
+
+ switch (type) {
+ case IR_HOMEBREW:
+ rcdev->input_name = "Serial IR type home-brew";
+ break;
+ case IR_IRDEO:
+ rcdev->input_name = "Serial IR type IRdeo";
+ break;
+ case IR_IRDEO_REMOTE:
+ rcdev->input_name = "Serial IR type IRdeo remote";
+ break;
+ case IR_ANIMAX:
+ rcdev->input_name = "Serial IR type AnimaX";
+ break;
+ case IR_IGOR:
+ rcdev->input_name = "Serial IR type IgorPlug";
+ break;
+ }
+
+ rcdev->input_phys = KBUILD_MODNAME "/input0";
+ rcdev->input_id.bustype = BUS_HOST;
+ rcdev->input_id.vendor = 0x0001;
+ rcdev->input_id.product = 0x0001;
+ rcdev->input_id.version = 0x0100;
+ rcdev->open = serial_ir_open;
+ rcdev->close = serial_ir_close;
+ rcdev->dev.parent = &serial_ir.pdev->dev;
+ rcdev->allowed_protocols = RC_BIT_ALL_IR_DECODER;
+ rcdev->driver_name = KBUILD_MODNAME;
+ rcdev->map_name = RC_MAP_RC6_MCE;
+ rcdev->min_timeout = 1;
+ rcdev->timeout = IR_DEFAULT_TIMEOUT;
+ rcdev->max_timeout = 10 * IR_DEFAULT_TIMEOUT;
+ rcdev->rx_resolution = 250000;
+
+ serial_ir.rcdev = rcdev;
+
+ setup_timer(&serial_ir.timeout_timer, serial_ir_timeout,
+ (unsigned long)&serial_ir);
+
+ result = devm_request_irq(&dev->dev, irq, serial_ir_irq_handler,
+ share_irq ? IRQF_SHARED : 0,
+ KBUILD_MODNAME, &hardware);
+ if (result < 0) {
+ if (result == -EBUSY)
+ dev_err(&dev->dev, "IRQ %d busy\n", irq);
+ else if (result == -EINVAL)
+ dev_err(&dev->dev, "Bad irq number or handler\n");
+ return result;
+ }
+
+ /* Reserve io region. */
+ if ((iommap &&
+ (devm_request_mem_region(&dev->dev, iommap, 8 << ioshift,
+ KBUILD_MODNAME) == NULL)) ||
+ (!iommap && (devm_request_region(&dev->dev, io, 8,
+ KBUILD_MODNAME) == NULL))) {
+ dev_err(&dev->dev, "port %04x already in use\n", io);
+ dev_warn(&dev->dev, "use 'setserial /dev/ttySX uart none'\n");
+ dev_warn(&dev->dev,
+ "or compile the serial port driver as module and\n");
+ dev_warn(&dev->dev, "make sure this module is loaded first\n");
+ return -EBUSY;
+ }
+
+ result = hardware_init_port();
+ if (result < 0)
+ return result;
+
+ /* Initialize pulse/space widths */
+ init_timing_params(50, 38000);
+
+ /* If pin is high, then this must be an active low receiver. */
+ if (sense == -1) {
+ /* wait 1/2 sec for the power supply */
+ msleep(500);
+
+ /*
+ * probe 9 times every 0.04s, collect "votes" for
+ * active high/low
+ */
+ nlow = 0;
+ nhigh = 0;
+ for (i = 0; i < 9; i++) {
+ if (sinp(UART_MSR) & hardware[type].signal_pin)
+ nlow++;
+ else
+ nhigh++;
+ msleep(40);
+ }
+ sense = nlow >= nhigh ? 1 : 0;
+ dev_info(&dev->dev, "auto-detected active %s receiver\n",
+ sense ? "low" : "high");
+ } else
+ dev_info(&dev->dev, "Manually using active %s receiver\n",
+ sense ? "low" : "high");
+
+ dev_dbg(&dev->dev, "Interrupt %d, port %04x obtained\n", irq, io);
+
+ return devm_rc_register_device(&dev->dev, rcdev);
+}
+
static struct platform_driver serial_ir_driver = {
.probe = serial_ir_probe,
.suspend = serial_ir_suspend,
@@ -723,7 +772,6 @@ static void serial_ir_exit(void)

static int __init serial_ir_init_module(void)
{
- struct rc_dev *rcdev;
int result;

switch (type) {
@@ -754,63 +802,9 @@ static int __init serial_ir_init_module(void)
sense = !!sense;

result = serial_ir_init();
- if (result)
- return result;
-
- rcdev = devm_rc_allocate_device(&serial_ir.pdev->dev, RC_DRIVER_IR_RAW);
- if (!rcdev) {
- result = -ENOMEM;
- goto serial_cleanup;
- }
-
- if (hardware[type].send_pulse && hardware[type].send_space)
- rcdev->tx_ir = serial_ir_tx;
- if (hardware[type].set_send_carrier)
- rcdev->s_tx_carrier = serial_ir_tx_carrier;
- if (hardware[type].set_duty_cycle)
- rcdev->s_tx_duty_cycle = serial_ir_tx_duty_cycle;
-
- switch (type) {
- case IR_HOMEBREW:
- rcdev->input_name = "Serial IR type home-brew";
- break;
- case IR_IRDEO:
- rcdev->input_name = "Serial IR type IRdeo";
- break;
- case IR_IRDEO_REMOTE:
- rcdev->input_name = "Serial IR type IRdeo remote";
- break;
- case IR_ANIMAX:
- rcdev->input_name = "Serial IR type AnimaX";
- break;
- case IR_IGOR:
- rcdev->input_name = "Serial IR type IgorPlug";
- break;
- }
-
- rcdev->input_phys = KBUILD_MODNAME "/input0";
- rcdev->input_id.bustype = BUS_HOST;
- rcdev->input_id.vendor = 0x0001;
- rcdev->input_id.product = 0x0001;
- rcdev->input_id.version = 0x0100;
- rcdev->open = serial_ir_open;
- rcdev->close = serial_ir_close;
- rcdev->dev.parent = &serial_ir.pdev->dev;
- rcdev->allowed_protocols = RC_BIT_ALL_IR_DECODER;
- rcdev->driver_name = KBUILD_MODNAME;
- rcdev->map_name = RC_MAP_RC6_MCE;
- rcdev->min_timeout = 1;
- rcdev->timeout = IR_DEFAULT_TIMEOUT;
- rcdev->max_timeout = 10 * IR_DEFAULT_TIMEOUT;
- rcdev->rx_resolution = 250000;
-
- serial_ir.rcdev = rcdev;
-
- result = rc_register_device(rcdev);
-
if (!result)
return 0;
-serial_cleanup:
+
serial_ir_exit();
return result;
}
@@ -818,7 +812,6 @@ static int __init serial_ir_init_module(void)
static void __exit serial_ir_exit_module(void)
{
del_timer_sync(&serial_ir.timeout_timer);
- rc_unregister_device(serial_ir.rcdev);
serial_ir_exit();
}

--
2.9.3

On Sat, Feb 25, 2017 at 11:14:25AM +0000, Sean Young wrote:
> On Fri, Feb 24, 2017 at 11:15:51AM -0800, Linus Torvalds wrote:
> > Added more relevant people. I've debugged the immediate problem below,
> > but I think there's another problem that actually triggered this.
> >
> > On Fri, Feb 24, 2017 at 10:28 AM, kernel test robot
> > <fengguang.wu@xxxxxxxxx> wrote:
> > >
> > > 0day kernel testing robot got the below dmesg and the first bad commit is
> > >
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> > >
> > > commit ff58d005cd10fcd372787cceac547e11cf706ff6
> > > Merge: 5ab3566 9eeb0ed
> > >
> > > Merge tag 'media/v4.11-1' of git://git.kernel.org/pub/scm/linux/kernel/git/mchehab/linux-media
> > [...]
> > > [ 4.664940] rc rc0: lirc_dev: driver ir-lirc-codec (rc-loopback) registered at minor = 0
> > > [ 4.666322] BUG: unable to handle kernel NULL pointer dereference at 0000039c
> > > [ 4.666675] IP: serial_ir_irq_handler+0x189/0x410
> >
> > This merge being fingered ends up being a subtle interaction with other changes.
> >
> > Those "other changes" are (again) the interrupt retrigger code that
> > was reverted for 4.10, and then we tried to merge them again this
> > merge window.
> >
> > Because the immediate cause is:
> >
> > > [ 4.666675] EIP: serial_ir_irq_handler+0x189/0x410
> > > [ 4.666675] Call Trace:
> > > [ 4.666675] <IRQ>
> > > [ 4.666675] __handle_irq_event_percpu+0x57/0x100
> > > [ 4.666675] handle_irq_event_percpu+0x1d/0x50
> > > [ 4.666675] handle_irq_event+0x32/0x60
> > > [ 4.666675] handle_edge_irq+0xa5/0x120
> > > [ 4.666675] handle_irq+0x9d/0xd0
> > > [ 4.666675] </IRQ>
> > > [ 4.666675] do_IRQ+0x5f/0x130
> > > [ 4.666675] common_interrupt+0x33/0x38
> > > [ 4.666675] EIP: hardware_init_port+0x3f/0x190
> > > [ 4.666675] EFLAGS: 00200246 CPU: 0
> > > [ 4.666675] EAX: c718990f EBX: 00000000 ECX: 00000000 EDX: 000003f9
> > > [ 4.666675] ESI: 000003f9 EDI: 000003f8 EBP: c0065d98 ESP: c0065d84
> > > [ 4.666675] DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068
> > > [ 4.666675] serial_ir_probe+0xbb/0x300
> > > [ 4.666675] platform_drv_probe+0x48/0xb0
> > ...
> >
> > ie an interrupt came in immediately after the request_irq(), before
> > all the data was properly set up, which then causes the interrupt
> > handler to take a fault because it tries to access some field that
> > hasn't even been set up yet.
>
> Oh dear. I've pointed out others making the same mistake when doing code
> reviews, clearly I need review my own code better.
>
> >
> > The code line is helpful, the faulting instruction is
> >
> > mov 0x39c(%rax),%eax <--- fault
> > call ..
> > mov someglobalvar,%edx
> >
> > which together with the supplied config file makes me able to match it
> > up with the assembly generation around it:
> >
> > inb %dx, %al # tmp254, value
> > andb $1, %al #, tmp255
> > testb %al, %al # tmp255
> > je .L233 #,
> > .L215:
> > movl serial_ir+8, %eax # serial_ir.rcdev, serial_ir.rcdev
> > xorl %edx, %edx # _66->timeout
> > movl 924(%eax), %eax # _66->timeout, _66->timeout
> > call nsecs_to_jiffies #
> > movl jiffies, %edx # jiffies, jiffies.33_70
> > addl %eax, %edx # _69, tmp259
> > movl $serial_ir+16, %eax #,
> > call mod_timer #
> > movl serial_ir+8, %eax # serial_ir.rcdev,
> > call ir_raw_event_handle #
> > movl $1, %eax #, <retval>
> >
> > so it's that "serial_ir.rcdev->timeout" access that faults. So this is
> > the faulting source code:
> >
> > drivers/media/rc/serial_ir.c: 402
> >
> > mod_timer(&serial_ir.timeout_timer,
> > jiffies + nsecs_to_jiffies(serial_ir.rcdev->timeout));
> >
> > ir_raw_event_handle(serial_ir.rcdev);
> >
> > return IRQ_HANDLED;
> >
> > and serial_ir.rcdev is NULL when ti tries to look up the timeout.
>
> ir_raw_event_handle() call will also go bang if passed a null pointer, so
> this problem existed before (since v4.10).
>
> Thanks for debugging this, I'll send a patch as a reply to this email.
>
>
> Sean