Kernel oops when executing function to read hardware registers

Question

I'm referencing this answer for crash help in analyzing this bit of code which caused problems. The context for everyone, I'm working a character driver, which will act as a pass through from user space directly to the hardware, for the ahci driver. I'm modifying the ahci driver accordingly for this purpose.

I'm starting small. I want to peek at the port registers for the HBA port 0 of the AHCI HBA on my VM. My character driver ioctl code:

switch (cmd) {
    case AHCIP_GPORT_REG:
        pPciDev = pci_get_device(0x8086, 0x2829, NULL);

        if (pPciDev) {
            /* This will set ret to the value that it needs to be.  This
             * is true of __put_user() too */
            if ((ret = __get_user(off, (u32*)obj))) {
                printk(KERN_INFO "unable to read from user space\n");
                goto ioctl_quick_out;
            }

            reg = get_port_reg(&pPciDev->dev, off);
            if ((ret = __put_user(reg, (u32*)obj)))
            {
                printk(KERN_INFO "Unable to write to user space\n");
            }

            pci_dev_put(pPciDev);
        }

        // This break wasn't in the code when it crashed
        break;

    default:
        // POSIX compliance with this one (REF of LDD3)
        ret = -ENOTTY;
}

The code from my modified version of ahci.c which this character driver calls into:

u32 get_port_reg(struct device *dev, u32 off)
{
    struct Scsi_Host *shost = class_to_shost(dev);
    struct ata_port *ap = ata_shost_to_port(shost);
    void __iomem *port_mmio = ahci_port_base(ap);

    return ioread32(port_mmio + off);
}
EXPORT_SYMBOL(get_port_reg);

The kernel oops that this caused, happened here:

PID: 3357   TASK: ffff88011c9b7500  CPU: 0   COMMAND: "peek"
 #0 [ffff8800abfc79f0] machine_kexec at ffffffff8103b5bb
 #1 [ffff8800abfc7a50] crash_kexec at ffffffff810c9852
 #2 [ffff8800abfc7b20] oops_end at ffffffff8152e0f0
 #3 [ffff8800abfc7b50] no_context at ffffffff8104c80b
 #4 [ffff8800abfc7ba0] __bad_area_nosemaphore at ffffffff8104ca95
 #5 [ffff8800abfc7bf0] bad_area at ffffffff8104cbbe
 #6 [ffff8800abfc7c20] __do_page_fault at ffffffff8104d36f
 #7 [ffff8800abfc7d40] do_page_fault at ffffffff8153003e
 #8 [ffff8800abfc7d70] page_fault at ffffffff8152d3f5
    [exception RIP: get_port_reg+18]
    RIP: ffffffffa03c4cd2  RSP: ffff8800abfc7e28  RFLAGS: 00010246
    RAX: 0000000000020101  RBX: 00007fff17273960  RCX: ffffffff812b0710
    RDX: ffff88011ddd5000  RSI: 0000000000000000  RDI: ffff88011ddd5090
    RBP: ffff8800abfc7e28   R8: 0000000000000000   R9: 0000000000000000
    R10: 00000000000007d5  R11: 0000000000000006  R12: ffff88011ddd5000
    R13: 0000000000000000  R14: 0000000000000000  R15: 0000000000000000
    ORIG_RAX: ffffffffffffffff  CS: 0010  SS: 0018

As you can see, the instruction pointer was get_port_reg+18. Since this function is quite small, here's the full disassembly

