Breakage in "track dev_mapping in more robust and flexible way"

[Thomas Hellstrom]

On 10/26/2012 03:14 PM, Ilija Hadzic wrote:
>
>
> On Fri, 26 Oct 2012, Thomas Hellstrom wrote:
>
>> Hi,
>>
>> On 10/25/2012 11:27 PM, Ilija Hadzic wrote:
>>>
>>> Can you give the attached patch a whirl and let me know if it fixes 
>>> the problem?
>>>
>>> As I indicated in my previous note, vmwgfx should be the only 
>>> affected driver because it looks at dev_mapping in the open hook 
>>> (others do it when they create an object, so they are not affected).
>>>
>>> I'll probably revise it (and I'll have some general questions about 
>>> drm_open syscall) before officially send the patch, but I wanted to 
>>> get something quickly to you to check if it fixes your problem. I 
>>> hope that your vmwgfx test environment is such that you can 
>>> reproduce the original
>>> problem.
>>>
>>> thanks,
>>>
>>> -- Ilija
>>
>> Yes, it appears like this patch fixes the problem. It'd be good to 
>> have it in 3.7 (drm-fixes) with a cc to stable.
>>
>
> OK great. Thanks for testing. Before I send out an "official" patch, I 
> have a few questions for those who have been around longer and can 
> possibly reflect better than me on the history of drm_open syscall.
>
> Currently, before touching dev->dev_mapping field we grab dev->struct 
> mutex. This has been introduced by Dave Airlie a long time ago in 
> a2c0a97b784f837300f7b0869c82ab712c600952. I tried to preserve that in 
> all patches where I touched dev_open, but looking at the code I don't 
> think the mutex is necessary. Namely, drm_open is only set in 
> drm_open, and concurrent openers are protected with drm_global_mutex. 
> Other places (drivers) where dev->dev_mapping is accessed is read-only 
> and dev_mapping is written at first open when there are no file 
> descriptors around to issue any other call. Also, it doesn't look to 
> me that any driver locks dev->struct_mutex before accessing 
> dev->dev_mapping anyway. So I am thinking of dropping the mutex 
> completely, but I would like to hear a second thought.

Without having looked a the code, with your current changes 
dev->dev_mapping should be immutable and initialized before any 
consumers reference
it, and as such would need no mutex, so dropping the protection of 
dev->dev_mapping from that point of view should be fine. I think people 
sooner or later want to get rid of drm_global_mutex, though, but at that 
point we probably want another mutex that protects open-time 
initialization of immutable members only, so from my point of view this 
is OK, but you might want to double-check with Dave.


>
> The other issue, I noticed is that of the drm_setup() call fails, the 
> open_count counter would remain incremented and I think we need to 
> restore it back (or if I am missing something, would someone please 
> enlighten me). This was also in the kernel all this time (and I have 
> not noticed until now), so I "smuggled" that fix in the patch that I 
> sent you. However, wonder if I should cut the separate patch for 
> open_count fix.



>
> Actually, I think that I should cut three patches: one to drop the 
> mutex, one to fix the open_count and one to fix your problem with 
> dev_mapping and that probably all three should CC stable. Before I do 
> that, I'd like to hear opinions of others.

I think you should, However stable doesn't want fixes for theoretical 
stuff that have never been triggered in real life, so the patch to drop 
mutex protection doesn't belong there. That's a patch for drm-next, so 
people get a decent chance to see if it breaks something. The 
dev_mapping thing opens up a quite severe security issue and should got 
into drm-fixes with Cc to stable as soon as ever possible. The 
open_count stuff should go into drm-fixes, possibly cc'd to stable.

Thanks,
Thomas


>
> thanks,
>
> Ilija
>
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Breakage in "track dev_mapping in more robust and flexible way"

[Thomas Hellstrom]

On 10/26/2012 03:14 PM, Ilija Hadzic wrote:



On Fri, 26 Oct 2012, Thomas Hellstrom wrote:


Hi,

On 10/25/2012 11:27 PM, Ilija Hadzic wrote:


Can you give the attached patch a whirl and let me know if it fixes 
the problem?


As I indicated in my previous note, vmwgfx should be the only 
affected driver because it looks at dev_mapping in the open hook 
(others do it when they create an object, so they are not affected).


I'll probably revise it (and I'll have some general questions about 
drm_open syscall) before officially send the patch, but I wanted to 
get something quickly to you to check if it fixes your problem. I 
hope that your vmwgfx test environment is such that you can 
reproduce the original

problem.

thanks,

-- Ilija


Yes, it appears like this patch fixes the problem. It'd be good to 
have it in 3.7 (drm-fixes) with a cc to stable.




OK great. Thanks for testing. Before I send out an "official" patch, I 
have a few questions for those who have been around longer and can 
possibly reflect better than me on the history of drm_open syscall.


