subject:"\[PATCH 33\/39\] drm\: rip out drm_core_has

[PATCH 33/39] drm: rip out drm_core_has_MTRR checks

2013-07-10 Thread Daniel Vetter

On Wed, Jul 10, 2013 at 6:27 PM, Andy Lutomirski  wrote:
> Are all of those codepaths really inaccessible in non-legacy drm
> drivers?  I didn't try to fully unravel all the ioctls and such, but
> it seems like userspace could add bufs and map them.  Since the mtrr
> code isn't very robust (reference counting?  what reference
> counting?), I'm a little bit worried that potentially enabling it in
> more cases, which your patch does, could be harmful.
>
> The arch_phys_wc stuff puts a prettier interface on the mtrr code and
> turns it off when PAT is available, but the underlying code is still
> just as bad.

Well, the entire drm bufs stuff isn't refcounted and there are indeed
legacy driver that abused this in a completely unsafe way. E.g. for
i810.ko the ddx driver in userspace creates a register mapping through
the addbuf ioctl, which the kernel driver then uses. With no
refcounting at all to prevent an Oops (and I've seen them happen, you
simply need to kill X).

So I don't think this patch will make matters worse, especially since
most drivers set DRIVER_USE_MTRR. The way to fix this up is to
holesale block out these unsafe ioctls for kernel modesetting driver,
which this series here does for a lot of cases (still a bunch of them
left though). There's no way we can fix up the ums drm drivers without
breaking userspace :(

I haven't yet gotten around to blocking out the addmap ioctls since
reviewing existing userspace code will be a real pain. But at least
addmap is restrict to CAP_SYS_ADMIN, so not a that grave exploit
issue. But I very much plan to do that audit and then disable addmap
and friends for kms drivers.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 33/39] drm: rip out drm_core_has_MTRR checks

2013-07-10 Thread Daniel Vetter

On Wed, Jul 10, 2013 at 5:41 PM, David Herrmann  
wrote:
> On Wed, Jul 10, 2013 at 5:22 PM, Daniel Vetter  
> wrote:
>> On Wed, Jul 10, 2013 at 3:51 PM, David Herrmann  
>> wrote:
 -#if __OS_HAS_MTRR
 -static inline int drm_core_has_MTRR(struct drm_device *dev)
 -{
 -   return drm_core_check_feature(dev, DRIVER_USE_MTRR);
 -}
 -#else
 -#define drm_core_has_MTRR(dev) (0)
 -#endif
 -