crash> dis get_port_reg
0xffffffffa03c4cc0 <get_port_reg>:      push   %rbp
0xffffffffa03c4cc1 <get_port_reg+1>:    mov    %rsp,%rbp
0xffffffffa03c4cc4 <get_port_reg+4>:    nopl   0x0(%rax,%rax,1)
0xffffffffa03c4cc9 <get_port_reg+9>:    mov    0x240(%rdi),%rax
0xffffffffa03c4cd0 <get_port_reg+16>:   mov    %esi,%esi
0xffffffffa03c4cd2 <get_port_reg+18>:   mov    0x2838(%rax),%rdx
0xffffffffa03c4cd9 <get_port_reg+25>:   mov    0x28(%rax),%eax
0xffffffffa03c4cdc <get_port_reg+28>:   mov    0x10(%rdx),%rdx
0xffffffffa03c4ce0 <get_port_reg+32>:   shl    $0x7,%eax
0xffffffffa03c4ce3 <get_port_reg+35>:   mov    %eax,%eax
0xffffffffa03c4ce5 <get_port_reg+37>:   add    0x28(%rdx),%rax
0xffffffffa03c4ce9 <get_port_reg+41>:   lea    0x100(%rax,%rsi,1),%rdi
0xffffffffa03c4cf1 <get_port_reg+49>:   callq  0xffffffff8129dde0 <ioread32>
0xffffffffa03c4cf6 <get_port_reg+54>:   leaveq 
0xffffffffa03c4cf7 <get_port_reg+55>:   retq   
0xffffffffa03c4cf8 <get_port_reg+56>:   nopl   0x0(%rax,%rax,1)

As you might have guessed, I'm something of an assembly neophyte. Which line of code would be get_port_reg+18? I'm puzzled because I'm calling functions on each line of that function but the only call I see is to ioread32().

For reference, I've modeled my function get_port_reg after ahci_show_port_cmd() within the same file. I could not think of any other means of getting the struct pci_dev structure necessary on which this is to operate. Am I making bad use of get_pci_device() and pci_dev_put()? Is this not the issue at all?

Thanks for any help
Andy

Answer 1

I am going to post my own answer. The two commentators of my question have put me onto the correct path for fixing this. As I mentioned, my approach was to do something which I'd seen done elsewhere in the ahci driver (ahci.c). Basically, the assumption was simple, this function in ahci.c required a struct device* and from that was able to get the ata_port information that was required. I'd seen, in ahci.c, that the author had done struct device* = &pdev->dev; occasionally. In other words, I figured that the dev member of struct pci_dev was getting me what I needed. I was apparently unaware of "class types" or something similar (see @myaut's first comment). @alexhoppus essentially draws the same/similar conclusion based on the code and disassembly which I posted.

The fix which I have employed, and which does work nicely, is as follows:

/* ioctl code in character driver */
switch (cmd) {
    case AHCIP_GPORT_REG:
        pPciDev = pci_get_device(0x8086, 0x2829, NULL);

        if (pPciDev) {
            struct ata_host *pHost = NULL;
            struct ata_port *pPort = NULL;
            printk(KERN_INFO "found the PCI device\n");
            /* Get the devices driver data */
            pHost = pci_get_drvdata(pPciDev);
            if (!pHost) {
                ret = -EFAULT;
                goto ioctl_valid_pci_dev_out;
            }

            /* for this test, we'll use just port 0 */
            pPort = pHost->ports[0];
            if (!pPort) {
                ret = -EFAULT;
                goto ioctl_valid_pci_dev_out;
            }

            /* This will set ret to the value that it needs to be.  This
             * is true of __put_user() too */
            if ((ret = __get_user(off, (u32*)obj))) {
                printk(KERN_INFO "unable to read from user space\n");
                goto ioctl_valid_pci_dev_out;
            }

            reg = get_port_reg(pPort, off);
            if ((ret = __put_user(reg, (u32*)obj)))
            {
                printk(KERN_INFO "Unable to write to user space\n");
            }
        }

        break;

    default:
        // POSIX compliance with this one (REF of LDD3)
        ret = -ENOTTY;
}

The ahci driver was modified thusly as well

u32 get_port_reg(struct ata_port* pPort, u32 off)
{
    void __iomem *port_mmio = ahci_port_base(pPort);

    return ioread32(port_mmio + off);
}
EXPORT_SYMBOL(get_port_reg);

Though this has fixed the issue for me, I would really appreciate someone explaining to me what is placed in (struct pci_dev)device.dev.p->driver_data. I can use, and have, the Linux cross referencing tools to see the data types. What is supposed to be stored instruct device_private`? This is the structure which I'm now using to get the data I need. I'd truly appreciate someone commenting on this answer to explain that one.

Thanks to @myaut and @alexhoppus