Currently, before touching dev->dev_mapping field we grab dev->struct 
mutex. This has been introduced by Dave Airlie a long time ago in 
a2c0a97b784f837300f7b0869c82ab712c600952. I tried to preserve that in 
all patches where I touched dev_open, but looking at the code I don't 
think the mutex is necessary. Namely, drm_open is only set in 
drm_open, and concurrent openers are protected with drm_global_mutex. 
Other places (drivers) where dev->dev_mapping is accessed is read-only 
and dev_mapping is written at first open when there are no file 
descriptors around to issue any other call. Also, it doesn't look to 
me that any driver locks dev->struct_mutex before accessing 
dev->dev_mapping anyway. So I am thinking of dropping the mutex 
completely, but I would like to hear a second thought.


Without having looked a the code, with your current changes 
dev->dev_mapping should be immutable and initialized before any 
consumers reference
it, and as such would need no mutex, so dropping the protection of 
dev->dev_mapping from that point of view should be fine. I think people 
sooner or later want to get rid of drm_global_mutex, though, but at that 
point we probably want another mutex that protects open-time 
initialization of immutable members only, so from my point of view this 
is OK, but you might want to double-check with Dave.





The other issue, I noticed is that of the drm_setup() call fails, the 
open_count counter would remain incremented and I think we need to 
restore it back (or if I am missing something, would someone please 
enlighten me). This was also in the kernel all this time (and I have 
not noticed until now), so I "smuggled" that fix in the patch that I 
sent you. However, wonder if I should cut the separate patch for 
open_count fix.






Actually, I think that I should cut three patches: one to drop the 
mutex, one to fix the open_count and one to fix your problem with 
dev_mapping and that probably all three should CC stable. Before I do 
that, I'd like to hear opinions of others.


I think you should, However stable doesn't want fixes for theoretical 
stuff that have never been triggered in real life, so the patch to drop 
mutex protection doesn't belong there. That's a patch for drm-next, so 
people get a decent chance to see if it breaks something. The 
dev_mapping thing opens up a quite severe security issue and should got 
into drm-fixes with Cc to stable as soon as ever possible. The 
open_count stuff should go into drm-fixes, possibly cc'd to stable.


Thanks,
Thomas




thanks,

Ilija

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




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

Breakage in "track dev_mapping in more robust and flexible way"

[Thomas Hellstrom]

Hi,

On 10/25/2012 11:27 PM, Ilija Hadzic wrote:
>
> Can you give the attached patch a whirl and let me know if it fixes 
> the problem?
>
> As I indicated in my previous note, vmwgfx should be the only affected 
> driver because it looks at dev_mapping in the open hook (others do it 
> when they create an object, so they are not affected).
>
> I'll probably revise it (and I'll have some general questions about 
> drm_open syscall) before officially send the patch, but I wanted to 
> get something quickly to you to check if it fixes your problem. I hope 
> that your vmwgfx test environment is such that you can reproduce the 
> original
> problem.
>
> thanks,
>
> -- Ilija

Yes, it appears like this patch fixes the problem. It'd be good to have 
it in 3.7 (drm-fixes) with a cc to stable.

/Thomas

Breakage in "track dev_mapping in more robust and flexible way"

[Ilija Hadzic]

On Fri, 26 Oct 2012, Thomas Hellstrom wrote:

> Hi,
>
> On 10/25/2012 11:27 PM, Ilija Hadzic wrote:
>> 
>> Can you give the attached patch a whirl and let me know if it fixes the 
>> problem?
>> 
>> As I indicated in my previous note, vmwgfx should be the only affected 
>> driver because it looks at dev_mapping in the open hook (others do it when 
>> they create an object, so they are not affected).
>> 
>> I'll probably revise it (and I'll have some general questions about 
>> drm_open syscall) before officially send the patch, but I wanted to get 
>> something quickly to you to check if it fixes your problem. I hope that 
>> your vmwgfx test environment is such that you can reproduce the original
>> problem.
>> 
>> thanks,
>> 
>> -- Ilija
>
> Yes, it appears like this patch fixes the problem. It'd be good to have it in 
> 3.7 (drm-fixes) with a cc to stable.
>

OK great. Thanks for testing. Before I send out an "official" patch, I 
have a few questions for those who have been around longer and can 
possibly reflect better than me on the history of drm_open syscall.

Currently, before touching dev->dev_mapping field we grab dev->struct 
mutex. This has been introduced by Dave Airlie a long time ago in 
a2c0a97b784f837300f7b0869c82ab712c600952. I tried to preserve that in all 
patches where I touched dev_open, but looking at the code I don't think 
the mutex is necessary. Namely, drm_open is only set in drm_open, and 
concurrent openers are protected with drm_global_mutex. Other places 
(drivers) where dev->dev_mapping is accessed is read-only and dev_mapping 
is written at first open when there are no file descriptors around to 
issue any other call. Also, it doesn't look to me that any driver locks 
dev->struct_mutex before accessing dev->dev_mapping anyway. So I am 
thinking of dropping the mutex completely, but I would like to hear a 
second thought.

