UIO: missing resource mapping

UIO: missing resource mapping

[Andreas Schallenberg]

Hello,

I'm doing experiments with the Userspace IO driver (UIO_PCI_GENERIC) and 
a set of PCIe cards. The kernel version is 3.4.4, CPU is a Marvell 
MV78200 (ARMv5te). Example with an Intel ethernet card:

This makes /dev/uio0 appear
echo -n "8086 10d3" >/sys/bus/pci/drivers/uio_pci_generic/new_id

# lspci -v -k -s 0000:00:01.0
00:01.0 Ethernet controller: Intel Corporation 82574L Gigabit Network 
Connection
         Subsystem: Intel Corporation Gigabit CT Desktop Adapter
         Flags: bus master, fast devsel, latency 0, IRQ 36
         Memory at c00c0000 (32-bit, non-prefetchable) [size=128K]
         Memory at c0000000 (32-bit, non-prefetchable) [size=512K]
         I/O ports at f0800000 [size=32]
         Memory at c00e0000 (32-bit, non-prefetchable) [size=16K]
         [virtual] Expansion ROM at c0080000 [disabled] [size=256K]
         Capabilities: [c8] Power Management version 2
         Capabilities: [d0] MSI: Enable- Count=1/1 Maskable- 64bit+
         Capabilities: [e0] Express Endpoint, MSI 00
         Capabilities: [a0] MSI-X: Enable- Count=5 Masked-
         Capabilities: [100] Advanced Error Reporting
         Capabilities: [140] Device Serial Number 00-1b-21-ff-ff-c4-f7-2f
         Kernel driver in use: uio_pci_generic

Suprisingly, there is no "maps" directory in /sys/class/uio/uio0 now. I 
made a small change to the UIO code:

diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
index a783d53..b639654 100644
--- a/drivers/uio/uio.c
+++ b/drivers/uio/uio.c
@@ -274,8 +274,10 @@ static int uio_dev_add_attributes(struct uio_device 
*idev)

         for (mi = 0; mi < MAX_UIO_MAPS; mi++) {
                 mem = &idev->info->mem[mi];
-               if (mem->size == 0)
-                       break;
+               if (mem->size == 0) {
+                       dev_err(idev->dev, "map %d has zero size\n", mi);
+                       continue; // was: break
+               }
                 if (!map_found) {
                         map_found = 1;
                         idev->map_dir = kobject_create_and_add("maps",

If I have this in the kernel and give the "echo" command as shown above 
I get:
[   43.761723] uio uio0: map 0 has zero size
[   43.765760] uio uio0: map 1 has zero size
[   43.769787] uio uio0: map 2 has zero size
[   43.774298] uio uio0: map 3 has zero size
[   43.778333] uio uio0: map 4 has zero size

On the other hand, I can see the resources:
# cat /sys/class/uio/uio0/device/resource
0x00000000c00c0000 0x00000000c00dffff 0x0000000000040200
0x00000000c0000000 0x00000000c007ffff 0x0000000000040200
0x00000000f0800000 0x00000000f080001f 0x0000000000040101
0x00000000c00e0000 0x00000000c00e3fff 0x0000000000040200
...more...

Looking further at the code, I cannot see where the mem fields are being 
filled at all.
Which code is supposed to write the struct uio_mem?

Best,
Andreas

Re: UIO: missing resource mapping

[Hans J. Koch]

On Thu, Jul 12, 2012 at 09:26:23AM +0200, Andreas Schallenberg wrote:

[Added more people to Cc:]

> Hello,
> 
> I'm doing experiments with the Userspace IO driver (UIO_PCI_GENERIC)
> and a set of PCIe cards. The kernel version is 3.4.4, CPU is a
> Marvell MV78200 (ARMv5te). Example with an Intel ethernet card:
> 
> This makes /dev/uio0 appear
> echo -n "8086 10d3" >/sys/bus/pci/drivers/uio_pci_generic/new_id
> 
> # lspci -v -k -s 0000:00:01.0
> 00:01.0 Ethernet controller: Intel Corporation 82574L Gigabit
> Network Connection
>         Subsystem: Intel Corporation Gigabit CT Desktop Adapter
>         Flags: bus master, fast devsel, latency 0, IRQ 36
>         Memory at c00c0000 (32-bit, non-prefetchable) [size=128K]
>         Memory at c0000000 (32-bit, non-prefetchable) [size=512K]
>         I/O ports at f0800000 [size=32]
>         Memory at c00e0000 (32-bit, non-prefetchable) [size=16K]
>         [virtual] Expansion ROM at c0080000 [disabled] [size=256K]
>         Capabilities: [c8] Power Management version 2
>         Capabilities: [d0] MSI: Enable- Count=1/1 Maskable- 64bit+
>         Capabilities: [e0] Express Endpoint, MSI 00
>         Capabilities: [a0] MSI-X: Enable- Count=5 Masked-
>         Capabilities: [100] Advanced Error Reporting
>         Capabilities: [140] Device Serial Number 00-1b-21-ff-ff-c4-f7-2f
>         Kernel driver in use: uio_pci_generic
> 
> Suprisingly, there is no "maps" directory in /sys/class/uio/uio0
> now. I made a small change to the UIO code:
> 
> diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
> index a783d53..b639654 100644
> --- a/drivers/uio/uio.c
> +++ b/drivers/uio/uio.c
> @@ -274,8 +274,10 @@ static int uio_dev_add_attributes(struct
> uio_device *idev)
> 
>         for (mi = 0; mi < MAX_UIO_MAPS; mi++) {
>                 mem = &idev->info->mem[mi];
> -               if (mem->size == 0)
> -                       break;
> +               if (mem->size == 0) {
> +                       dev_err(idev->dev, "map %d has zero size\n", mi);
> +                       continue; // was: break
> +               }
>                 if (!map_found) {
>                         map_found = 1;
>                         idev->map_dir = kobject_create_and_add("maps",
> 
> If I have this in the kernel and give the "echo" command as shown
> above I get:
> [   43.761723] uio uio0: map 0 has zero size
> [   43.765760] uio uio0: map 1 has zero size
> [   43.769787] uio uio0: map 2 has zero size
> [   43.774298] uio uio0: map 3 has zero size
> [   43.778333] uio uio0: map 4 has zero size
> 
> On the other hand, I can see the resources:
> # cat /sys/class/uio/uio0/device/resource
> 0x00000000c00c0000 0x00000000c00dffff 0x0000000000040200
> 0x00000000c0000000 0x00000000c007ffff 0x0000000000040200
> 0x00000000f0800000 0x00000000f080001f 0x0000000000040101
> 0x00000000c00e0000 0x00000000c00e3fff 0x0000000000040200
> ...more...
> 
> Looking further at the code, I cannot see where the mem fields are
> being filled at all.
> Which code is supposed to write the struct uio_mem?

In my opinion, the driver should. However, Michael's idea is to use
/sys/bus/pci/devices/XXXXXresourceX for mapping purposes.

That is of course also possible, but obviously it leads to confusion.
We already had a long thread about this:

http://www.spinics.net/lists/kvm/msg73837.html

Michael, can we change the driver to offer all available PCI BARs in the
normal UIO way? I'm afraid otherwise we'll have the same discussion over
and over again.

Thanks,
Hans

Re: UIO: missing resource mapping

[Michael S. Tsirkin]

On Thu, Jul 12, 2012 at 09:44:33PM +0200, Hans J. Koch wrote:
> On Thu, Jul 12, 2012 at 09:26:23AM +0200, Andreas Schallenberg wrote:
> 
> [Added more people to Cc:]
> 
> > Hello,
> > 
> > I'm doing experiments with the Userspace IO driver (UIO_PCI_GENERIC)
> > and a set of PCIe cards. The kernel version is 3.4.4, CPU is a
> > Marvell MV78200 (ARMv5te). Example with an Intel ethernet card:
> > 
> > This makes /dev/uio0 appear
> > echo -n "8086 10d3" >/sys/bus/pci/drivers/uio_pci_generic/new_id
> > 
> > # lspci -v -k -s 0000:00:01.0
> > 00:01.0 Ethernet controller: Intel Corporation 82574L Gigabit
> > Network Connection
> >         Subsystem: Intel Corporation Gigabit CT Desktop Adapter
> >         Flags: bus master, fast devsel, latency 0, IRQ 36
> >         Memory at c00c0000 (32-bit, non-prefetchable) [size=128K]
> >         Memory at c0000000 (32-bit, non-prefetchable) [size=512K]
> >         I/O ports at f0800000 [size=32]
> >         Memory at c00e0000 (32-bit, non-prefetchable) [size=16K]
> >         [virtual] Expansion ROM at c0080000 [disabled] [size=256K]
> >         Capabilities: [c8] Power Management version 2
> >         Capabilities: [d0] MSI: Enable- Count=1/1 Maskable- 64bit+
> >         Capabilities: [e0] Express Endpoint, MSI 00
> >         Capabilities: [a0] MSI-X: Enable- Count=5 Masked-
> >         Capabilities: [100] Advanced Error Reporting
> >         Capabilities: [140] Device Serial Number 00-1b-21-ff-ff-c4-f7-2f
> >         Kernel driver in use: uio_pci_generic
> > 
> > Suprisingly, there is no "maps" directory in /sys/class/uio/uio0
> > now. I made a small change to the UIO code:
> > 
> > diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
> > index a783d53..b639654 100644
> > --- a/drivers/uio/uio.c
> > +++ b/drivers/uio/uio.c
> > @@ -274,8 +274,10 @@ static int uio_dev_add_attributes(struct
> > uio_device *idev)
> > 
> >         for (mi = 0; mi < MAX_UIO_MAPS; mi++) {
> >                 mem = &idev->info->mem[mi];
> > -               if (mem->size == 0)
> > -                       break;
> > +               if (mem->size == 0) {
> > +                       dev_err(idev->dev, "map %d has zero size\n", mi);
> > +                       continue; // was: break
> > +               }
> >                 if (!map_found) {
> >                         map_found = 1;
> >                         idev->map_dir = kobject_create_and_add("maps",
> > 
> > If I have this in the kernel and give the "echo" command as shown
> > above I get:
> > [   43.761723] uio uio0: map 0 has zero size
> > [   43.765760] uio uio0: map 1 has zero size
> > [   43.769787] uio uio0: map 2 has zero size
> > [   43.774298] uio uio0: map 3 has zero size
> > [   43.778333] uio uio0: map 4 has zero size
> > 
> > On the other hand, I can see the resources:
> > # cat /sys/class/uio/uio0/device/resource
> > 0x00000000c00c0000 0x00000000c00dffff 0x0000000000040200
> > 0x00000000c0000000 0x00000000c007ffff 0x0000000000040200
> > 0x00000000f0800000 0x00000000f080001f 0x0000000000040101
> > 0x00000000c00e0000 0x00000000c00e3fff 0x0000000000040200
> > ...more...
> > 
> > Looking further at the code, I cannot see where the mem fields are
> > being filled at all.
> > Which code is supposed to write the struct uio_mem?
> 
> In my opinion, the driver should. However, Michael's idea is to use
> /sys/bus/pci/devices/XXXXXresourceX for mapping purposes.
> 
> That is of course also possible, but obviously it leads to confusion.
> We already had a long thread about this:
> 
> http://www.spinics.net/lists/kvm/msg73837.html
> 
> Michael, can we change the driver to offer all available PCI BARs in the
> normal UIO way? I'm afraid otherwise we'll have the same discussion over
> and over again.
> 
> Thanks,
> Hans

My concern was people will ask for more and more stuff that pci
sysfs already has.
If we do add these is there a way to not duplicate code from pci?
Export pci_mmap_resource and use it? Or make the uio attributes
softlinks to pci sysfs somehow?
Alternatively we could just clarify this in the documentation
which is of course easier and gives us less code to maintain ...

-- 
MST

Re: UIO: missing resource mapping

[Hans J. Koch]

On Fri, Jul 13, 2012 at 02:16:32AM +0300, Michael S. Tsirkin wrote:
> On Thu, Jul 12, 2012 at 09:44:33PM +0200, Hans J. Koch wrote:
> > > Looking further at the code, I cannot see where the mem fields are
> > > being filled at all.
> > > Which code is supposed to write the struct uio_mem?
> > 
> > In my opinion, the driver should. However, Michael's idea is to use
> > /sys/bus/pci/devices/XXXXXresourceX for mapping purposes.
> > 
> > That is of course also possible, but obviously it leads to confusion.
> > We already had a long thread about this:
> > 
> > http://www.spinics.net/lists/kvm/msg73837.html
> > 
> > Michael, can we change the driver to offer all available PCI BARs in the
> > normal UIO way? I'm afraid otherwise we'll have the same discussion over
> > and over again.
> > 
> > Thanks,
> > Hans
> 
> My concern was people will ask for more and more stuff that pci
> sysfs already has.
> If we do add these is there a way to not duplicate code from pci?
> Export pci_mmap_resource and use it? Or make the uio attributes
> softlinks to pci sysfs somehow?

I understand your concern. Of course I'm also not in favor of duplicating
code, but I don't think there's much overhead. PCI already exports all
information needed by pci_resource_start(dev, bar) and
pci_resource_len(dev, bar). Have a look at other UIO PCI drivers like
uio_cif.c or uio_netx.c to see how it works. It's just a few more lines
of code to loop through all BARs and add the to info->mem[i].

By the way, the current size of the info->mem[] array was exactly made
with PCI in mind...

Thanks,
Hans

Re: UIO: missing resource mapping

[Michael S. Tsirkin]

On Fri, Jul 13, 2012 at 01:40:11AM +0200, Hans J. Koch wrote:
> On Fri, Jul 13, 2012 at 02:16:32AM +0300, Michael S. Tsirkin wrote:
> > On Thu, Jul 12, 2012 at 09:44:33PM +0200, Hans J. Koch wrote:
> > > > Looking further at the code, I cannot see where the mem fields are
> > > > being filled at all.
> > > > Which code is supposed to write the struct uio_mem?
> > > 
> > > In my opinion, the driver should. However, Michael's idea is to use
> > > /sys/bus/pci/devices/XXXXXresourceX for mapping purposes.
> > > 
> > > That is of course also possible, but obviously it leads to confusion.
> > > We already had a long thread about this:
> > > 
> > > http://www.spinics.net/lists/kvm/msg73837.html
> > > 
> > > Michael, can we change the driver to offer all available PCI BARs in the
> > > normal UIO way? I'm afraid otherwise we'll have the same discussion over
> > > and over again.
> > > 
> > > Thanks,
> > > Hans
> > 
> > My concern was people will ask for more and more stuff that pci
> > sysfs already has.
> > If we do add these is there a way to not duplicate code from pci?
> > Export pci_mmap_resource and use it? Or make the uio attributes
> > softlinks to pci sysfs somehow?
> 
> I understand your concern. Of course I'm also not in favor of duplicating
> code, but I don't think there's much overhead. PCI already exports all
> information needed by pci_resource_start(dev, bar) and
> pci_resource_len(dev, bar). Have a look at other UIO PCI drivers like
> uio_cif.c or uio_netx.c to see how it works. It's just a few more lines
> of code to loop through all BARs and add the to info->mem[i].
> 
> By the way, the current size of the info->mem[] array was exactly made
> with PCI in mind...
> 
> Thanks,
> Hans

Well we also need to check resource type and probably iomem_is_exclusive.

-- 
MST

Re: UIO: missing resource mapping

[Dominic Eschweiler]

Am Freitag, den 13.07.2012, 02:16 +0300 schrieb Michael S. Tsirkin:
> My concern was people will ask for more and more stuff that pci
> sysfs already has.
> If we do add these is there a way to not duplicate code from pci? 

I have some concerns about the placing for the BAR mapping code inside
the kernel. The point is, that sysfs currently makes it possible to map
BARs of all card which are handled by any driver. This is fine in case
of UIO, because it is intended that a user-space program maps BARs, but
it is also possible to map BARs that are already handle by a kernel
driver. It i therefore possible to jam the system by confusing sysfs
entries.

I don't know which implications this has, but I would move the BAR
mapping capabilities completely to UIO. This should ensure that only
BARs can be mapped, which are handled by UIO and no other kernel-space
driver.

-- 
Gruß
  Dominic

Frankfurt Institute for Advanced Studies (FIAS)
Ruth-Moufang-Straße 1
D-60438 Frankfurt am Main
Germany

Phone:  +49 69 79844114

Re: UIO: missing resource mapping

[Michael S. Tsirkin]

On Fri, Jul 13, 2012 at 10:09:15AM +0200, Dominic Eschweiler wrote:
> Am Freitag, den 13.07.2012, 02:16 +0300 schrieb Michael S. Tsirkin:
> > My concern was people will ask for more and more stuff that pci
> > sysfs already has.
> > If we do add these is there a way to not duplicate code from pci? 
> 
> I have some concerns about the placing for the BAR mapping code inside
> the kernel. The point is, that sysfs currently makes it possible to map
> BARs of all card which are handled by any driver. This is fine in case
> of UIO, because it is intended that a user-space program maps BARs, but
> it is also possible to map BARs that are already handle by a kernel
> driver. It i therefore possible to jam the system by confusing sysfs
> entries.
> 
> I don't know which implications this has, but I would move the BAR
> mapping capabilities completely to UIO. This should ensure that only
> BARs can be mapped, which are handled by UIO and no other kernel-space
> driver.

Could you give an example of the problem? How do you bind
both UIO and another driver to the same device?

-- 
MST

Re: UIO: missing resource mapping

[Hans J. Koch]

On Fri, Jul 13, 2012 at 04:22:23PM +0300, Michael S. Tsirkin wrote:
> On Fri, Jul 13, 2012 at 10:09:15AM +0200, Dominic Eschweiler wrote:
> > Am Freitag, den 13.07.2012, 02:16 +0300 schrieb Michael S. Tsirkin:
> > > My concern was people will ask for more and more stuff that pci
> > > sysfs already has.
> > > If we do add these is there a way to not duplicate code from pci? 
> > 
> > I have some concerns about the placing for the BAR mapping code inside
> > the kernel. The point is, that sysfs currently makes it possible to map
> > BARs of all card which are handled by any driver. This is fine in case
> > of UIO, because it is intended that a user-space program maps BARs, but
> > it is also possible to map BARs that are already handle by a kernel
> > driver. It i therefore possible to jam the system by confusing sysfs
> > entries.
> > 
> > I don't know which implications this has, but I would move the BAR
> > mapping capabilities completely to UIO. This should ensure that only
> > BARs can be mapped, which are handled by UIO and no other kernel-space
> > driver.
> 
> Could you give an example of the problem? How do you bind
> both UIO and another driver to the same device?

You don't. If you already have a driver for your PCI device, why would you
want to write a UIO driver? The sysfs files we were talking about are
generated by the PCI core, which is not a driver. They simply reflect what
the kernel can find out about the device.

If somebody maps the card's memory through the UIO driver and somebody else
also maps it using sysfs, that is possible. What happens if both write to
the same hardware registers is a different topic. Writing a driver implies
some responsibility, even if it's done in userspace.

Thanks,
Hans

Re: UIO: missing resource mapping

[Dominic Eschweiler]

Am Freitag, den 13.07.2012, 16:22 +0300 schrieb Michael S. Tsirkin:
> Could you give an example of the problem? How do you bind
> both UIO and another driver to the same device? 

Sorry, I'm looking on it from the user-space perspective. Maybe I'm
wrong, but I can give you an example :

lspci -v
...
03:00.0 Network controller: Broadcom Corporation BCM4331 802.11a/b/g/n
(rev 02)
	Subsystem: Apple Inc. AirPort Extreme
	Flags: bus master, fast devsel, latency 0, IRQ 17
	Memory at b0600000 (64-bit, non-prefetchable) [size=16K]
	Capabilities: [40] Power Management version 3
	Capabilities: [58] Vendor Specific Information: Len=78 <?>
	Capabilities: [48] MSI: Enable- Count=1/1 Maskable- 64bit+
	Capabilities: [d0] Express Endpoint, MSI 00
	Capabilities: [100] Advanced Error Reporting
	Capabilities: [13c] Virtual Channel
	Capabilities: [160] Device Serial Number 85-f2-6d-ff-ff-42-68-a8
	Capabilities: [16c] Power Budgeting <?>
	Kernel driver in use: bcma-pci-bridge
...

This Device has one 64 Bit Bar. When I look at the related sysfs
entry ...


ls /sys/bus/pci/devices/0000:03:00.0
...
--w------- 1 root root 4.0K Jul 13 16:35 reset
-r--r--r-- 1 root root 4.0K Jul 12 21:43 resource
-rw------- 1 root root  16K Jul 13 16:35 resource0
lrwxrwxrwx 1 root root    0 Jul 12 23:41 subsystem
-> ../../../../bus/pci
...

... I can see that it should be possible to map resource0 and directly
write into a BAR which is already managed by a kernel drivers. 

Moving this functionality to UIO would only generate those resource
files, if the device is handled by UIO and therefore intended to be
managed from the user-space. 

-- 
Gruß
  Dominic

Re: UIO: missing resource mapping

[Dominic Eschweiler]

Am Freitag, den 13.07.2012, 16:18 +0200 schrieb Hans J. Koch:
> If somebody maps the card's memory through the UIO driver and somebody
> else also maps it using sysfs, that is possible. What happens if both
> write to the same hardware registers is a different topic. Writing a
> driver implies some responsibility, even if it's done in userspace. 

That's true and I would not complain. I'm just saying that there is the
possibility to disturb a kernel driver by mapping memory etc. I think it
is worth to considerate moving the BAR mapping code to UIO. Especially,
when I look at the discussions about what effects mappable DMA memory
can have on the system security. 

-- 
Gruß
  Dominic

Re: UIO: missing resource mapping

[Michael S. Tsirkin]

On Fri, Jul 13, 2012 at 04:42:51PM +0200, Dominic Eschweiler wrote:
> Am Freitag, den 13.07.2012, 16:22 +0300 schrieb Michael S. Tsirkin:
> > Could you give an example of the problem? How do you bind
> > both UIO and another driver to the same device? 
> 
> Sorry, I'm looking on it from the user-space perspective. Maybe I'm
> wrong, but I can give you an example :
> 
> lspci -v
> ...
> 03:00.0 Network controller: Broadcom Corporation BCM4331 802.11a/b/g/n
> (rev 02)
> 	Subsystem: Apple Inc. AirPort Extreme
> 	Flags: bus master, fast devsel, latency 0, IRQ 17
> 	Memory at b0600000 (64-bit, non-prefetchable) [size=16K]
> 	Capabilities: [40] Power Management version 3
> 	Capabilities: [58] Vendor Specific Information: Len=78 <?>
> 	Capabilities: [48] MSI: Enable- Count=1/1 Maskable- 64bit+
> 	Capabilities: [d0] Express Endpoint, MSI 00
> 	Capabilities: [100] Advanced Error Reporting
> 	Capabilities: [13c] Virtual Channel
> 	Capabilities: [160] Device Serial Number 85-f2-6d-ff-ff-42-68-a8
> 	Capabilities: [16c] Power Budgeting <?>
> 	Kernel driver in use: bcma-pci-bridge
> ...
> 
> This Device has one 64 Bit Bar. When I look at the related sysfs
> entry ...
> 
> 
> ls /sys/bus/pci/devices/0000:03:00.0
> ...
> --w------- 1 root root 4.0K Jul 13 16:35 reset
> -r--r--r-- 1 root root 4.0K Jul 12 21:43 resource
> -rw------- 1 root root  16K Jul 13 16:35 resource0
> lrwxrwxrwx 1 root root    0 Jul 12 23:41 subsystem
> -> ../../../../bus/pci
> ...
> 
> ... I can see that it should be possible to map resource0 and directly
> write into a BAR which is already managed by a kernel drivers. 
> 
> Moving this functionality to UIO would only generate those resource
> files, if the device is handled by UIO and therefore intended to be
> managed from the user-space. 

UIO has the same property, doesn't it? Multiple users can
access device memory through sysfs.


> -- 
> Gruß
>   Dominic

Re: UIO: missing resource mapping

[Dominic Eschweiler]

Am Freitag, den 13.07.2012, 21:19 +0300 schrieb Michael S. Tsirkin:
> 
> UIO has the same property, doesn't it? Multiple users can
> access device memory through sysfs. 


Indeed, that's a similar problem. I haven't tried it (yet), but this
particular problem can maybe circumvented by using mmap with the
MAP_PRIVATE flag. Doing so is the responsibility of the driver
programmer (like Hans already said). Even if that mmap trick does not
work, it is pretty much sure that a BAR is already used by another
program, if a related kernel driver is loaded. In that case the kernel
has a chance to avoid such BAR race conditions by not giving the
possibility to map them to the userspace.

Nevertheless, I'm pretty sure that the possibility via sysfs to access
BARs, which are already managed by a kernel driver, opens the door for
denial of service attacks.

On the other hand, I'm quite a newbie on this topic and maybe I don't
see the big picture here. Therefore it is up to you guys to make the
right decision (if needed). 

-- 
Gruß
  Dominic

Re: UIO: missing resource mapping

[Hans J. Koch]

On Mon, Jul 16, 2012 at 08:16:12PM +0200, Dominic Eschweiler wrote:
> Am Freitag, den 13.07.2012, 21:19 +0300 schrieb Michael S. Tsirkin:
> > 
> > UIO has the same property, doesn't it? Multiple users can
> > access device memory through sysfs. 
> 
> 
> Indeed, that's a similar problem. I haven't tried it (yet), but this
> particular problem can maybe circumvented by using mmap with the
> MAP_PRIVATE flag. Doing so is the responsibility of the driver
> programmer (like Hans already said). Even if that mmap trick does not
> work, it is pretty much sure that a BAR is already used by another
> program, if a related kernel driver is loaded. In that case the kernel
> has a chance to avoid such BAR race conditions by not giving the
> possibility to map them to the userspace.

Don't make it more complicated than it is. I see no general problem in
mapping BARs in uio_pci_generic like in any other UIO PCI driver.

> 
> Nevertheless, I'm pretty sure that the possibility via sysfs to access
> BARs, which are already managed by a kernel driver, opens the door for
> denial of service attacks.

There are also a few other possible attack scenarios, depending on the
hardware.
That's a general problem of all userspace drivers (e.g. X graphics drivers).
You have to make sure access rights are correct. Making a UIO device node
available to all normal users is foolishly dangerous.

> 
> On the other hand, I'm quite a newbie on this topic and maybe I don't
> see the big picture here. Therefore it is up to you guys to make the
> right decision (if needed).

Try to hack up a patch to add generic BAR mapping to uio_pci_generic.c
and post it for review.

Thanks,
Hans

Re: UIO: missing resource mapping

[Dominic Eschweiler]

Am Montag, den 16.07.2012, 23:58 +0200 schrieb Hans J. Koch:
> Try to hack up a patch to add generic BAR mapping to uio_pci_generic.c
> and post it for review.
> 

Here we go ...
> 
Signed-off-by: Dominic Eschweiler <eschweiler@fias.uni-frankfurt.de>
diff --git a/drivers/uio/uio_pci_generic.c
b/drivers/uio/uio_pci_generic.c
index 0bd08ef..e25991e 100644
--- a/drivers/uio/uio_pci_generic.c
+++ b/drivers/uio/uio_pci_generic.c
@@ -25,10 +25,12 @@
 #include <linux/slab.h>
 #include <linux/uio_driver.h>
 
-#define DRIVER_VERSION	"0.01.0"
+#define DRIVER_VERSION	"0.02.0"
 #define DRIVER_AUTHOR	"Michael S. Tsirkin <mst@redhat.com>"
 #define DRIVER_DESC	"Generic UIO driver for PCI 2.3 devices"
 
+#define DRV_NAME "uio_pci_generic"
+
 struct uio_pci_generic_dev {
 	struct uio_info info;
 	struct pci_dev *pdev;
@@ -58,6 +60,7 @@ static int __devinit probe(struct pci_dev *pdev,
 {
 	struct uio_pci_generic_dev *gdev;
 	int err;
+	int i;
 
 	err = pci_enable_device(pdev);
 	if (err) {
@@ -67,8 +70,7 @@ static int __devinit probe(struct pci_dev *pdev,
 	}
 
 	if (!pdev->irq) {
-		dev_warn(&pdev->dev, "No IRQ assigned to device: "
-			 "no support for interrupts?\n");
+		dev_warn(&pdev->dev, "No IRQ assigned to device: no support for
interrupts?\n");
 		pci_disable_device(pdev);
 		return -ENODEV;
 	}
@@ -91,10 +93,31 @@ static int __devinit probe(struct pci_dev *pdev,
 	gdev->info.handler = irqhandler;
 	gdev->pdev = pdev;
 
+	/* request regions */
+	err = pci_request_regions(pdev, DRV_NAME);
+	if (err) {
+		dev_err(&pdev->dev, "Couldn't get PCI resources, aborting\n");
+		return err;
+	}
+
+	/* create attributes for BAR mappings */
+	for (i = 0; i < PCI_NUM_RESOURCES; i++) {
+		if (pdev->resource[i].flags &&
+		(pdev->resource[i].flags & IORESOURCE_MEM)) {
+			gdev->info.mem[i].addr = pci_resource_start(pdev, i);
+			gdev->info.mem[i].size = pci_resource_len(pdev, i);
+			gdev->info.mem[i].internal_addr = NULL;
+			gdev->info.mem[i].memtype = UIO_MEM_PHYS;
+		}
+	}
+
 	if (uio_register_device(&pdev->dev, &gdev->info))
 		goto err_register;
 	pci_set_drvdata(pdev, gdev);
 
+	pr_info("UIO_PCI_GENERIC : initialized new device (%x %x)\n",
+	pdev->vendor, pdev->device);
+
 	return 0;
 err_register:
 	kfree(gdev);
@@ -107,17 +130,21 @@ err_verify:
 static void remove(struct pci_dev *pdev)
 {
 	struct uio_pci_generic_dev *gdev = pci_get_drvdata(pdev);
-
 	uio_unregister_device(&gdev->info);
+
+	pci_release_regions(pdev);
 	pci_disable_device(pdev);
 	kfree(gdev);
+
+	pr_info("UIO_PCI_GENERIC : removed device (%x %x)\n",
+	pdev->vendor, pdev->device);
 }
 
 static struct pci_driver driver = {
-	.name = "uio_pci_generic",
+	.name     = DRV_NAME,
 	.id_table = NULL, /* only dynamic id's */
-	.probe = probe,
-	.remove = remove,
+	.probe    = probe,
+	.remove   = remove,
 };
 
 static int __init init(void)

-- 
Gruß
  Dominic

Frankfurt Institute for Advanced Studies (FIAS)
Ruth-Moufang-Straße 1
D-60438 Frankfurt am Main
Germany

Phone:  +49 69 79844114

Re: UIO: missing resource mapping

[Hans J. Koch]

On Wed, Jul 18, 2012 at 12:40:47PM +0200, Dominic Eschweiler wrote:
> Am Montag, den 16.07.2012, 23:58 +0200 schrieb Hans J. Koch:
> > Try to hack up a patch to add generic BAR mapping to uio_pci_generic.c
> > and post it for review.
> > 
> 
> Here we go ...

Great! I'm a bit under time pressure with my current work ATM. A call to
open("/dev/hansjkoch", O_NONBLOCK) only returns -EBUSY. So it'll take me one or
two days to review it thoroughly. But at first sight, it is what I had in mind.

You'll hear from me soon, thanks for your work! Comments and reviews from
others are welcome...

Hans

> > 
> Signed-off-by: Dominic Eschweiler <eschweiler@fias.uni-frankfurt.de>
> diff --git a/drivers/uio/uio_pci_generic.c
> b/drivers/uio/uio_pci_generic.c
> index 0bd08ef..e25991e 100644
> --- a/drivers/uio/uio_pci_generic.c
> +++ b/drivers/uio/uio_pci_generic.c
> @@ -25,10 +25,12 @@
>  #include <linux/slab.h>
>  #include <linux/uio_driver.h>
>  
> -#define DRIVER_VERSION	"0.01.0"
> +#define DRIVER_VERSION	"0.02.0"
>  #define DRIVER_AUTHOR	"Michael S. Tsirkin <mst@redhat.com>"
>  #define DRIVER_DESC	"Generic UIO driver for PCI 2.3 devices"
>  
> +#define DRV_NAME "uio_pci_generic"
> +
>  struct uio_pci_generic_dev {
>  	struct uio_info info;
>  	struct pci_dev *pdev;
> @@ -58,6 +60,7 @@ static int __devinit probe(struct pci_dev *pdev,
>  {
>  	struct uio_pci_generic_dev *gdev;
>  	int err;
> +	int i;
>  
>  	err = pci_enable_device(pdev);
>  	if (err) {
> @@ -67,8 +70,7 @@ static int __devinit probe(struct pci_dev *pdev,
>  	}
>  
>  	if (!pdev->irq) {
> -		dev_warn(&pdev->dev, "No IRQ assigned to device: "
> -			 "no support for interrupts?\n");
> +		dev_warn(&pdev->dev, "No IRQ assigned to device: no support for
> interrupts?\n");
>  		pci_disable_device(pdev);
>  		return -ENODEV;
>  	}
> @@ -91,10 +93,31 @@ static int __devinit probe(struct pci_dev *pdev,
>  	gdev->info.handler = irqhandler;
>  	gdev->pdev = pdev;
>  
> +	/* request regions */
> +	err = pci_request_regions(pdev, DRV_NAME);
> +	if (err) {
> +		dev_err(&pdev->dev, "Couldn't get PCI resources, aborting\n");
> +		return err;
> +	}
> +
> +	/* create attributes for BAR mappings */
> +	for (i = 0; i < PCI_NUM_RESOURCES; i++) {
> +		if (pdev->resource[i].flags &&
> +		(pdev->resource[i].flags & IORESOURCE_MEM)) {
> +			gdev->info.mem[i].addr = pci_resource_start(pdev, i);
> +			gdev->info.mem[i].size = pci_resource_len(pdev, i);
> +			gdev->info.mem[i].internal_addr = NULL;
> +			gdev->info.mem[i].memtype = UIO_MEM_PHYS;
> +		}
> +	}
> +
>  	if (uio_register_device(&pdev->dev, &gdev->info))
>  		goto err_register;
>  	pci_set_drvdata(pdev, gdev);
>  
> +	pr_info("UIO_PCI_GENERIC : initialized new device (%x %x)\n",
> +	pdev->vendor, pdev->device);
> +
>  	return 0;
>  err_register:
>  	kfree(gdev);
> @@ -107,17 +130,21 @@ err_verify:
>  static void remove(struct pci_dev *pdev)
>  {
>  	struct uio_pci_generic_dev *gdev = pci_get_drvdata(pdev);
> -
>  	uio_unregister_device(&gdev->info);
> +
> +	pci_release_regions(pdev);
>  	pci_disable_device(pdev);
>  	kfree(gdev);
> +
> +	pr_info("UIO_PCI_GENERIC : removed device (%x %x)\n",
> +	pdev->vendor, pdev->device);
>  }
>  
>  static struct pci_driver driver = {
> -	.name = "uio_pci_generic",
> +	.name     = DRV_NAME,
>  	.id_table = NULL, /* only dynamic id's */
> -	.probe = probe,
> -	.remove = remove,
> +	.probe    = probe,
> +	.remove   = remove,
>  };
>  
>  static int __init init(void)
> 
> -- 
> Gruß
>   Dominic
> 
> Frankfurt Institute for Advanced Studies (FIAS)
> Ruth-Moufang-Straße 1
> D-60438 Frankfurt am Main
> Germany
> 
> Phone:  +49 69 79844114
> 
> 

Re: UIO: missing resource mapping

[Dominic Eschweiler]

Hello Hans

Am Donnerstag, den 19.07.2012, 01:47 +0200 schrieb Hans J. Koch:
> You'll hear from me soon, thanks for your work! Comments and reviews
> from others are welcome... 

Is there any progress on this topic?

-- 
Gruß
  Dominic

Frankfurt Institute for Advanced Studies (FIAS)
Ruth-Moufang-Straße 1
D-60438 Frankfurt am Main
Germany

Phone:  +49 69 79844114

Re: UIO: missing resource mapping

[Hans J. Koch]

On Wed, Jul 18, 2012 at 12:40:47PM +0200, Dominic Eschweiler wrote:
> Am Montag, den 16.07.2012, 23:58 +0200 schrieb Hans J. Koch:
> > Try to hack up a patch to add generic BAR mapping to uio_pci_generic.c
> > and post it for review.
> > 
> 
> Here we go ...

Thank you very much for your work. I'm really sorry for the long delay,
but I was busy finishing a project because I go to vacation tomorrow.
Sorry, that might cause further delay since I don't know yet how often
I can read my mail...
Greg, can you review the next one?

Here's a first review.

Thanks,
Hans

> > 
> Signed-off-by: Dominic Eschweiler <eschweiler@fias.uni-frankfurt.de>
> diff --git a/drivers/uio/uio_pci_generic.c
> b/drivers/uio/uio_pci_generic.c
> index 0bd08ef..e25991e 100644
> --- a/drivers/uio/uio_pci_generic.c
> +++ b/drivers/uio/uio_pci_generic.c
> @@ -25,10 +25,12 @@
>  #include <linux/slab.h>
>  #include <linux/uio_driver.h>
>  
> -#define DRIVER_VERSION	"0.01.0"
> +#define DRIVER_VERSION	"0.02.0"
>  #define DRIVER_AUTHOR	"Michael S. Tsirkin <mst@redhat.com>"
>  #define DRIVER_DESC	"Generic UIO driver for PCI 2.3 devices"
>  
> +#define DRV_NAME "uio_pci_generic"
> +
>  struct uio_pci_generic_dev {
>  	struct uio_info info;
>  	struct pci_dev *pdev;
> @@ -58,6 +60,7 @@ static int __devinit probe(struct pci_dev *pdev,
>  {
>  	struct uio_pci_generic_dev *gdev;
>  	int err;
> +	int i;
>  
>  	err = pci_enable_device(pdev);
>  	if (err) {
> @@ -67,8 +70,7 @@ static int __devinit probe(struct pci_dev *pdev,
>  	}
>  
>  	if (!pdev->irq) {
> -		dev_warn(&pdev->dev, "No IRQ assigned to device: "
> -			 "no support for interrupts?\n");
> +		dev_warn(&pdev->dev, "No IRQ assigned to device: no support for
> interrupts?\n");

Please configure your mail client not to break lines when sending a patch.
It can't be applied like this.

Why did you make that change anyway? If it's just coding style, please send
another patch, don't mix functional changes with coding style fixes.

>  		pci_disable_device(pdev);
>  		return -ENODEV;
>  	}
> @@ -91,10 +93,31 @@ static int __devinit probe(struct pci_dev *pdev,
>  	gdev->info.handler = irqhandler;
>  	gdev->pdev = pdev;
>  
> +	/* request regions */
> +	err = pci_request_regions(pdev, DRV_NAME);
> +	if (err) {
> +		dev_err(&pdev->dev, "Couldn't get PCI resources, aborting\n");
> +		return err;
> +	}
> +
> +	/* create attributes for BAR mappings */
> +	for (i = 0; i < PCI_NUM_RESOURCES; i++) {
> +		if (pdev->resource[i].flags &&
> +		(pdev->resource[i].flags & IORESOURCE_MEM)) {
> +			gdev->info.mem[i].addr = pci_resource_start(pdev, i);
> +			gdev->info.mem[i].size = pci_resource_len(pdev, i);
> +			gdev->info.mem[i].internal_addr = NULL;
> +			gdev->info.mem[i].memtype = UIO_MEM_PHYS;
> +		}
> +	}
> +
>  	if (uio_register_device(&pdev->dev, &gdev->info))
>  		goto err_register;
>  	pci_set_drvdata(pdev, gdev);
>  
> +	pr_info("UIO_PCI_GENERIC : initialized new device (%x %x)\n",

Please use dev_info()

> +	pdev->vendor, pdev->device);
> +
>  	return 0;
>  err_register:
>  	kfree(gdev);
> @@ -107,17 +130,21 @@ err_verify:
>  static void remove(struct pci_dev *pdev)
>  {
>  	struct uio_pci_generic_dev *gdev = pci_get_drvdata(pdev);
> -
>  	uio_unregister_device(&gdev->info);
> +
> +	pci_release_regions(pdev);
>  	pci_disable_device(pdev);
>  	kfree(gdev);
> +
> +	pr_info("UIO_PCI_GENERIC : removed device (%x %x)\n",

ditto

> +	pdev->vendor, pdev->device);
>  }
>  
>  static struct pci_driver driver = {
> -	.name = "uio_pci_generic",
> +	.name     = DRV_NAME,
>  	.id_table = NULL, /* only dynamic id's */
> -	.probe = probe,
> -	.remove = remove,
> +	.probe    = probe,
> +	.remove   = remove,

As above: Please put coding style fixes in an extra patch (if you really
insist on tabs instead of spaces...)

>  };
>  
>  static int __init init(void)
> 
> -- 
> Gruß
>   Dominic
> 
> Frankfurt Institute for Advanced Studies (FIAS)
> Ruth-Moufang-Straße 1
> D-60438 Frankfurt am Main
> Germany
> 
> Phone:  +49 69 79844114
> 
>