>>>
>>> That was the last user of DRIVER_USE_MTRR (apart from drivers setting
>>> it in .driver_features). Any reason to keep it around?
>>
>> Yeah, I guess we could rip things out. Which will also force me to
>> properly audit drivers for the eventual behaviour change this could
>> entail (in case there's an x86 driver which did not ask for an mtrr,
>> but iirc there isn't).
>
> david at david-mb ~/dev/kernel/linux $ for i in drivers/gpu/drm/* ; do if
> test -d "$i" ; then if ! grep -q USE_MTRR -r $i ; then echo $i ; fi ;
> fi ; done
> drivers/gpu/drm/exynos
> drivers/gpu/drm/gma500
> drivers/gpu/drm/i2c
> drivers/gpu/drm/nouveau
> drivers/gpu/drm/omapdrm
> drivers/gpu/drm/qxl
> drivers/gpu/drm/rcar-du
> drivers/gpu/drm/shmobile
> drivers/gpu/drm/tilcdc
> drivers/gpu/drm/ttm
> drivers/gpu/drm/udl
> drivers/gpu/drm/vmwgfx
> david at david-mb ~/dev/kernel/linux $
>
> So for x86 gma500,nouveau,qxl,udl,vmwgfx don't set DRIVER_USE_MTRR.
> But I cannot tell whether they break if we call arch_phys_wc_add/del,
> anyway. At least nouveau seemed to work here, but it doesn't use AGP
> or drm_bufs, I guess.

Cool, thanks a lot for stitching together the list of drivers to look
at. So for real KMS drivers it's the drives responsibility to add an
mtrr if it needs one. nouvea, radeon, mgag200, i915 and vmwgfx do that
already. Somehow the savage driver also ends up doing that, I have no
idea why.

Note that gma500 as a pure KMS driver doesn't need MTRR setup since
the platforms that it supports all support PAT. So no MTRRs needed to
get wc iomappings.

The mtrr support in the drm core is all for legacy mappings of garts,
framebuffers and registers. All legacy drivers set the USE_MTRR flag,
so we're good there.

All in all I think we can really just ditch this. I'll update the patch.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 33/39] drm: rip out drm_core_has_MTRR checks

2013-07-10 Thread David Herrmann

Hi

On Wed, Jul 10, 2013 at 5:22 PM, Daniel Vetter  
wrote:
> On Wed, Jul 10, 2013 at 3:51 PM, David Herrmann  
> wrote:
>>> -#if __OS_HAS_MTRR
>>> -static inline int drm_core_has_MTRR(struct drm_device *dev)
>>> -{
>>> -   return drm_core_check_feature(dev, DRIVER_USE_MTRR);
>>> -}
>>> -#else
>>> -#define drm_core_has_MTRR(dev) (0)
>>> -#endif
>>> -
>>
>> That was the last user of DRIVER_USE_MTRR (apart from drivers setting
>> it in .driver_features). Any reason to keep it around?
>
> Yeah, I guess we could rip things out. Which will also force me to
> properly audit drivers for the eventual behaviour change this could
> entail (in case there's an x86 driver which did not ask for an mtrr,
> but iirc there isn't).

david at david-mb ~/dev/kernel/linux $ for i in drivers/gpu/drm/* ; do if
test -d "$i" ; then if ! grep -q USE_MTRR -r $i ; then echo $i ; fi ;
fi ; done
drivers/gpu/drm/exynos
drivers/gpu/drm/gma500
drivers/gpu/drm/i2c
drivers/gpu/drm/nouveau
drivers/gpu/drm/omapdrm
drivers/gpu/drm/qxl
drivers/gpu/drm/rcar-du
drivers/gpu/drm/shmobile
drivers/gpu/drm/tilcdc
drivers/gpu/drm/ttm
drivers/gpu/drm/udl
drivers/gpu/drm/vmwgfx
david at david-mb ~/dev/kernel/linux $

So for x86 gma500,nouveau,qxl,udl,vmwgfx don't set DRIVER_USE_MTRR.
But I cannot tell whether they break if we call arch_phys_wc_add/del,
anyway. At least nouveau seemed to work here, but it doesn't use AGP
or drm_bufs, I guess.

Cheers
David

> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 33/39] drm: rip out drm_core_has_MTRR checks

2013-07-10 Thread Daniel Vetter

On Wed, Jul 10, 2013 at 3:51 PM, David Herrmann  
wrote:
>> -#if __OS_HAS_MTRR
>> -static inline int drm_core_has_MTRR(struct drm_device *dev)
>> -{
>> -   return drm_core_check_feature(dev, DRIVER_USE_MTRR);
>> -}
>> -#else
>> -#define drm_core_has_MTRR(dev) (0)
>> -#endif
>> -
>
> That was the last user of DRIVER_USE_MTRR (apart from drivers setting
> it in .driver_features). Any reason to keep it around?

Yeah, I guess we could rip things out. Which will also force me to
properly audit drivers for the eventual behaviour change this could
entail (in case there's an x86 driver which did not ask for an mtrr,
but iirc there isn't).
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 33/39] drm: rip out drm_core_has_MTRR checks

2013-07-10 Thread David Herrmann

Hi

On Wed, Jul 10, 2013 at 2:12 PM, Daniel Vetter  
wrote:
> The new arch_phys_wc_add/del functions do the right thing both with
> and without MTRR support in the kernel. So we can drop these
> additional checks.
>
> Cc: Andy Lutomirski 
> Signed-off-by: Daniel Vetter 
> ---
>  drivers/gpu/drm/drm_bufs.c | 13 +
>  drivers/gpu/drm/drm_pci.c  | 11 +--
>  drivers/gpu/drm/drm_stub.c |  2 +-
>  drivers/gpu/drm/drm_vm.c   |  3 +--
>  include/drm/drmP.h | 10 --
>  5 files changed, 12 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_bufs.c b/drivers/gpu/drm/drm_bufs.c
> index 5f73f0a..f63133b 100644
> --- a/drivers/gpu/drm/drm_bufs.c
> +++ b/drivers/gpu/drm/drm_bufs.c
> @@ -207,12 +207,10 @@ static int drm_addmap_core(struct drm_device * dev, 
> resource_size_t offset,
> return 0;
> }
>
> -   if (drm_core_has_MTRR(dev)) {
> -   if (map->type == _DRM_FRAME_BUFFER ||
> -   (map->flags & _DRM_WRITE_COMBINING)) {
> -   map->mtrr =
> -   arch_phys_wc_add(map->offset, 
> map->size);
> -   }
> +   if (map->type == _DRM_FRAME_BUFFER ||
> +   (map->flags & _DRM_WRITE_COMBINING)) {
> +   map->mtrr =
> +   arch_phys_wc_add(map->offset, map->size);
> }
> if (map->type == _DRM_REGISTERS) {
> if (map->flags & _DRM_WRITE_COMBINING)
> @@ -464,8 +462,7 @@ int drm_rmmap_locked(struct drm_device *dev, struct 
> drm_local_map *map)
> iounmap(map->handle);
> /* FALLTHROUGH */
> case _DRM_FRAME_BUFFER:
> -   if (drm_core_has_MTRR(dev))
> -   arch_phys_wc_del(map->mtrr);
> +   arch_phys_wc_del(map->mtrr);
> break;
> case _DRM_SHM:
> vfree(map->handle);
> diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c
> index 80c0b2b..9e93ea5 100644
> --- a/drivers/gpu/drm/drm_pci.c
> +++ b/drivers/gpu/drm/drm_pci.c
> @@ -276,12 +276,11 @@ static int drm_pci_agp_init(struct drm_device *dev)
> DRM_ERROR("Cannot initialize the agpgart module.\n");
> return -EINVAL;
> }
> -   if (drm_core_has_MTRR(dev)) {
> -   if (dev->agp)
> -   dev->agp->agp_mtrr = arch_phys_wc_add(
> -   dev->agp->agp_info.aper_base,
> -   dev->agp->agp_info.aper_size *
> -   1024 * 1024);
> +   if (dev->agp) {
> +   dev->agp->agp_mtrr = arch_phys_wc_add(
> +   dev->agp->agp_info.aper_base,
> +   dev->agp->agp_info.aper_size *
> +   1024 * 1024);
> }
> }
> return 0;
> diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
> index e7471ef..30cbd62 100644
> --- a/drivers/gpu/drm/drm_stub.c
> +++ b/drivers/gpu/drm/drm_stub.c
> @@ -445,7 +445,7 @@ void drm_put_dev(struct drm_device *dev)
>
> drm_lastclose(dev);
>
> -   if (drm_core_has_MTRR(dev) && drm_core_has_AGP(dev) && dev->agp)
> +   if (dev->agp)
> arch_phys_wc_del(dev->agp->agp_mtrr);
>
> if (dev->driver->unload)
> diff --git a/drivers/gpu/drm/drm_vm.c b/drivers/gpu/drm/drm_vm.c
> index feb2003..b5c5af7 100644
> --- a/drivers/gpu/drm/drm_vm.c
> +++ b/drivers/gpu/drm/drm_vm.c
> @@ -251,8 +251,7 @@ static void drm_vm_shm_close(struct vm_area_struct *vma)
> switch (map->type) {
> case _DRM_REGISTERS:
> case _DRM_FRAME_BUFFER:
> -   if (drm_core_has_MTRR(dev))
> -   arch_phys_wc_del(map->mtrr);
> +   arch_phys_wc_del(map->mtrr);
> iounmap(map->handle);
> break;
> case _DRM_SHM:
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index 62e9e41..d933f06 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -75,7 +75,6 @@
>  #include 
>
>  #define __OS_HAS_AGP (defined(CONFIG_AGP) || (defined(CONFIG_AGP_MODULE) && 
> defined(MODULE)))
> -#define __OS_HAS_MTRR (defined(CONFIG_MTRR))
>
>  struct module;
>
> @@ -1220,15 +1219,6 @@ static inline int drm_core_has_AGP(struct drm_device 
> *dev)
>  #define drm_core_has_AGP(dev) (0)
>  #endif
>
> -#if __OS_HAS_MTRR
> -static inline int drm_core_has_MTRR(struct drm_device *dev)
> -{
> -   return drm_core_check_feature(dev, DRIVER_USE_MTRR);
> -}
> -#else
> -#define drm_core_has_MTRR(dev) (0)
> -#endif
>