The other issue, I noticed is that of the drm_setup() call fails, the 
open_count counter would remain incremented and I think we need to restore 
it back (or if I am missing something, would someone please enlighten me). 
This was also in the kernel all this time (and I have not noticed until 
now), so I "smuggled" that fix in the patch that I sent you. However, 
wonder if I should cut the separate patch for open_count fix.

Actually, I think that I should cut three patches: one to drop the mutex, 
one to fix the open_count and one to fix your problem with dev_mapping and 
that probably all three should CC stable. Before I do that, I'd like to 
hear opinions of others.

thanks,

Ilija

Re: Breakage in "track dev_mapping in more robust and flexible way"

[Ilija Hadzic]

On Fri, 26 Oct 2012, Thomas Hellstrom wrote:


Hi,

On 10/25/2012 11:27 PM, Ilija Hadzic wrote:


Can you give the attached patch a whirl and let me know if it fixes the 
problem?


As I indicated in my previous note, vmwgfx should be the only affected 
driver because it looks at dev_mapping in the open hook (others do it when 
they create an object, so they are not affected).


I'll probably revise it (and I'll have some general questions about 
drm_open syscall) before officially send the patch, but I wanted to get 
something quickly to you to check if it fixes your problem. I hope that 
your vmwgfx test environment is such that you can reproduce the original

problem.

thanks,

-- Ilija


Yes, it appears like this patch fixes the problem. It'd be good to have it in 
3.7 (drm-fixes) with a cc to stable.




OK great. Thanks for testing. Before I send out an "official" patch, I 
have a few questions for those who have been around longer and can 
possibly reflect better than me on the history of drm_open syscall.


Currently, before touching dev->dev_mapping field we grab dev->struct 
mutex. This has been introduced by Dave Airlie a long time ago in 
a2c0a97b784f837300f7b0869c82ab712c600952. I tried to preserve that in all 
patches where I touched dev_open, but looking at the code I don't think 
the mutex is necessary. Namely, drm_open is only set in drm_open, and 
concurrent openers are protected with drm_global_mutex. Other places 
(drivers) where dev->dev_mapping is accessed is read-only and dev_mapping 
is written at first open when there are no file descriptors around to 
issue any other call. Also, it doesn't look to me that any driver locks 
dev->struct_mutex before accessing dev->dev_mapping anyway. So I am 
thinking of dropping the mutex completely, but I would like to hear a 
second thought.


The other issue, I noticed is that of the drm_setup() call fails, the 
open_count counter would remain incremented and I think we need to restore 
it back (or if I am missing something, would someone please enlighten me). 
This was also in the kernel all this time (and I have not noticed until 
now), so I "smuggled" that fix in the patch that I sent you. However, 
wonder if I should cut the separate patch for open_count fix.


Actually, I think that I should cut three patches: one to drop the mutex, 
one to fix the open_count and one to fix your problem with dev_mapping and 
that probably all three should CC stable. Before I do that, I'd like to 
hear opinions of others.


thanks,

Ilija

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

Re: Breakage in "track dev_mapping in more robust and flexible way"

[Thomas Hellstrom]

Hi,

On 10/25/2012 11:27 PM, Ilija Hadzic wrote:


Can you give the attached patch a whirl and let me know if it fixes 
the problem?


As I indicated in my previous note, vmwgfx should be the only affected 
driver because it looks at dev_mapping in the open hook (others do it 
when they create an object, so they are not affected).


I'll probably revise it (and I'll have some general questions about 
drm_open syscall) before officially send the patch, but I wanted to 
get something quickly to you to check if it fixes your problem. I hope 
that your vmwgfx test environment is such that you can reproduce the 
original

problem.

thanks,

-- Ilija


Yes, it appears like this patch fixes the problem. It'd be good to have 
it in 3.7 (drm-fixes) with a cc to stable.


/Thomas

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

Breakage in "track dev_mapping in more robust and flexible way"

[Thomas Hellström]

