Re: [PATCH] drm/vmwgfx: remove DRM_ERROR message, stops log spamming

[Daniel Vetter]

On Tue, Sep 12, 2017 at 06:54:45PM +0100, Emil Velikov wrote:
> On 12 September 2017 at 18:47, Colin Ian King  
> wrote:
> > On 12/09/17 18:42, Thomas Hellstrom wrote:
> >> Hi, Colin,
> >>
> >> On 09/12/2017 07:35 PM, Colin King wrote:
> >>> From: Colin Ian King 
> >>>
> >>> mmap'ing the device multiple times will spam the kernel log with the
> >>> DRM_ERROR message about illegal mmap'ing the old fifo space.
> >> How are you hitting this? Multiple mappings should be fine as long as
> >> mapping offsets are correct,
> >> so hitting this message should indicate that the user-space app is doing
> >> something seriously wrong, and
> >> having it present in the log should probably help more than it hurts.
> >>
> >> /Thomas
> >
> > Good question.  I hit similar issues with the drm qxl driver when
> > running some kernel regression tests with stress-ng [1]. I realize this
> > is an artificial test scenario so it is definitely not a typical
> > use-case, however, sync the illegal mmapping will return -EINVAL the
> > application will pick up that this is an error without the need of
> > spotting it in the kernel log. And a user space application can perform
> > many millions of these invalid mmaps causing kernel log spamming.
> >
> FWIW I'm the one to "blame" here - pointing Colin to drop the message.
> 
> Two reasons come to mind:
>  - there is a unwritten rule that roughly says "user input should not
> cause kernel log spam"
>  - out of all the DRM drivers only QXL and VMWGFX print a message,
> with a patch addressing the former

Maybe we should make this a written rule by patching
Documentation/drivers/gpu? Would definitely make sense as part of this
patch series.

Thanks, Daniel
> 
> HTH
> Emil
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH] drm/vmwgfx: remove DRM_ERROR message, stops log spamming

[Daniel Vetter]

On Tue, Sep 12, 2017 at 06:54:45PM +0100, Emil Velikov wrote:
> On 12 September 2017 at 18:47, Colin Ian King  
> wrote:
> > On 12/09/17 18:42, Thomas Hellstrom wrote:
> >> Hi, Colin,
> >>
> >> On 09/12/2017 07:35 PM, Colin King wrote:
> >>> From: Colin Ian King 
> >>>
> >>> mmap'ing the device multiple times will spam the kernel log with the
> >>> DRM_ERROR message about illegal mmap'ing the old fifo space.
> >> How are you hitting this? Multiple mappings should be fine as long as
> >> mapping offsets are correct,
> >> so hitting this message should indicate that the user-space app is doing
> >> something seriously wrong, and
> >> having it present in the log should probably help more than it hurts.
> >>
> >> /Thomas
> >
> > Good question.  I hit similar issues with the drm qxl driver when
> > running some kernel regression tests with stress-ng [1]. I realize this
> > is an artificial test scenario so it is definitely not a typical
> > use-case, however, sync the illegal mmapping will return -EINVAL the
> > application will pick up that this is an error without the need of
> > spotting it in the kernel log. And a user space application can perform
> > many millions of these invalid mmaps causing kernel log spamming.
> >
> FWIW I'm the one to "blame" here - pointing Colin to drop the message.
> 
> Two reasons come to mind:
>  - there is a unwritten rule that roughly says "user input should not
> cause kernel log spam"
>  - out of all the DRM drivers only QXL and VMWGFX print a message,
> with a patch addressing the former

Maybe we should make this a written rule by patching
Documentation/drivers/gpu? Would definitely make sense as part of this
patch series.

Thanks, Daniel
> 
> HTH
> Emil
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH] drm/vmwgfx: remove DRM_ERROR message, stops log spamming

[Emil Velikov]