[PATCH 33/39] drm: rip out drm_core_has_MTRR checks

2013-07-10 Thread Daniel Vetter

The new arch_phys_wc_add/del functions do the right thing both with
and without MTRR support in the kernel. So we can drop these
additional checks.

Cc: Andy Lutomirski 
Signed-off-by: Daniel Vetter 
---
 drivers/gpu/drm/drm_bufs.c | 13 +
 drivers/gpu/drm/drm_pci.c  | 11 +--
 drivers/gpu/drm/drm_stub.c |  2 +-
 drivers/gpu/drm/drm_vm.c   |  3 +--
 include/drm/drmP.h | 10 --
 5 files changed, 12 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/drm_bufs.c b/drivers/gpu/drm/drm_bufs.c
index 5f73f0a..f63133b 100644
--- a/drivers/gpu/drm/drm_bufs.c
+++ b/drivers/gpu/drm/drm_bufs.c
@@ -207,12 +207,10 @@ static int drm_addmap_core(struct drm_device * dev, 
resource_size_t offset,
return 0;
}

-   if (drm_core_has_MTRR(dev)) {
-   if (map->type == _DRM_FRAME_BUFFER ||
-   (map->flags & _DRM_WRITE_COMBINING)) {
-   map->mtrr =
-   arch_phys_wc_add(map->offset, 
map->size);
-   }
+   if (map->type == _DRM_FRAME_BUFFER ||
+   (map->flags & _DRM_WRITE_COMBINING)) {
+   map->mtrr =
+   arch_phys_wc_add(map->offset, map->size);
}
if (map->type == _DRM_REGISTERS) {
if (map->flags & _DRM_WRITE_COMBINING)
@@ -464,8 +462,7 @@ int drm_rmmap_locked(struct drm_device *dev, struct 
drm_local_map *map)
iounmap(map->handle);
/* FALLTHROUGH */
case _DRM_FRAME_BUFFER:
-   if (drm_core_has_MTRR(dev))
-   arch_phys_wc_del(map->mtrr);
+   arch_phys_wc_del(map->mtrr);
break;
case _DRM_SHM:
vfree(map->handle);
diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c
index 80c0b2b..9e93ea5 100644
--- a/drivers/gpu/drm/drm_pci.c
+++ b/drivers/gpu/drm/drm_pci.c
@@ -276,12 +276,11 @@ static int drm_pci_agp_init(struct drm_device *dev)
DRM_ERROR("Cannot initialize the agpgart module.\n");
return -EINVAL;
}
-   if (drm_core_has_MTRR(dev)) {
-   if (dev->agp)
-   dev->agp->agp_mtrr = arch_phys_wc_add(
-   dev->agp->agp_info.aper_base,
-   dev->agp->agp_info.aper_size *
-   1024 * 1024);
+   if (dev->agp) {
+   dev->agp->agp_mtrr = arch_phys_wc_add(
+   dev->agp->agp_info.aper_base,
+   dev->agp->agp_info.aper_size *
+   1024 * 1024);
}
}
return 0;
diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
index e7471ef..30cbd62 100644
--- a/drivers/gpu/drm/drm_stub.c
+++ b/drivers/gpu/drm/drm_stub.c
@@ -445,7 +445,7 @@ void drm_put_dev(struct drm_device *dev)

drm_lastclose(dev);

-   if (drm_core_has_MTRR(dev) && drm_core_has_AGP(dev) && dev->agp)
+   if (dev->agp)
arch_phys_wc_del(dev->agp->agp_mtrr);

if (dev->driver->unload)
diff --git a/drivers/gpu/drm/drm_vm.c b/drivers/gpu/drm/drm_vm.c
index feb2003..b5c5af7 100644
--- a/drivers/gpu/drm/drm_vm.c
+++ b/drivers/gpu/drm/drm_vm.c
@@ -251,8 +251,7 @@ static void drm_vm_shm_close(struct vm_area_struct *vma)
switch (map->type) {
case _DRM_REGISTERS:
case _DRM_FRAME_BUFFER:
-   if (drm_core_has_MTRR(dev))
-   arch_phys_wc_del(map->mtrr);
+   arch_phys_wc_del(map->mtrr);
iounmap(map->handle);
break;
case _DRM_SHM:
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 62e9e41..d933f06 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -75,7 +75,6 @@
 #include 

 #define __OS_HAS_AGP (defined(CONFIG_AGP) || (defined(CONFIG_AGP_MODULE) && 
defined(MODULE)))
-#define __OS_HAS_MTRR (defined(CONFIG_MTRR))

 struct module;