On 10/25/12 7:12 PM, Ilija Hadzic wrote:
> On Thu, Oct 25, 2012 at 11:10 AM, Thomas Hellstr?m
>  wrote:
>> On 10/25/12 4:41 PM, Jerome Glisse wrote:
>>> On Thu, Oct 25, 2012 at 04:02:25PM +0200, Thomas Hellstrom wrote:
 Hi,

 This commit

   From 949c4a34afacfe800fc442afac117aba15284962 Mon Sep 17 00:00:00 2001
 From: Ilija Hadzic 
 Date: Tue, 15 May 2012 16:40:10 -0400
 Subject: [PATCH] drm: track dev_mapping in more robust and flexible way

 Setting dev_mapping (pointer to the address_space structure
 used for memory mappings) to the address_space of the first
 opener's inode and then failing if other openers come in
 through a different inode has a few restrictions that are
 eliminated by this patch.

 If we already have valid dev_mapping and we spot an opener
 with different i_node, we force its i_mapping pointer to the
 already established address_space structure (first opener's
 inode). This will make all mappings from drm device hang off
 the same address_space object.
 ...

 Breaks drivers using TTM, since when the X server calls into the
 driver open, drm's dev_mapping has not
 yet been setup. The setup needs to be moved before the driver's open
 hook is called.

 Typically, if a TTM-aware driver is provoked by the Xorg server to
 move a buffer from system to VRAM or AGP,
 before any other drm client is started, The user-space page table
 entries are not killed before the move, and left pointing
 into freed pages, causing system crashes and / or user-space access
 to arbitrary memory.
>>> Doesn't handle move invalidate the drm file mapping before scheduling
>>> the move ?
>> Yes, but to do that it needs a correct value of bdev::dev_mapping, which is
>> now incorrectly set on the
>> *second* open instead of the first open.
>>
> So you are implying that in the first open the assignment of dev->dev_mapping 
> is
> somehow skipped (which could happen if drm_setup returns an error) or that the
> driver on which you are having problems with (nouveau I presume) needs
> dev_mapping
> in the firstopen hook ?
No. On open, driver::open is called from drm::open. It copies the value 
of dev->dev_mapping, however,
driver::open is called *before* dev->dev_mapping is set up, so what I'm 
saying is that the setup of
dev->dev_mapping must be moved to before driver::open is called from 
drm::open

(this was hit while testing vmwgfx with new code, BTW. It will be hard, 
but probably possible to trigger
from unpriviliged user-space with the current vmwgfx code.

Thanks,
Thomas

Breakage in "track dev_mapping in more robust and flexible way"

[Thomas Hellström]

On 10/25/12 4:41 PM, Jerome Glisse wrote:
> On Thu, Oct 25, 2012 at 04:02:25PM +0200, Thomas Hellstrom wrote:
>> Hi,
>>
>> This commit
>>
>>  From 949c4a34afacfe800fc442afac117aba15284962 Mon Sep 17 00:00:00 2001
>> From: Ilija Hadzic 
>> Date: Tue, 15 May 2012 16:40:10 -0400
>> Subject: [PATCH] drm: track dev_mapping in more robust and flexible way
>>
>> Setting dev_mapping (pointer to the address_space structure
>> used for memory mappings) to the address_space of the first
>> opener's inode and then failing if other openers come in
>> through a different inode has a few restrictions that are
>> eliminated by this patch.
>>
>> If we already have valid dev_mapping and we spot an opener
>> with different i_node, we force its i_mapping pointer to the
>> already established address_space structure (first opener's
>> inode). This will make all mappings from drm device hang off
>> the same address_space object.
>> ...
>>
>> Breaks drivers using TTM, since when the X server calls into the
>> driver open, drm's dev_mapping has not
>> yet been setup. The setup needs to be moved before the driver's open
>> hook is called.
>>
>> Typically, if a TTM-aware driver is provoked by the Xorg server to
>> move a buffer from system to VRAM or AGP,
>> before any other drm client is started, The user-space page table
>> entries are not killed before the move, and left pointing
>> into freed pages, causing system crashes and / or user-space access
>> to arbitrary memory.
> Doesn't handle move invalidate the drm file mapping before scheduling
> the move ?
Yes, but to do that it needs a correct value of bdev::dev_mapping, which 
is now incorrectly set on the
*second* open instead of the first open.

/Thomas


> Cheers,
> Jerome

Breakage in "track dev_mapping in more robust and flexible way"

[Ilija Hadzic]

Can you give the attached patch a whirl and let me know if it fixes the 
problem?

As I indicated in my previous note, vmwgfx should be the only affected 
driver because it looks at dev_mapping in the open hook (others do it when 
they create an object, so they are not affected).

I'll probably revise it (and I'll have some general questions about 
drm_open syscall) before officially send the patch, but I wanted to get 
something quickly to you to check if it fixes your problem. I hope that 
your vmwgfx test environment is such that you can reproduce the original
problem.

thanks,

-- Ilija
-- next part --

Breakage in "track dev_mapping in more robust and flexible way"

[Thomas Hellstrom]

Hi,

This commit

 From 949c4a34afacfe800fc442afac117aba15284962 Mon Sep 17 00:00:00 2001
