| Message ID | 1373458333-5988-34-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi On Wed, Jul 10, 2013 at 2:12 PM, Daniel Vetter <daniel.vetter@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 <luto@amacapital.net> > Signed-off-by: Daniel Vetter <daniel.vetter@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 reason to keep it around? Cheers David > 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
On Wed, Jul 10, 2013 at 3:51 PM, David Herrmann <dh.herrmann@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
Hi On Wed, Jul 10, 2013 at 5:22 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > On Wed, Jul 10, 2013 at 3:51 PM, David Herrmann <dh.herrmann@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
On Wed, Jul 10, 2013 at 5:41 PM, David Herrmann <dh.herrmann@gmail.com> wrote: > On Wed, Jul 10, 2013 at 5:22 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: >> On Wed, Jul 10, 2013 at 3:51 PM, David Herrmann <dh.herrmann@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
On Wed, Jul 10, 2013 at 8:59 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > On Wed, Jul 10, 2013 at 5:41 PM, David Herrmann <dh.herrmann@gmail.com> wrote: >> On Wed, Jul 10, 2013 at 5:22 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: >>> On Wed, Jul 10, 2013 at 3:51 PM, David Herrmann <dh.herrmann@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
On Wed, Jul 10, 2013 at 6:27 PM, Andy Lutomirski <luto@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
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();
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 <luto@amacapital.net> Signed-off-by: Daniel Vetter <daniel.vetter@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(-)