@@ -1220,15 +1219,6 @@ static inline int drm_core_has_AGP(struct drm_device 
*dev)
 #define drm_core_has_AGP(dev) (0)
 #endif

-#if __OS_HAS_MTRR
-static inline int drm_core_has_MTRR(struct drm_device *dev)
-{
-   return drm_core_check_feature(dev, DRIVER_USE_MTRR);
-}
-#else
-#define drm_core_has_MTRR(dev) (0)
-#endif
-
 static inline void drm_device_set_unplugged(struct drm_device *dev)
 {
smp_wmb();
-- 
1.8.3.2

[PATCH 33/39] drm: rip out drm_core_has_MTRR checks

2013-07-10 Thread Andy Lutomirski

On Wed, Jul 10, 2013 at 8:59 AM, Daniel Vetter  
wrote:
> On Wed, Jul 10, 2013 at 5:41 PM, David Herrmann  
> wrote:
>> On Wed, Jul 10, 2013 at 5:22 PM, Daniel Vetter  
>> wrote:
>>> On Wed, Jul 10, 2013 at 3:51 PM, David Herrmann  
>>> wrote:
> -#if __OS_HAS_MTRR
> -static inline int drm_core_has_MTRR(struct drm_device *dev)
> -{
> -   return drm_core_check_feature(dev, DRIVER_USE_MTRR);
> -}
> -#else
> -#define drm_core_has_MTRR(dev) (0)
> -#endif
> -

 That was the last user of DRIVER_USE_MTRR (apart from drivers setting
 it in .driver_features). Any reason to keep it around?
>>>
>>> Yeah, I guess we could rip things out. Which will also force me to
>>> properly audit drivers for the eventual behaviour change this could
>>> entail (in case there's an x86 driver which did not ask for an mtrr,
>>> but iirc there isn't).
>>
>> david at david-mb ~/dev/kernel/linux $ for i in drivers/gpu/drm/* ; do if
>> test -d "$i" ; then if ! grep -q USE_MTRR -r $i ; then echo $i ; fi ;
>> fi ; done
>> drivers/gpu/drm/exynos
>> drivers/gpu/drm/gma500
>> drivers/gpu/drm/i2c
>> drivers/gpu/drm/nouveau
>> drivers/gpu/drm/omapdrm
>> drivers/gpu/drm/qxl
>> drivers/gpu/drm/rcar-du
>> drivers/gpu/drm/shmobile
>> drivers/gpu/drm/tilcdc
>> drivers/gpu/drm/ttm
>> drivers/gpu/drm/udl
>> drivers/gpu/drm/vmwgfx
>> david at david-mb ~/dev/kernel/linux $
>>
>> So for x86 gma500,nouveau,qxl,udl,vmwgfx don't set DRIVER_USE_MTRR.
>> But I cannot tell whether they break if we call arch_phys_wc_add/del,
>> anyway. At least nouveau seemed to work here, but it doesn't use AGP
>> or drm_bufs, I guess.
>
> Cool, thanks a lot for stitching together the list of drivers to look
> at. So for real KMS drivers it's the drives responsibility to add an
> mtrr if it needs one. nouvea, radeon, mgag200, i915 and vmwgfx do that
> already. Somehow the savage driver also ends up doing that, I have no
> idea why.
>
> Note that gma500 as a pure KMS driver doesn't need MTRR setup since
> the platforms that it supports all support PAT. So no MTRRs needed to
> get wc iomappings.
>
> The mtrr support in the drm core is all for legacy mappings of garts,
> framebuffers and registers. All legacy drivers set the USE_MTRR flag,
> so we're good there.
>

Are all of those codepaths really inaccessible in non-legacy drm
drivers?  I didn't try to fully unravel all the ioctls and such, but
it seems like userspace could add bufs and map them.  Since the mtrr
code isn't very robust (reference counting?  what reference
counting?), I'm a little bit worried that potentially enabling it in
more cases, which your patch does, could be harmful.

The arch_phys_wc stuff puts a prettier interface on the mtrr code and
turns it off when PAT is available, but the underlying code is still
just as bad.

--Andy

[PATCH 33/39] drm: rip out drm_core_has_MTRR checks

2013-07-10 Thread Daniel Vetter

The new arch_phys_wc_add/del functions do the right thing both with
and without MTRR support in the kernel. So we can drop these
additional checks.

Cc: Andy Lutomirski l...@amacapital.net
Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch
---
 drivers/gpu/drm/drm_bufs.c | 13 +
 drivers/gpu/drm/drm_pci.c  | 11 +--
 drivers/gpu/drm/drm_stub.c |  2 +-
 drivers/gpu/drm/drm_vm.c   |  3 +--
 include/drm/drmP.h | 10 --
 5 files changed, 12 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/drm_bufs.c b/drivers/gpu/drm/drm_bufs.c
index 5f73f0a..f63133b 100644
--- a/drivers/gpu/drm/drm_bufs.c
+++ b/drivers/gpu/drm/drm_bufs.c
@@ -207,12 +207,10 @@ static int drm_addmap_core(struct drm_device * dev, 
resource_size_t offset,
return 0;
}
 
-   if (drm_core_has_MTRR(dev)) {
-   if (map-type == _DRM_FRAME_BUFFER ||
-   (map-flags  _DRM_WRITE_COMBINING)) {
-   map-mtrr =
-   arch_phys_wc_add(map-offset, 
map-size);
-   }
+   if (map-type == _DRM_FRAME_BUFFER ||
+   (map-flags  _DRM_WRITE_COMBINING)) {
+   map-mtrr =
+   arch_phys_wc_add(map-offset, map-size);
}
if (map-type == _DRM_REGISTERS) {
if (map-flags  _DRM_WRITE_COMBINING)
@@ -464,8 +462,7 @@ int drm_rmmap_locked(struct drm_device *dev, struct 
drm_local_map *map)
iounmap(map-handle);
/* FALLTHROUGH */
case _DRM_FRAME_BUFFER:
-   if (drm_core_has_MTRR(dev))
-   arch_phys_wc_del(map-mtrr);
+   arch_phys_wc_del(map-mtrr);
break;
case _DRM_SHM:
vfree(map-handle);
diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c
index 80c0b2b..9e93ea5 100644
--- a/drivers/gpu/drm/drm_pci.c
+++ b/drivers/gpu/drm/drm_pci.c
@@ -276,12 +276,11 @@ static int drm_pci_agp_init(struct drm_device *dev)
DRM_ERROR(Cannot initialize the agpgart module.\n);
return -EINVAL;
}
-   if (drm_core_has_MTRR(dev)) {
-   if (dev-agp)
-   dev-agp-agp_mtrr = arch_phys_wc_add(
-   dev-agp-agp_info.aper_base,
-   dev-agp-agp_info.aper_size *
-   1024 * 1024);
+   if (dev-agp) {
+   dev-agp-agp_mtrr = arch_phys_wc_add(
+   dev-agp-agp_info.aper_base,
+   dev-agp-agp_info.aper_size *
+   1024 * 1024);
}
}
return 0;
diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
index e7471ef..30cbd62 100644
--- a/drivers/gpu/drm/drm_stub.c
+++ b/drivers/gpu/drm/drm_stub.c
@@ -445,7 +445,7 @@ void drm_put_dev(struct drm_device *dev)
 
drm_lastclose(dev);
 
-   if (drm_core_has_MTRR(dev)  drm_core_has_AGP(dev)  dev-agp)
+   if (dev-agp)
arch_phys_wc_del(dev-agp-agp_mtrr);
 
if (dev-driver-unload)
diff --git a/drivers/gpu/drm/drm_vm.c b/drivers/gpu/drm/drm_vm.c
index feb2003..b5c5af7 100644
--- a/drivers/gpu/drm/drm_vm.c
+++ b/drivers/gpu/drm/drm_vm.c
@@ -251,8 +251,7 @@ static void drm_vm_shm_close(struct vm_area_struct *vma)
switch (map-type) {
case _DRM_REGISTERS:
case _DRM_FRAME_BUFFER:
-   if (drm_core_has_MTRR(dev))
-   arch_phys_wc_del(map-mtrr);
+   arch_phys_wc_del(map-mtrr);
iounmap(map-handle);
break;
case _DRM_SHM:
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 62e9e41..d933f06 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -75,7 +75,6 @@
 #include linux/idr.h
 
 #define __OS_HAS_AGP (defined(CONFIG_AGP) || (defined(CONFIG_AGP_MODULE)  
defined(MODULE)))
-#define __OS_HAS_MTRR (defined(CONFIG_MTRR))
 
 struct module;
 