From: Ilija Hadzic 
Date: Tue, 15 May 2012 16:40:10 -0400
Subject: [PATCH] drm: track dev_mapping in more robust and flexible way

Setting dev_mapping (pointer to the address_space structure
used for memory mappings) to the address_space of the first
opener's inode and then failing if other openers come in
through a different inode has a few restrictions that are
eliminated by this patch.

If we already have valid dev_mapping and we spot an opener
with different i_node, we force its i_mapping pointer to the
already established address_space structure (first opener's
inode). This will make all mappings from drm device hang off
the same address_space object.
...

Breaks drivers using TTM, since when the X server calls into the driver 
open, drm's dev_mapping has not
yet been setup. The setup needs to be moved before the driver's open 
hook is called.

Typically, if a TTM-aware driver is provoked by the Xorg server to move 
a buffer from system to VRAM or AGP,
before any other drm client is started, The user-space page table 
entries are not killed before the move, and left pointing
into freed pages, causing system crashes and / or user-space access to 
arbitrary memory.

/Thomas

Re: Breakage in "track dev_mapping in more robust and flexible way"

[Ilija Hadzic]

Can you give the attached patch a whirl and let me know if it fixes the 
problem?


As I indicated in my previous note, vmwgfx should be the only affected 
driver because it looks at dev_mapping in the open hook (others do it when 
they create an object, so they are not affected).


I'll probably revise it (and I'll have some general questions about 
drm_open syscall) before officially send the patch, but I wanted to get 
something quickly to you to check if it fixes your problem. I hope that 
your vmwgfx test environment is such that you can reproduce the original

problem.

thanks,

-- Ilija
From 18a489e7415f495c7ba48cc61733d6c7d8f3fd68 Mon Sep 17 00:00:00 2001
From: Ilija Hadzic 
Date: Thu, 25 Oct 2012 15:28:05 -0400
Subject: [PATCH] drm: set dev_mapping before calling drm_open_helper

Some drivers (specifically vmwgfx) look at dev_mapping
in their open hook, so we have to set dev->dev_mapping
earlier in the process.

Signed-off-by: Ilija Hadzic 
---
 drivers/gpu/drm/drm_fops.c |   43 +--
 1 files changed, 29 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
index 7ef1b67..50b7b47 100644
--- a/drivers/gpu/drm/drm_fops.c
+++ b/drivers/gpu/drm/drm_fops.c
@@ -121,6 +121,8 @@ int drm_open(struct inode *inode, struct file *filp)
int minor_id = iminor(inode);
struct drm_minor *minor;
int retcode = 0;
+   int need_setup = 0;
+   struct address_space *old_mapping;
 
minor = idr_find(&drm_minors_idr, minor_id);
if (!minor)
@@ -132,23 +134,36 @@ int drm_open(struct inode *inode, struct file *filp)
if (drm_device_is_unplugged(dev))
return -ENODEV;
 
+   if (!dev->open_count++)
+   need_setup = 1;
+   mutex_lock(&dev->struct_mutex);
+   old_mapping = dev->dev_mapping;
+   if (old_mapping == NULL)
+   dev->dev_mapping = &inode->i_data;
+   mutex_unlock(&dev->struct_mutex);
+
retcode = drm_open_helper(inode, filp, dev);
-   if (!retcode) {
-   atomic_inc(&dev->counts[_DRM_STAT_OPENS]);
-   if (!dev->open_count++)
-   retcode = drm_setup(dev);
-   }
-   if (!retcode) {
-   mutex_lock(&dev->struct_mutex);
-   if (dev->dev_mapping == NULL)
-   dev->dev_mapping = &inode->i_data;
-   /* ihold ensures nobody can remove inode with our i_data */
-   ihold(container_of(dev->dev_mapping, struct inode, i_data));
-   inode->i_mapping = dev->dev_mapping;
-   filp->f_mapping = dev->dev_mapping;
-   mutex_unlock(&dev->struct_mutex);
+   if (retcode)
+   goto err_undo;
+   atomic_inc(&dev->counts[_DRM_STAT_OPENS]);
+   if (need_setup) {
+   retcode = drm_setup(dev);
+   if (retcode)
+   goto err_undo;
}
+   /* ihold ensures nobody can remove inode with our i_data */
+   mutex_lock(&dev->struct_mutex);
+   ihold(container_of(dev->dev_mapping, struct inode, i_data));
+   inode->i_mapping = dev->dev_mapping;
+   filp->f_mapping = dev->dev_mapping;
+   mutex_unlock(&dev->struct_mutex);
+   return 0;
 
+err_undo:
+   dev->open_count--;
+   mutex_lock(&dev->struct_mutex);
+   dev->dev_mapping = old_mapping;
+   mutex_unlock(&dev->struct_mutex);
return retcode;
 }
 EXPORT_SYMBOL(drm_open);