On 12 September 2017 at 18:47, Colin Ian King  wrote:
> On 12/09/17 18:42, Thomas Hellstrom wrote:
>> Hi, Colin,
>>
>> On 09/12/2017 07:35 PM, Colin King wrote:
>>> From: Colin Ian King 
>>>
>>> mmap'ing the device multiple times will spam the kernel log with the
>>> DRM_ERROR message about illegal mmap'ing the old fifo space.
>> How are you hitting this? Multiple mappings should be fine as long as
>> mapping offsets are correct,
>> so hitting this message should indicate that the user-space app is doing
>> something seriously wrong, and
>> having it present in the log should probably help more than it hurts.
>>
>> /Thomas
>
> Good question.  I hit similar issues with the drm qxl driver when
> running some kernel regression tests with stress-ng [1]. I realize this
> is an artificial test scenario so it is definitely not a typical
> use-case, however, sync the illegal mmapping will return -EINVAL the
> application will pick up that this is an error without the need of
> spotting it in the kernel log. And a user space application can perform
> many millions of these invalid mmaps causing kernel log spamming.
>
FWIW I'm the one to "blame" here - pointing Colin to drop the message.

Two reasons come to mind:
 - there is a unwritten rule that roughly says "user input should not
cause kernel log spam"
 - out of all the DRM drivers only QXL and VMWGFX print a message,
with a patch addressing the former

HTH
Emil

Re: [PATCH] drm/vmwgfx: remove DRM_ERROR message, stops log spamming

[Emil Velikov]

On 12 September 2017 at 18:47, Colin Ian King  wrote:
> On 12/09/17 18:42, Thomas Hellstrom wrote:
>> Hi, Colin,
>>
>> On 09/12/2017 07:35 PM, Colin King wrote:
>>> From: Colin Ian King 
>>>
>>> mmap'ing the device multiple times will spam the kernel log with the
>>> DRM_ERROR message about illegal mmap'ing the old fifo space.
>> How are you hitting this? Multiple mappings should be fine as long as
>> mapping offsets are correct,
>> so hitting this message should indicate that the user-space app is doing
>> something seriously wrong, and
>> having it present in the log should probably help more than it hurts.
>>
>> /Thomas
>
> Good question.  I hit similar issues with the drm qxl driver when
> running some kernel regression tests with stress-ng [1]. I realize this
> is an artificial test scenario so it is definitely not a typical
> use-case, however, sync the illegal mmapping will return -EINVAL the
> application will pick up that this is an error without the need of
> spotting it in the kernel log. And a user space application can perform
> many millions of these invalid mmaps causing kernel log spamming.
>
FWIW I'm the one to "blame" here - pointing Colin to drop the message.

Two reasons come to mind:
 - there is a unwritten rule that roughly says "user input should not
cause kernel log spam"
 - out of all the DRM drivers only QXL and VMWGFX print a message,
with a patch addressing the former

HTH
Emil

Re: [PATCH] drm/vmwgfx: remove DRM_ERROR message, stops log spamming

[Colin Ian King]

On 12/09/17 18:42, Thomas Hellstrom wrote:
> Hi, Colin,
> 
> On 09/12/2017 07:35 PM, Colin King wrote:
>> From: Colin Ian King 
>>
>> mmap'ing the device multiple times will spam the kernel log with the
>> DRM_ERROR message about illegal mmap'ing the old fifo space.
> How are you hitting this? Multiple mappings should be fine as long as
> mapping offsets are correct,
> so hitting this message should indicate that the user-space app is doing
> something seriously wrong, and
> having it present in the log should probably help more than it hurts.
> 
> /Thomas

Good question.  I hit similar issues with the drm qxl driver when
running some kernel regression tests with stress-ng [1]. I realize this
is an artificial test scenario so it is definitely not a typical
use-case, however, sync the illegal mmapping will return -EINVAL the
application will pick up that this is an error without the need of
spotting it in the kernel log. And a user space application can perform
many millions of these invalid mmaps causing kernel log spamming.

[1] http://kernel.ubuntu.com/~cking/stress-ng/