@@ -1220,15 +1219,6 @@ static inline int drm_core_has_AGP(struct drm_device 
*dev)
 #define drm_core_has_AGP(dev) (0)
 #endif
 
-#if __OS_HAS_MTRR
-static inline int drm_core_has_MTRR(struct drm_device *dev)
-{
-   return drm_core_check_feature(dev, DRIVER_USE_MTRR);
-}
-#else
-#define drm_core_has_MTRR(dev) (0)
-#endif
-
 static inline void drm_device_set_unplugged(struct drm_device *dev)
 {
smp_wmb();
-- 
1.8.3.2

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 33/39] drm: rip out drm_core_has_MTRR checks

2013-07-10 Thread David Herrmann

Hi

On Wed, Jul 10, 2013 at 2:12 PM, Daniel Vetter daniel.vet...@ffwll.ch wrote:
 The new arch_phys_wc_add/del functions do the right thing both with
 and without MTRR support in the kernel. So we can drop these
 additional checks.

 Cc: Andy Lutomirski l...@amacapital.net
 Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch
 ---
  drivers/gpu/drm/drm_bufs.c | 13 +
  drivers/gpu/drm/drm_pci.c  | 11 +--
  drivers/gpu/drm/drm_stub.c |  2 +-
  drivers/gpu/drm/drm_vm.c   |  3 +--
  include/drm/drmP.h | 10 --
  5 files changed, 12 insertions(+), 27 deletions(-)

 diff --git a/drivers/gpu/drm/drm_bufs.c b/drivers/gpu/drm/drm_bufs.c
 index 5f73f0a..f63133b 100644
 --- a/drivers/gpu/drm/drm_bufs.c
 +++ b/drivers/gpu/drm/drm_bufs.c
 @@ -207,12 +207,10 @@ static int drm_addmap_core(struct drm_device * dev, 
 resource_size_t offset,
 return 0;
 }

 -   if (drm_core_has_MTRR(dev)) {
 -   if (map-type == _DRM_FRAME_BUFFER ||
 -   (map-flags  _DRM_WRITE_COMBINING)) {
 -   map-mtrr =
 -   arch_phys_wc_add(map-offset, 
 map-size);
 -   }
 +   if (map-type == _DRM_FRAME_BUFFER ||
 +   (map-flags  _DRM_WRITE_COMBINING)) {
 +   map-mtrr =
 +   arch_phys_wc_add(map-offset, map-size);
 }
 if (map-type == _DRM_REGISTERS) {
 if (map-flags  _DRM_WRITE_COMBINING)
 @@ -464,8 +462,7 @@ int drm_rmmap_locked(struct drm_device *dev, struct 
 drm_local_map *map)
 iounmap(map-handle);
 /* FALLTHROUGH */
 case _DRM_FRAME_BUFFER:
 -   if (drm_core_has_MTRR(dev))
 -   arch_phys_wc_del(map-mtrr);
 +   arch_phys_wc_del(map-mtrr);
 break;
 case _DRM_SHM:
 vfree(map-handle);
 diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c
 index 80c0b2b..9e93ea5 100644
 --- a/drivers/gpu/drm/drm_pci.c
 +++ b/drivers/gpu/drm/drm_pci.c
 @@ -276,12 +276,11 @@ static int drm_pci_agp_init(struct drm_device *dev)
 DRM_ERROR(Cannot initialize the agpgart module.\n);
 return -EINVAL;
 }
 -   if (drm_core_has_MTRR(dev)) {
 -   if (dev-agp)
 -   dev-agp-agp_mtrr = arch_phys_wc_add(
 -   dev-agp-agp_info.aper_base,
 -   dev-agp-agp_info.aper_size *
 -   1024 * 1024);
 +   if (dev-agp) {
 +   dev-agp-agp_mtrr = arch_phys_wc_add(
 +   dev-agp-agp_info.aper_base,
 +   dev-agp-agp_info.aper_size *
 +   1024 * 1024);
 }
 }
 return 0;
 diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
 index e7471ef..30cbd62 100644
 --- a/drivers/gpu/drm/drm_stub.c
 +++ b/drivers/gpu/drm/drm_stub.c
 @@ -445,7 +445,7 @@ void drm_put_dev(struct drm_device *dev)

 drm_lastclose(dev);

 -   if (drm_core_has_MTRR(dev)  drm_core_has_AGP(dev)  dev-agp)
 +   if (dev-agp)
 arch_phys_wc_del(dev-agp-agp_mtrr);

 if (dev-driver-unload)
 diff --git a/drivers/gpu/drm/drm_vm.c b/drivers/gpu/drm/drm_vm.c
 index feb2003..b5c5af7 100644
 --- a/drivers/gpu/drm/drm_vm.c
 +++ b/drivers/gpu/drm/drm_vm.c
 @@ -251,8 +251,7 @@ static void drm_vm_shm_close(struct vm_area_struct *vma)
 switch (map-type) {
 case _DRM_REGISTERS:
 case _DRM_FRAME_BUFFER:
 -   if (drm_core_has_MTRR(dev))
 -   arch_phys_wc_del(map-mtrr);
 +   arch_phys_wc_del(map-mtrr);
 iounmap(map-handle);
 break;
 case _DRM_SHM:
 diff --git a/include/drm/drmP.h b/include/drm/drmP.h
 index 62e9e41..d933f06 100644
 --- a/include/drm/drmP.h
 +++ b/include/drm/drmP.h
 @@ -75,7 +75,6 @@
  #include linux/idr.h

  #define __OS_HAS_AGP (defined(CONFIG_AGP) || (defined(CONFIG_AGP_MODULE)  
 defined(MODULE)))
 -#define __OS_HAS_MTRR (defined(CONFIG_MTRR))

  struct module;

 @@ -1220,15 +1219,6 @@ static inline int drm_core_has_AGP(struct drm_device 
 *dev)
  #define drm_core_has_AGP(dev) (0)
  #endif

 -#if __OS_HAS_MTRR
 -static inline int drm_core_has_MTRR(struct drm_device *dev)
 -{
 -   return drm_core_check_feature(dev, DRIVER_USE_MTRR);
 -}
 -#else
 -#define drm_core_has_MTRR(dev) (0)
 -#endif
 -