-- 
1.7.4.1

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

Breakage in "track dev_mapping in more robust and flexible way"

[Ilija Hadzic]

On Thu, Oct 25, 2012 at 11:10 AM, Thomas Hellstr?m
 wrote:
> On 10/25/12 4:41 PM, Jerome Glisse wrote:
>>
>> On Thu, Oct 25, 2012 at 04:02:25PM +0200, Thomas Hellstrom wrote:
>>>
>>> Hi,
>>>
>>> This commit
>>>
>>>  From 949c4a34afacfe800fc442afac117aba15284962 Mon Sep 17 00:00:00 2001
>>> From: Ilija Hadzic 
>>> Date: Tue, 15 May 2012 16:40:10 -0400
>>> Subject: [PATCH] drm: track dev_mapping in more robust and flexible way
>>>
>>> Setting dev_mapping (pointer to the address_space structure
>>> used for memory mappings) to the address_space of the first
>>> opener's inode and then failing if other openers come in
>>> through a different inode has a few restrictions that are
>>> eliminated by this patch.
>>>
>>> If we already have valid dev_mapping and we spot an opener
>>> with different i_node, we force its i_mapping pointer to the
>>> already established address_space structure (first opener's
>>> inode). This will make all mappings from drm device hang off
>>> the same address_space object.
>>> ...
>>>
>>> Breaks drivers using TTM, since when the X server calls into the
>>> driver open, drm's dev_mapping has not
>>> yet been setup. The setup needs to be moved before the driver's open
>>> hook is called.
>>>
>>> Typically, if a TTM-aware driver is provoked by the Xorg server to
>>> move a buffer from system to VRAM or AGP,
>>> before any other drm client is started, The user-space page table
>>> entries are not killed before the move, and left pointing
>>> into freed pages, causing system crashes and / or user-space access
>>> to arbitrary memory.
>>
>> Doesn't handle move invalidate the drm file mapping before scheduling
>> the move ?
>
> Yes, but to do that it needs a correct value of bdev::dev_mapping, which is
> now incorrectly set on the
> *second* open instead of the first open.
>

So you are implying that in the first open the assignment of dev->dev_mapping is
somehow skipped (which could happen if drm_setup returns an error) or that the
driver on which you are having problems with (nouveau I presume) needs
dev_mapping
in the firstopen hook ?

It's been a while since I did it, but if my memory serves me well I
thought I explicitly
verified that dev_mapping was correctly set in the first open (though
GPUs I use are
primarily AMD).

-- Ilija


> /Thomas
>
>
>
>> Cheers,
>> Jerome
>
>
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Breakage in "track dev_mapping in more robust and flexible way"

[Ilija Hadzic]

Or could the driver that causes the problem be vmwgfx ? I now looked at 
the code and I notice that indeed vmwgfx sets its private dev_mapping in 
the open hook, while all other TTM-based drivers (radeon and nouveau) do 
it when they create an object.

-- Ilija

Re: Breakage in "track dev_mapping in more robust and flexible way"

[Thomas Hellström]

On 10/25/12 7:12 PM, Ilija Hadzic wrote:

On Thu, Oct 25, 2012 at 11:10 AM, Thomas Hellström
 wrote:

On 10/25/12 4:41 PM, Jerome Glisse wrote:

On Thu, Oct 25, 2012 at 04:02:25PM +0200, Thomas Hellstrom wrote:

Hi,

This commit

  From 949c4a34afacfe800fc442afac117aba15284962 Mon Sep 17 00:00:00 2001
From: Ilija Hadzic 
Date: Tue, 15 May 2012 16:40:10 -0400
Subject: [PATCH] drm: track dev_mapping in more robust and flexible way

Setting dev_mapping (pointer to the address_space structure
used for memory mappings) to the address_space of the first
opener's inode and then failing if other openers come in
through a different inode has a few restrictions that are
eliminated by this patch.

If we already have valid dev_mapping and we spot an opener
with different i_node, we force its i_mapping pointer to the
already established address_space structure (first opener's
inode). This will make all mappings from drm device hang off
the same address_space object.
...

Breaks drivers using TTM, since when the X server calls into the
driver open, drm's dev_mapping has not
yet been setup. The setup needs to be moved before the driver's open
hook is called.

Typically, if a TTM-aware driver is provoked by the Xorg server to
move a buffer from system to VRAM or AGP,
before any other drm client is started, The user-space page table
entries are not killed before the move, and left pointing
into freed pages, causing system crashes and / or user-space access
to arbitrary memory.

Doesn't handle move invalidate the drm file mapping before scheduling
the move ?

Yes, but to do that it needs a correct value of bdev::dev_mapping, which is
now incorrectly set on the
*second* open instead of the first open.