> 
> 
> 
>> Since
>> the mmap'ing will fail with an -EINVAL there is no need to emit this
>> message, so just remove it.
>>
>> Signed-off-by: Colin Ian King 
>> ---
>>   drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c | 4 +---
>>   1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c
>> b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c
>> index e771091d2cd3..1e633c602fb1 100644
>> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c
>> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c
>> @@ -33,10 +33,8 @@ int vmw_mmap(struct file *filp, struct
>> vm_area_struct *vma)
>>   struct drm_file *file_priv;
>>   struct vmw_private *dev_priv;
>>   -if (unlikely(vma->vm_pgoff < VMWGFX_FILE_PAGE_OFFSET)) {
>> -DRM_ERROR("Illegal attempt to mmap old fifo space.\n");
>> +if (unlikely(vma->vm_pgoff < VMWGFX_FILE_PAGE_OFFSET))
>>   return -EINVAL;
>> -}
>> file_priv = filp->private_data;
>>   dev_priv = vmw_priv(file_priv->minor->dev);
> 
> 

Re: [PATCH] drm/vmwgfx: remove DRM_ERROR message, stops log spamming

[Colin Ian King]

On 12/09/17 18:42, Thomas Hellstrom wrote:
> Hi, Colin,
> 
> On 09/12/2017 07:35 PM, Colin King wrote:
>> From: Colin Ian King 
>>
>> mmap'ing the device multiple times will spam the kernel log with the
>> DRM_ERROR message about illegal mmap'ing the old fifo space.
> How are you hitting this? Multiple mappings should be fine as long as
> mapping offsets are correct,
> so hitting this message should indicate that the user-space app is doing
> something seriously wrong, and
> having it present in the log should probably help more than it hurts.
> 
> /Thomas

Good question.  I hit similar issues with the drm qxl driver when
running some kernel regression tests with stress-ng [1]. I realize this
is an artificial test scenario so it is definitely not a typical
use-case, however, sync the illegal mmapping will return -EINVAL the
application will pick up that this is an error without the need of
spotting it in the kernel log. And a user space application can perform
many millions of these invalid mmaps causing kernel log spamming.

[1] http://kernel.ubuntu.com/~cking/stress-ng/

> 
> 
> 
>> Since
>> the mmap'ing will fail with an -EINVAL there is no need to emit this
>> message, so just remove it.
>>
>> Signed-off-by: Colin Ian King 
>> ---
>>   drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c | 4 +---
>>   1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c
>> b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c
>> index e771091d2cd3..1e633c602fb1 100644
>> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c
>> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c
>> @@ -33,10 +33,8 @@ int vmw_mmap(struct file *filp, struct
>> vm_area_struct *vma)
>>   struct drm_file *file_priv;
>>   struct vmw_private *dev_priv;
>>   -if (unlikely(vma->vm_pgoff < VMWGFX_FILE_PAGE_OFFSET)) {
>> -DRM_ERROR("Illegal attempt to mmap old fifo space.\n");
>> +if (unlikely(vma->vm_pgoff < VMWGFX_FILE_PAGE_OFFSET))
>>   return -EINVAL;
>> -}
>> file_priv = filp->private_data;
>>   dev_priv = vmw_priv(file_priv->minor->dev);
> 
> 

Re: [PATCH] drm/vmwgfx: remove DRM_ERROR message, stops log spamming

[Thomas Hellstrom]

Hi, Colin,

On 09/12/2017 07:35 PM, Colin King wrote:

From: Colin Ian King 

mmap'ing the device multiple times will spam the kernel log with the
DRM_ERROR message about illegal mmap'ing the old fifo space.
How are you hitting this? Multiple mappings should be fine as long as 
mapping offsets are correct,
so hitting this message should indicate that the user-space app is doing 
something seriously wrong, and

having it present in the log should probably help more than it hurts.

/Thomas




Since
the mmap'ing will fail with an -EINVAL there is no need to emit this
message, so just remove it.

Signed-off-by: Colin Ian King 
---
  drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c | 4 +---
  1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c 
b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c
index e771091d2cd3..1e633c602fb1 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c
@@ -33,10 +33,8 @@ int vmw_mmap(struct file *filp, struct vm_area_struct *vma)
struct drm_file *file_priv;
struct vmw_private *dev_priv;
  