That was the last user of DRIVER_USE_MTRR (apart from drivers setting
it in .driver_features). Any

Re: [PATCH 33/39] drm: rip out drm_core_has_MTRR checks

2013-07-10 Thread Daniel Vetter

On Wed, Jul 10, 2013 at 3:51 PM, David Herrmann dh.herrm...@gmail.com wrote:
 -#if __OS_HAS_MTRR
 -static inline int drm_core_has_MTRR(struct drm_device *dev)
 -{
 -   return drm_core_check_feature(dev, DRIVER_USE_MTRR);
 -}
 -#else
 -#define drm_core_has_MTRR(dev) (0)
 -#endif
 -

 That was the last user of DRIVER_USE_MTRR (apart from drivers setting
 it in .driver_features). Any reason to keep it around?

Yeah, I guess we could rip things out. Which will also force me to
properly audit drivers for the eventual behaviour change this could
entail (in case there's an x86 driver which did not ask for an mtrr,
but iirc there isn't).
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 33/39] drm: rip out drm_core_has_MTRR checks

2013-07-10 Thread David Herrmann

Hi

On Wed, Jul 10, 2013 at 5:22 PM, Daniel Vetter daniel.vet...@ffwll.ch wrote:
 On Wed, Jul 10, 2013 at 3:51 PM, David Herrmann dh.herrm...@gmail.com wrote:
 -#if __OS_HAS_MTRR
 -static inline int drm_core_has_MTRR(struct drm_device *dev)
 -{
 -   return drm_core_check_feature(dev, DRIVER_USE_MTRR);
 -}
 -#else
 -#define drm_core_has_MTRR(dev) (0)
 -#endif
 -

 That was the last user of DRIVER_USE_MTRR (apart from drivers setting
 it in .driver_features). Any reason to keep it around?

 Yeah, I guess we could rip things out. Which will also force me to
 properly audit drivers for the eventual behaviour change this could
 entail (in case there's an x86 driver which did not ask for an mtrr,
 but iirc there isn't).

david@david-mb ~/dev/kernel/linux $ for i in drivers/gpu/drm/* ; do if
test -d $i ; then if ! grep -q USE_MTRR -r $i ; then echo $i ; fi ;
fi ; done
drivers/gpu/drm/exynos
drivers/gpu/drm/gma500
drivers/gpu/drm/i2c
drivers/gpu/drm/nouveau
drivers/gpu/drm/omapdrm
drivers/gpu/drm/qxl
drivers/gpu/drm/rcar-du
drivers/gpu/drm/shmobile
drivers/gpu/drm/tilcdc
drivers/gpu/drm/ttm
drivers/gpu/drm/udl
drivers/gpu/drm/vmwgfx
david@david-mb ~/dev/kernel/linux $

So for x86 gma500,nouveau,qxl,udl,vmwgfx don't set DRIVER_USE_MTRR.
But I cannot tell whether they break if we call arch_phys_wc_add/del,
anyway. At least nouveau seemed to work here, but it doesn't use AGP
or drm_bufs, I guess.

Cheers
David

 -Daniel
 --
 Daniel Vetter
 Software Engineer, Intel Corporation
 +41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 33/39] drm: rip out drm_core_has_MTRR checks

2013-07-10 Thread Daniel Vetter

On Wed, Jul 10, 2013 at 5:41 PM, David Herrmann dh.herrm...@gmail.com wrote:
 On Wed, Jul 10, 2013 at 5:22 PM, Daniel Vetter daniel.vet...@ffwll.ch wrote:
 On Wed, Jul 10, 2013 at 3:51 PM, David Herrmann dh.herrm...@gmail.com 
 wrote:
 -#if __OS_HAS_MTRR
 -static inline int drm_core_has_MTRR(struct drm_device *dev)
 -{
 -   return drm_core_check_feature(dev, DRIVER_USE_MTRR);
 -}
 -#else
 -#define drm_core_has_MTRR(dev) (0)
 -#endif
 -

 That was the last user of DRIVER_USE_MTRR (apart from drivers setting
 it in .driver_features). Any reason to keep it around?

 Yeah, I guess we could rip things out. Which will also force me to
 properly audit drivers for the eventual behaviour change this could
 entail (in case there's an x86 driver which did not ask for an mtrr,
 but iirc there isn't).

 david@david-mb ~/dev/kernel/linux $ for i in drivers/gpu/drm/* ; do if
 test -d $i ; then if ! grep -q USE_MTRR -r $i ; then echo $i ; fi ;
 fi ; done
 drivers/gpu/drm/exynos
 drivers/gpu/drm/gma500
 drivers/gpu/drm/i2c
 drivers/gpu/drm/nouveau
 drivers/gpu/drm/omapdrm
 drivers/gpu/drm/qxl
 drivers/gpu/drm/rcar-du
 drivers/gpu/drm/shmobile
 drivers/gpu/drm/tilcdc
 drivers/gpu/drm/ttm
 drivers/gpu/drm/udl
 drivers/gpu/drm/vmwgfx
 david@david-mb ~/dev/kernel/linux $

 So for x86 gma500,nouveau,qxl,udl,vmwgfx don't set DRIVER_USE_MTRR.
 But I cannot tell whether they break if we call arch_phys_wc_add/del,
 anyway. At least nouveau seemed to work here, but it doesn't use AGP
 or drm_bufs, I guess.

Cool, thanks a lot for stitching together the list of drivers to look
at. So for real KMS drivers it's the drives responsibility to add an
mtrr if it needs one. nouvea, radeon, mgag200, i915 and vmwgfx do that
already. Somehow the savage driver also ends up doing that, I have no
idea why.

Note that gma500 as a pure KMS driver doesn't need MTRR setup since
the platforms that it supports all support PAT. So no MTRRs needed to
get wc iomappings.

The mtrr support in the drm core is all for legacy mappings of garts,
framebuffers and registers. All legacy drivers set the USE_MTRR flag,
so we're good there.

All in all I think we can really just ditch this. I'll update the patch.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 33/39] drm: rip out drm_core_has_MTRR checks

2013-07-10 Thread Daniel Vetter

On Wed, Jul 10, 2013 at 6:27 PM, Andy Lutomirski l...@amacapital.net wrote:
 Are all of those codepaths really inaccessible in non-legacy drm
 drivers?  I didn't try to fully unravel all the ioctls and such, but
 it seems like userspace could add bufs and map them.  Since the mtrr
 code isn't very robust (reference counting?  what reference
 counting?), I'm a little bit worried that potentially enabling it in
 more cases, which your patch does, could be harmful.

 The arch_phys_wc stuff puts a prettier interface on the mtrr code and
 turns it off when PAT is available, but the underlying code is still
 just as bad.

Well, the entire drm bufs stuff isn't refcounted and there are indeed
legacy driver that abused this in a completely unsafe way. E.g. for
i810.ko the ddx driver in userspace creates a register mapping through
the addbuf ioctl, which the kernel driver then uses. With no
refcounting at all to prevent an Oops (and I've seen them happen, you
simply need to kill X).

So I don't think this patch will make matters worse, especially since
most drivers set DRIVER_USE_MTRR. The way to fix this up is to
holesale block out these unsafe ioctls for kernel modesetting driver,
which this series here does for a lot of cases (still a bunch of them
left though). There's no way we can fix up the ums drm drivers without
breaking userspace :(

I haven't yet gotten around to blocking out the addmap ioctls since
reviewing existing userspace code will be a real pain. But at least
addmap is restrict to CAP_SYS_ADMIN, so not a that grave exploit
issue. But I very much plan to do that audit and then disable addmap
and friends for kms drivers.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 33/39] drm: rip out drm_core_has_MTRR checks

2013-07-10 Thread Andy Lutomirski

On Wed, Jul 10, 2013 at 8:59 AM, Daniel Vetter daniel.vet...@ffwll.ch wrote:
 On Wed, Jul 10, 2013 at 5:41 PM, David Herrmann dh.herrm...@gmail.com wrote:
 On Wed, Jul 10, 2013 at 5:22 PM, Daniel Vetter daniel.vet...@ffwll.ch 
 wrote:
 On Wed, Jul 10, 2013 at 3:51 PM, David Herrmann dh.herrm...@gmail.com 
 wrote:
 -#if __OS_HAS_MTRR
 -static inline int drm_core_has_MTRR(struct drm_device *dev)
 -{
 -   return drm_core_check_feature(dev, DRIVER_USE_MTRR);
 -}
 -#else
 -#define drm_core_has_MTRR(dev) (0)
 -#endif
 -

 That was the last user of DRIVER_USE_MTRR (apart from drivers setting
 it in .driver_features). Any reason to keep it around?

 Yeah, I guess we could rip things out. Which will also force me to
 properly audit drivers for the eventual behaviour change this could
 entail (in case there's an x86 driver which did not ask for an mtrr,
 but iirc there isn't).

 david@david-mb ~/dev/kernel/linux $ for i in drivers/gpu/drm/* ; do if
 test -d $i ; then if ! grep -q USE_MTRR -r $i ; then echo $i ; fi ;
 fi ; done
 drivers/gpu/drm/exynos
 drivers/gpu/drm/gma500
 drivers/gpu/drm/i2c
 drivers/gpu/drm/nouveau
 drivers/gpu/drm/omapdrm
 drivers/gpu/drm/qxl
 drivers/gpu/drm/rcar-du
 drivers/gpu/drm/shmobile
 drivers/gpu/drm/tilcdc
 drivers/gpu/drm/ttm
 drivers/gpu/drm/udl
 drivers/gpu/drm/vmwgfx
 david@david-mb ~/dev/kernel/linux $

 So for x86 gma500,nouveau,qxl,udl,vmwgfx don't set DRIVER_USE_MTRR.
 But I cannot tell whether they break if we call arch_phys_wc_add/del,
 anyway. At least nouveau seemed to work here, but it doesn't use AGP
 or drm_bufs, I guess.

 Cool, thanks a lot for stitching together the list of drivers to look
 at. So for real KMS drivers it's the drives responsibility to add an
 mtrr if it needs one. nouvea, radeon, mgag200, i915 and vmwgfx do that
 already. Somehow the savage driver also ends up doing that, I have no
 idea why.

 Note that gma500 as a pure KMS driver doesn't need MTRR setup since
 the platforms that it supports all support PAT. So no MTRRs needed to
 get wc iomappings.

 The mtrr support in the drm core is all for legacy mappings of garts,
 framebuffers and registers. All legacy drivers set the USE_MTRR flag,
 so we're good there.


Are all of those codepaths really inaccessible in non-legacy drm
drivers?  I didn't try to fully unravel all the ioctls and such, but
it seems like userspace could add bufs and map them.  Since the mtrr
code isn't very robust (reference counting?  what reference
counting?), I'm a little bit worried that potentially enabling it in
more cases, which your patch does, could be harmful.

The arch_phys_wc stuff puts a prettier interface on the mtrr code and
turns it off when PAT is available, but the underlying code is still
just as bad.

--Andy
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 33/39] drm: rip out drm_core_has_MTRR checks

[PATCH 33/39] drm: rip out drm_core_has_MTRR checks

[PATCH 33/39] drm: rip out drm_core_has_MTRR checks

[PATCH 33/39] drm: rip out drm_core_has_MTRR checks

[PATCH 33/39] drm: rip out drm_core_has_MTRR checks

[PATCH 33/39] drm: rip out drm_core_has_MTRR checks

[PATCH 33/39] drm: rip out drm_core_has_MTRR checks

[PATCH 33/39] drm: rip out drm_core_has_MTRR checks

Re: [PATCH 33/39] drm: rip out drm_core_has_MTRR checks

Re: [PATCH 33/39] drm: rip out drm_core_has_MTRR checks

Re: [PATCH 33/39] drm: rip out drm_core_has_MTRR checks

Re: [PATCH 33/39] drm: rip out drm_core_has_MTRR checks

Re: [PATCH 33/39] drm: rip out drm_core_has_MTRR checks

Re: [PATCH 33/39] drm: rip out drm_core_has_MTRR checks

14 matches

Site Navigation

Mail list logo

Footer information