So you are implying that in the first open the assignment of dev->dev_mapping is
somehow skipped (which could happen if drm_setup returns an error) or that the
driver on which you are having problems with (nouveau I presume) needs
dev_mapping
in the firstopen hook ?
No. On open, driver::open is called from drm::open. It copies the value 
of dev->dev_mapping, however,
driver::open is called *before* dev->dev_mapping is set up, so what I'm 
saying is that the setup of
dev->dev_mapping must be moved to before driver::open is called from 
drm::open


(this was hit while testing vmwgfx with new code, BTW. It will be hard, 
but probably possible to trigger

from unpriviliged user-space with the current vmwgfx code.

Thanks,
Thomas

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

Breakage in "track dev_mapping in more robust and flexible way"

[Jerome Glisse]

On Thu, Oct 25, 2012 at 04:02:25PM +0200, Thomas Hellstrom wrote:
> Hi,
> 
> This commit
> 
> From 949c4a34afacfe800fc442afac117aba15284962 Mon Sep 17 00:00:00 2001
> From: Ilija Hadzic 
> Date: Tue, 15 May 2012 16:40:10 -0400
> Subject: [PATCH] drm: track dev_mapping in more robust and flexible way
> 
> Setting dev_mapping (pointer to the address_space structure
> used for memory mappings) to the address_space of the first
> opener's inode and then failing if other openers come in
> through a different inode has a few restrictions that are
> eliminated by this patch.
> 
> If we already have valid dev_mapping and we spot an opener
> with different i_node, we force its i_mapping pointer to the
> already established address_space structure (first opener's
> inode). This will make all mappings from drm device hang off
> the same address_space object.
> ...
> 
> Breaks drivers using TTM, since when the X server calls into the
> driver open, drm's dev_mapping has not
> yet been setup. The setup needs to be moved before the driver's open
> hook is called.
> 
> Typically, if a TTM-aware driver is provoked by the Xorg server to
> move a buffer from system to VRAM or AGP,
> before any other drm client is started, The user-space page table
> entries are not killed before the move, and left pointing
> into freed pages, causing system crashes and / or user-space access
> to arbitrary memory.

Doesn't handle move invalidate the drm file mapping before scheduling
the move ?

Cheers,
Jerome

Re: Breakage in "track dev_mapping in more robust and flexible way"

[Ilija Hadzic]

Or could the driver that causes the problem be vmwgfx ? I now looked at 
the code and I notice that indeed vmwgfx sets its private dev_mapping in 
the open hook, while all other TTM-based drivers (radeon and nouveau) do 
it when they create an object.


-- Ilija


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

Re: Breakage in "track dev_mapping in more robust and flexible way"

[Ilija Hadzic]

On Thu, Oct 25, 2012 at 11:10 AM, Thomas Hellström
 wrote:
> On 10/25/12 4:41 PM, Jerome Glisse wrote:
>>
>> On Thu, Oct 25, 2012 at 04:02:25PM +0200, Thomas Hellstrom wrote:
>>>
>>> Hi,
>>>
>>> This commit
>>>
>>>  From 949c4a34afacfe800fc442afac117aba15284962 Mon Sep 17 00:00:00 2001
>>> From: Ilija Hadzic 
>>> Date: Tue, 15 May 2012 16:40:10 -0400
>>> Subject: [PATCH] drm: track dev_mapping in more robust and flexible way
>>>
>>> Setting dev_mapping (pointer to the address_space structure
>>> used for memory mappings) to the address_space of the first
>>> opener's inode and then failing if other openers come in
>>> through a different inode has a few restrictions that are
>>> eliminated by this patch.
>>>
>>> If we already have valid dev_mapping and we spot an opener
>>> with different i_node, we force its i_mapping pointer to the
>>> already established address_space structure (first opener's
>>> inode). This will make all mappings from drm device hang off
>>> the same address_space object.
>>> ...
>>>
>>> Breaks drivers using TTM, since when the X server calls into the
>>> driver open, drm's dev_mapping has not
>>> yet been setup. The setup needs to be moved before the driver's open
>>> hook is called.
>>>
>>> Typically, if a TTM-aware driver is provoked by the Xorg server to
>>> move a buffer from system to VRAM or AGP,
>>> before any other drm client is started, The user-space page table
>>> entries are not killed before the move, and left pointing
>>> into freed pages, causing system crashes and / or user-space access
>>> to arbitrary memory.
>>
>> Doesn't handle move invalidate the drm file mapping before scheduling
>> the move ?
>
> Yes, but to do that it needs a correct value of bdev::dev_mapping, which is
> now incorrectly set on the
> *second* open instead of the first open.
>

So you are implying that in the first open the assignment of dev->dev_mapping is
somehow skipped (which could happen if drm_setup returns an error) or that the
driver on which you are having problems with (nouveau I presume) needs
dev_mapping
in the firstopen hook ?

It's been a while since I did it, but if my memory serves me well I
thought I explicitly
verified that dev_mapping was correctly set in the first open (though
GPUs I use are
primarily AMD).

-- Ilija


> /Thomas
>
>
>
>> Cheers,
>> Jerome
>
>
> ___
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Breakage in "track dev_mapping in more robust and flexible way"

[Thomas Hellström]

On 10/25/12 4:41 PM, Jerome Glisse wrote:

On Thu, Oct 25, 2012 at 04:02:25PM +0200, Thomas Hellstrom wrote:

Hi,

This commit

 From 949c4a34afacfe800fc442afac117aba15284962 Mon Sep 17 00:00:00 2001
From: Ilija Hadzic 
Date: Tue, 15 May 2012 16:40:10 -0400
Subject: [PATCH] drm: track dev_mapping in more robust and flexible way

Setting dev_mapping (pointer to the address_space structure
used for memory mappings) to the address_space of the first
opener's inode and then failing if other openers come in
through a different inode has a few restrictions that are
eliminated by this patch.

If we already have valid dev_mapping and we spot an opener
with different i_node, we force its i_mapping pointer to the
already established address_space structure (first opener's
inode). This will make all mappings from drm device hang off
the same address_space object.
...

Breaks drivers using TTM, since when the X server calls into the
driver open, drm's dev_mapping has not
yet been setup. The setup needs to be moved before the driver's open
hook is called.

Typically, if a TTM-aware driver is provoked by the Xorg server to
move a buffer from system to VRAM or AGP,
before any other drm client is started, The user-space page table
entries are not killed before the move, and left pointing
into freed pages, causing system crashes and / or user-space access
to arbitrary memory.

Doesn't handle move invalidate the drm file mapping before scheduling
the move ?
Yes, but to do that it needs a correct value of bdev::dev_mapping, which 
is now incorrectly set on the

*second* open instead of the first open.

/Thomas



Cheers,
Jerome


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

Re: Breakage in "track dev_mapping in more robust and flexible way"

[Jerome Glisse]

On Thu, Oct 25, 2012 at 04:02:25PM +0200, Thomas Hellstrom wrote:
> Hi,
> 
> This commit
> 
> From 949c4a34afacfe800fc442afac117aba15284962 Mon Sep 17 00:00:00 2001
> From: Ilija Hadzic 
> Date: Tue, 15 May 2012 16:40:10 -0400
> Subject: [PATCH] drm: track dev_mapping in more robust and flexible way
> 
> Setting dev_mapping (pointer to the address_space structure
> used for memory mappings) to the address_space of the first
> opener's inode and then failing if other openers come in
> through a different inode has a few restrictions that are
> eliminated by this patch.
> 
> If we already have valid dev_mapping and we spot an opener
> with different i_node, we force its i_mapping pointer to the
> already established address_space structure (first opener's
> inode). This will make all mappings from drm device hang off
> the same address_space object.
> ...
> 
> Breaks drivers using TTM, since when the X server calls into the
> driver open, drm's dev_mapping has not
> yet been setup. The setup needs to be moved before the driver's open
> hook is called.
> 
> Typically, if a TTM-aware driver is provoked by the Xorg server to
> move a buffer from system to VRAM or AGP,
> before any other drm client is started, The user-space page table
> entries are not killed before the move, and left pointing
> into freed pages, causing system crashes and / or user-space access
> to arbitrary memory.

Doesn't handle move invalidate the drm file mapping before scheduling
the move ?

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

Breakage in "track dev_mapping in more robust and flexible way"

[Thomas Hellstrom]

Hi,

This commit

From 949c4a34afacfe800fc442afac117aba15284962 Mon Sep 17 00:00:00 2001
From: Ilija Hadzic 
Date: Tue, 15 May 2012 16:40:10 -0400
Subject: [PATCH] drm: track dev_mapping in more robust and flexible way

Setting dev_mapping (pointer to the address_space structure
used for memory mappings) to the address_space of the first
opener's inode and then failing if other openers come in
through a different inode has a few restrictions that are
eliminated by this patch.

If we already have valid dev_mapping and we spot an opener
with different i_node, we force its i_mapping pointer to the
already established address_space structure (first opener's
inode). This will make all mappings from drm device hang off
the same address_space object.
...

Breaks drivers using TTM, since when the X server calls into the driver 
open, drm's dev_mapping has not
yet been setup. The setup needs to be moved before the driver's open 
hook is called.


Typically, if a TTM-aware driver is provoked by the Xorg server to move 
a buffer from system to VRAM or AGP,
before any other drm client is started, The user-space page table 
entries are not killed before the move, and left pointing
into freed pages, causing system crashes and / or user-space access to 
arbitrary memory.


/Thomas

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