-	if (unlikely(vma->vm_pgoff < VMWGFX_FILE_PAGE_OFFSET)) {

-   DRM_ERROR("Illegal attempt to mmap old fifo space.\n");
+   if (unlikely(vma->vm_pgoff < VMWGFX_FILE_PAGE_OFFSET))
return -EINVAL;
-   }
  
  	file_priv = filp->private_data;

dev_priv = vmw_priv(file_priv->minor->dev);

Re: [PATCH] drm/vmwgfx: remove DRM_ERROR message, stops log spamming

[Thomas Hellstrom]

Hi, Colin,

On 09/12/2017 07:35 PM, Colin King wrote:

From: Colin Ian King 

mmap'ing the device multiple times will spam the kernel log with the
DRM_ERROR message about illegal mmap'ing the old fifo space.
How are you hitting this? Multiple mappings should be fine as long as 
mapping offsets are correct,
so hitting this message should indicate that the user-space app is doing 
something seriously wrong, and

having it present in the log should probably help more than it hurts.

/Thomas




Since
the mmap'ing will fail with an -EINVAL there is no need to emit this
message, so just remove it.

Signed-off-by: Colin Ian King 
---
  drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c | 4 +---
  1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c 
b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c
index e771091d2cd3..1e633c602fb1 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c
@@ -33,10 +33,8 @@ int vmw_mmap(struct file *filp, struct vm_area_struct *vma)
struct drm_file *file_priv;
struct vmw_private *dev_priv;
  
-	if (unlikely(vma->vm_pgoff < VMWGFX_FILE_PAGE_OFFSET)) {

-   DRM_ERROR("Illegal attempt to mmap old fifo space.\n");
+   if (unlikely(vma->vm_pgoff < VMWGFX_FILE_PAGE_OFFSET))
return -EINVAL;
-   }
  
  	file_priv = filp->private_data;

dev_priv = vmw_priv(file_priv->minor->dev);

[PATCH] drm/vmwgfx: remove DRM_ERROR message, stops log spamming

[Colin King]

From: Colin Ian King 

mmap'ing the device multiple times will spam the kernel log with the
DRM_ERROR message about illegal mmap'ing the old fifo space. Since
the mmap'ing will fail with an -EINVAL there is no need to emit this
message, so just remove it.

Signed-off-by: Colin Ian King 
---
 drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c 
b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c
index e771091d2cd3..1e633c602fb1 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c
@@ -33,10 +33,8 @@ int vmw_mmap(struct file *filp, struct vm_area_struct *vma)
struct drm_file *file_priv;
struct vmw_private *dev_priv;
 
-   if (unlikely(vma->vm_pgoff < VMWGFX_FILE_PAGE_OFFSET)) {
-   DRM_ERROR("Illegal attempt to mmap old fifo space.\n");
+   if (unlikely(vma->vm_pgoff < VMWGFX_FILE_PAGE_OFFSET))
return -EINVAL;
-   }
 
file_priv = filp->private_data;
dev_priv = vmw_priv(file_priv->minor->dev);
-- 
2.14.1

[PATCH] drm/vmwgfx: remove DRM_ERROR message, stops log spamming

[Colin King]

From: Colin Ian King 

mmap'ing the device multiple times will spam the kernel log with the
DRM_ERROR message about illegal mmap'ing the old fifo space. Since
the mmap'ing will fail with an -EINVAL there is no need to emit this
message, so just remove it.

Signed-off-by: Colin Ian King 
---
 drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c 
b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c
index e771091d2cd3..1e633c602fb1 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c
@@ -33,10 +33,8 @@ int vmw_mmap(struct file *filp, struct vm_area_struct *vma)
struct drm_file *file_priv;
struct vmw_private *dev_priv;
 
-   if (unlikely(vma->vm_pgoff < VMWGFX_FILE_PAGE_OFFSET)) {
-   DRM_ERROR("Illegal attempt to mmap old fifo space.\n");
+   if (unlikely(vma->vm_pgoff < VMWGFX_FILE_PAGE_OFFSET))
return -EINVAL;
-   }
 
file_priv = filp->private_data;
dev_priv = vmw_priv(file_priv->minor->dev);
-- 
2.14.1