Re: [PATCH] drm/vmwgfx: remove DRM_ERROR message, stops log spamming
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
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
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
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
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