linux-kernel - Re: [PATCH 1/2] vga: implements VGA arbitration on Linux

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20090716094855.455c3f77@lxorguk.ukuu.org.uk>
Date:	Thu, 16 Jul 2009 09:48:55 +0100
From:	Alan Cox <alan@...rguk.ukuu.org.uk>
To:	Dave Airlie <airlied@...il.com>
Cc:	Tiago Vignatti <tiago.vignatti@...ia.com>,
	Jesse Barnes <jbarnes@...tuousgeek.org>,
	Dave Airlie <airlied@...hat.com>, linux-pci@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] vga: implements VGA arbitration on Linux

> >> +     pci_read_config_word(pdev, PCI_COMMAND, &cmd);
> >> +     if (rsrc & (VGA_RSRC_LEGACY_IO | VGA_RSRC_NORMAL_IO))
> >> +             cmd |= PCI_COMMAND_IO;
> >> +     if (rsrc & (VGA_RSRC_LEGACY_MEM | VGA_RSRC_NORMAL_MEM))
> >> +             cmd |= PCI_COMMAND_MEMORY;
> >> +     pci_write_config_word(pdev, PCI_COMMAND, cmd);
> >
> > Locking question - what locks this lot against hotplug also touching
> > bridge settings ?
> 
> well here we just bang on device config space registers which means we
> can probably
> race against lots of other things that rmw the PCI_COMMAND not just hotplug.
> 
> Perhaps we need some sort per device PCI command space lock,
> granted this still means we race against anyone directly hacking it
> behind our backs.

I suspect the right thing to do is to move that function into the
drivers/pci code and lock it properly there. That would keep all the
locking detail internal and private (and someone else's problem ;))

> >> +             pdev = vga_default_device();
> >
> > What if the BIOS provided device was hot unplugged ?
> 
> we just use the pdev as a cookie, if it was hot unplugged we'll
> have gotten a callback to remove it from the VGA device list
> and the lookup which happens 5 lines later inside the spinlock
> will fail.

What if I inserted a new device - the allocator might give me a new
device with the same pointer if its reusing the same slab entry for that
size. Unlikely but given a zillion boxes this starts to occur 8(

> >> +             /* We have a conflict, we wait until somebody kicks the
> >> +              * work queue. Currently we have one work queue that we
> >
> > If two drivers own half the resources and both are waiting for the rest
> > what handles the deadlock
> >

Not commented on - but a serious question would be "do we actually care
enough or are there really devices with just I/O and just vga memory
access used ?"

> > PCI device refcounting ?
> 
> Again its just used a cookie for a later lookup in our vgadev array,
> its gone away it'll have been removed from the array,
> 
> We could use pci_dev_get/pci_dev_put I suppose but its only used as
> a cookie so far.

Your cookie validity is suspect I fear. Also holding the device reference
means you stop that exact set of resources being reissued too early and
you (or clients) scribbling on them through unfortunate timing. So I
think you actually do need to grab references properly, Doesn't make the
code much more complex that I can see.

Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/