Re: 6.10/bisected/regression - commits bc87d666c05 and 6d4279cb99ac cause appearing green flashing bar on top of screen on Radeon 6900XT and 120Hz

[Eric Biggers]

On Tue, Jul 16, 2024 at 01:10:37PM -0400, Alex Deucher wrote:
> From 8aaf8da07a8b542c0a0f4da2601da07beddfdeb0 Mon Sep 17 00:00:00 2001
> From: Alex Deucher 
> Date: Tue, 16 Jul 2024 12:49:25 -0400
> Subject: [PATCH] drm/amd/display: fix corruption with high refresh rates on
>  DCN 3.0
> 
> This reverts commit bc87d666c05a13e6d4ae1ddce41fc43d2567b9a2 and the
> register changes from commit 6d4279cb99ac4f51d10409501d29969f687ac8dc.
> 
> Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/3478
> Cc: mikhail.v.gavri...@gmail.com
> Cc: Rodrigo Siqueira 
> Signed-off-by: Alex Deucher 
> ---
>  .../drm/amd/display/dc/optc/dcn10/dcn10_optc.c| 15 +++
>  .../drm/amd/display/dc/optc/dcn20/dcn20_optc.c| 10 ++
>  2 files changed, 13 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/dc/optc/dcn10/dcn10_optc.c 
> b/drivers/gpu/drm/amd/display/dc/optc/dcn10/dcn10_optc.c
> index 4f82146d94b1..f00d27b7c6fe 100644
> --- a/drivers/gpu/drm/amd/display/dc/optc/dcn10/dcn10_optc.c
> +++ b/drivers/gpu/drm/amd/display/dc/optc/dcn10/dcn10_optc.c
> @@ -950,19 +950,10 @@ void optc1_set_drr(
>   OTG_FORCE_LOCK_ON_EVENT, 0,
>   OTG_SET_V_TOTAL_MIN_MASK_EN, 0,
>   OTG_SET_V_TOTAL_MIN_MASK, 0);
> -
> - // Setup manual flow control for EOF via TRIG_A
> - optc->funcs->setup_manual_trigger(optc);
> -
> - } else {
> - REG_UPDATE_4(OTG_V_TOTAL_CONTROL,
> - OTG_SET_V_TOTAL_MIN_MASK, 0,
> - OTG_V_TOTAL_MIN_SEL, 0,
> - OTG_V_TOTAL_MAX_SEL, 0,
> - OTG_FORCE_LOCK_ON_EVENT, 0);
> -
> - optc->funcs->set_vtotal_min_max(optc, 0, 0);
>   }
> +
> + // Setup manual flow control for EOF via TRIG_A
> + optc->funcs->setup_manual_trigger(optc);
>  }
>  
>  void optc1_set_vtotal_min_max(struct timing_generator *optc, int vtotal_min, 
> int vtotal_max)
> diff --git a/drivers/gpu/drm/amd/display/dc/optc/dcn20/dcn20_optc.c 
> b/drivers/gpu/drm/amd/display/dc/optc/dcn20/dcn20_optc.c
> index 43417cff2c9b..b4694985a40a 100644
> --- a/drivers/gpu/drm/amd/display/dc/optc/dcn20/dcn20_optc.c
> +++ b/drivers/gpu/drm/amd/display/dc/optc/dcn20/dcn20_optc.c
> @@ -453,6 +453,16 @@ void optc2_setup_manual_trigger(struct timing_generator 
> *optc)
>  {
>   struct optc *optc1 = DCN10TG_FROM_TG(optc);
>  
> + /* Set the min/max selectors unconditionally so that
> +  * DMCUB fw may change OTG timings when necessary
> +  * TODO: Remove the w/a after fixing the issue in DMCUB firmware
> +  */
> + REG_UPDATE_4(OTG_V_TOTAL_CONTROL,
> +  OTG_V_TOTAL_MIN_SEL, 1,
> +  OTG_V_TOTAL_MAX_SEL, 1,
> +  OTG_FORCE_LOCK_ON_EVENT, 0,
> +  OTG_SET_V_TOTAL_MIN_MASK, (1 << 1)); /* TRIGA 
> */
> +
>   REG_SET_8(OTG_TRIGA_CNTL, 0,
>   OTG_TRIGA_SOURCE_SELECT, 21,
>   OTG_TRIGA_SOURCE_PIPE_SELECT, optc->inst,
> -- 
> 2.45.2

This patch fixes the bug for me too.  I am using a Radeon RX 6400, and I've been
encountering the bug when using 1920x1080 resolution on a monitor whose native
resolution is 2560x1440.  Feel free to add:

Tested-by: Eric Biggers 

Thanks,

- Eric

Re: [PATCH 7/8] arm64: dts: qcom: sm8450: remove invalid reg-names from ufs node

[Eric Biggers]

Hi Neil,

On Thu, Mar 23, 2023 at 02:10:44PM +0100, Neil Armstrong wrote:
> Hi,
> 
> On 23/03/2023 11:49, Krzysztof Kozlowski wrote:
> > On 23/03/2023 11:25, Neil Armstrong wrote:
> > > Fixes the following DT bindings check error:
> > > ufshc@1d84000: Unevaluated properties are not allowed ('reg-names' was 
> > > unexpected)
> > > 
> > > Signed-off-by: Neil Armstrong 
> > > ---
> > >   arch/arm64/boot/dts/qcom/sm8450.dtsi | 1 -
> > >   1 file changed, 1 deletion(-)
> > > 
> > > diff --git a/arch/arm64/boot/dts/qcom/sm8450.dtsi 
> > > b/arch/arm64/boot/dts/qcom/sm8450.dtsi
> > > index ef9bae2e6acc..8ecc48c7c5ef 100644
> > > --- a/arch/arm64/boot/dts/qcom/sm8450.dtsi
> > > +++ b/arch/arm64/boot/dts/qcom/sm8450.dtsi
> > > @@ -3996,7 +3996,6 @@ ufs_mem_hc: ufshc@1d84000 {
> > >"jedec,ufs-2.0";
> > >   reg = <0 0x01d84000 0 0x3000>,
> > > <0 0x01d88000 0 0x8000>;
> > > - reg-names = "std", "ice";
> > 
> > This is also part of:
> > https://lore.kernel.org/linux-arm-msm/20230308155838.1094920-8-abel.v...@linaro.org/#Z31arch:arm64:boot:dts:qcom:sm8450.dtsi
> > but I actually wonder whether you just missed some binding patch?
> 
> I'm aware of Abel's RFC patchset to support shared ICE, but this is a cleanup 
> of the current DT,
> and the current bindings schema doesn't document reg-names.
> 

The ufs-qcom driver accesses the "ice" registers by name, so the reg-names can't
be removed from the device tree.  A few months ago there was a patch to fix the
device tree schema for qcom,ufs to include the reg-names.  It looks like that
patch got missed, though:
https://lore.kernel.org/r/20221209-dt-binding-ufs-v2-2-dc7a04699...@fairphone.com

- Eric

Re: [PATCH AUTOSEL 5.19 07/16] drm/amdgpu: use dirty framebuffer helper

[Eric Biggers]

On Wed, Sep 21, 2022 at 11:53:23AM -0400, Sasha Levin wrote:
> From: Hamza Mahfooz 
> 
> [ Upstream commit 66f99628eb24409cb8feb5061f78283c8b65f820 ]
> 
> Currently, we aren't handling DRM_IOCTL_MODE_DIRTYFB. So, use
> drm_atomic_helper_dirtyfb() as the dirty callback in the amdgpu_fb_funcs
> struct.
> 
> Signed-off-by: Hamza Mahfooz 
> Acked-by: Alex Deucher 
> Signed-off-by: Alex Deucher 
> Signed-off-by: Sasha Levin 

I just spent a long time bisecting a hard-to-reproduce regression to this
commit, only to find that a revert was just queued this week.

Why was this commit backported to stable in the first place?  It didn't have Cc
stable, and it didn't claim to be fixing anything.

- Eric

Re: [PATCH RFC PKS/PMEM 22/58] fs/f2fs: Utilize new kmap_thread()

[Eric Biggers]

On Sun, Oct 11, 2020 at 11:56:35PM -0700, Ira Weiny wrote:
> > 
> > And I still don't really understand.  After this patchset, there is still 
> > code
> > nearly identical to the above (doing a temporary mapping just for a memcpy) 
> > that
> > would still be using kmap_atomic().
> 
> I don't understand.  You mean there would be other call sites calling:
> 
> kmap_atomic()
> memcpy()
> kunmap_atomic()

Yes, there are tons of places that do this.  Try 'git grep -A6 kmap_atomic'
and look for memcpy().

Hence why I'm asking what will be the "recommended" way to do this...
kunmap_thread() or kmap_atomic()?

> And since I don't know the call site details if there are kmap_thread() calls
> which are better off as kmap_atomic() calls I think it is worth converting
> them.  But I made the assumption that kmap users would already be calling
> kmap_atomic() if they could (because it is more efficient).

Not necessarily.  In cases where either one is correct, people might not have
put much thought into which of kmap() and kmap_atomic() they are using.

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

Re: [PATCH RFC PKS/PMEM 22/58] fs/f2fs: Utilize new kmap_thread()

[Eric Biggers]

On Sat, Oct 10, 2020 at 01:39:54AM +0100, Matthew Wilcox wrote:
> On Fri, Oct 09, 2020 at 02:34:34PM -0700, Eric Biggers wrote:
> > On Fri, Oct 09, 2020 at 12:49:57PM -0700, ira.we...@intel.com wrote:
> > > The kmap() calls in this FS are localized to a single thread.  To avoid
> > > the over head of global PKRS updates use the new kmap_thread() call.
> > >
> > > @@ -2410,12 +2410,12 @@ static inline struct page 
> > > *f2fs_pagecache_get_page(
> > >  
> > >  static inline void f2fs_copy_page(struct page *src, struct page *dst)
> > >  {
> > > - char *src_kaddr = kmap(src);
> > > - char *dst_kaddr = kmap(dst);
> > > + char *src_kaddr = kmap_thread(src);
> > > + char *dst_kaddr = kmap_thread(dst);
> > >  
> > >   memcpy(dst_kaddr, src_kaddr, PAGE_SIZE);
> > > - kunmap(dst);
> > > - kunmap(src);
> > > + kunmap_thread(dst);
> > > + kunmap_thread(src);
> > >  }
> > 
> > Wouldn't it make more sense to switch cases like this to kmap_atomic()?
> > The pages are only mapped to do a memcpy(), then they're immediately 
> > unmapped.
> 
> Maybe you missed the earlier thread from Thomas trying to do something
> similar for rather different reasons ...
> 
> https://lore.kernel.org/lkml/20200919091751.06...@linutronix.de/

I did miss it.  I'm not subscribed to any of the mailing lists it was sent to.

Anyway, it shouldn't matter.  Patchsets should be standalone, and not require
reading random prior threads on linux-kernel to understand.

And I still don't really understand.  After this patchset, there is still code
nearly identical to the above (doing a temporary mapping just for a memcpy) that
would still be using kmap_atomic().  Is the idea that later, such code will be
converted to use kmap_thread() instead?  If not, why use one over the other?

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

Re: [PATCH RFC PKS/PMEM 22/58] fs/f2fs: Utilize new kmap_thread()

[Eric Biggers]

On Fri, Oct 09, 2020 at 12:49:57PM -0700, ira.we...@intel.com wrote:
> From: Ira Weiny 
> 
> The kmap() calls in this FS are localized to a single thread.  To avoid
> the over head of global PKRS updates use the new kmap_thread() call.
> 
> Cc: Jaegeuk Kim 
> Cc: Chao Yu 
> Signed-off-by: Ira Weiny 
> ---
>  fs/f2fs/f2fs.h | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index d9e52a7f3702..ff72a45a577e 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -2410,12 +2410,12 @@ static inline struct page *f2fs_pagecache_get_page(
>  
>  static inline void f2fs_copy_page(struct page *src, struct page *dst)
>  {
> - char *src_kaddr = kmap(src);
> - char *dst_kaddr = kmap(dst);
> + char *src_kaddr = kmap_thread(src);
> + char *dst_kaddr = kmap_thread(dst);
>  
>   memcpy(dst_kaddr, src_kaddr, PAGE_SIZE);
> - kunmap(dst);
> - kunmap(src);
> + kunmap_thread(dst);
> + kunmap_thread(src);
>  }

Wouldn't it make more sense to switch cases like this to kmap_atomic()?
The pages are only mapped to do a memcpy(), then they're immediately unmapped.

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

Re: [PATCH 12/15] drm/amdgpu/display: add a late register connector callback

[Eric Biggers]

On Fri, Feb 07, 2020 at 02:50:55PM -0500, Alex Deucher wrote:
> To handle debugfs setup on non DP MST connectors.
> 
> Reviewed-by: Harry Wentland 
> Acked-by: Christian König 
> Signed-off-by: Alex Deucher 
> ---
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c  | 18 ++
>  1 file changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c 
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index af8155708569..b6190079ed3f 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -4486,6 +4486,19 @@ amdgpu_dm_connector_atomic_duplicate_state(struct 
> drm_connector *connector)
>   return _state->base;
>  }
>  
> +static int
> +amdgpu_dm_connector_late_register(struct drm_connector *connector)
> +{
> + struct amdgpu_dm_connector *amdgpu_dm_connector =
> + to_amdgpu_dm_connector(connector);
> +
> +#if defined(CONFIG_DEBUG_FS)
> + connector_debugfs_init(amdgpu_dm_connector);
> +#endif
> +
> + return 0;
> +}

This introduced a compiler warning:

drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c: In function 
‘amdgpu_dm_connector_late_register’:
drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:4726:30: warning: 
unused variable ‘amdgpu_dm_connector’ [-Wunused-variable]
 4726 |  struct amdgpu_dm_connector *amdgpu_dm_connector =
  |  ^~~

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

Reminder: 1 open syzbot bug in drm subsystem

[Eric Biggers]

[This email was generated by a script.  Let me know if you have any suggestions
to make it better, or if you want it re-generated with the latest status.]

Of the currently open syzbot reports against the upstream kernel, I've manually
marked 1 of them as possibly being a bug in the drm subsystem.

If you believe this bug is no longer valid, please close the syzbot report by
sending a '#syz fix', '#syz dup', or '#syz invalid' command in reply to the
original thread, as explained at https://goo.gl/tpsmEJ#status

If you believe I misattributed this bug to the drm subsystem, please let me
know, and if possible forward the report to the correct people or mailing list.

Here is the bug:


Title:  WARNING in vkms_vblank_simulate
Last occurred:  0 days ago
Reported:   162 days ago
Branches:   Mainline and others
Dashboard link: 
https://syzkaller.appspot.com/bug?id=0ba17d70d062b2595e1f061231474800f076c7cb
Original thread:
https://lkml.kernel.org/lkml/11c9310581a57...@google.com/T/#u

This bug has a C reproducer.

This bug was bisected to:

commit 09ef09b4ab95dc405ad4171ec2cd8a4ff5227108
Author: Shayenne Moura 
Date:   Wed Feb 6 20:08:13 2019 +

  drm/vkms: WARN when hrtimer_forward_now fails

The original thread for this bug received 1 reply, 134 days ago.

If you fix this bug, please add the following tag to the commit:
Reported-by: syzbot+0871b14ca2e2fb64f...@syzkaller.appspotmail.com

If you send any email or patch for this bug, please consider replying to the
original thread.  For the git send-email command to use, or tips on how to reply
if the thread isn't in your mailbox, see the "Reply instructions" at
https://lkml.kernel.org/r/11c9310581a57...@google.com

Reminder: 1 open syzbot bug in drm subsystem

[Eric Biggers]

[This email was generated by a script.  Let me know if you have any suggestions
to make it better, or if you want it re-generated with the latest status.]

Of the currently open syzbot reports against the upstream kernel, I've manually
marked 1 of them as possibly being a bug in the drm subsystem.

If you believe this bug is no longer valid, please close the syzbot report by
sending a '#syz fix', '#syz dup', or '#syz invalid' command in reply to the
original thread, as explained at https://goo.gl/tpsmEJ#status

If you believe I misattributed this bug to the drm subsystem, please let me
know, and if possible forward the report to the correct people or mailing list.

Here is the bug:


Title:  WARNING in vkms_vblank_simulate
Last occurred:  0 days ago
Reported:   140 days ago
Branches:   Mainline and others
Dashboard link: 
https://syzkaller.appspot.com/bug?id=0ba17d70d062b2595e1f061231474800f076c7cb
Original thread:
https://lkml.kernel.org/lkml/11c9310581a57...@google.com/T/#u

This bug has a C reproducer.

This bug was bisected to:

commit 09ef09b4ab95dc405ad4171ec2cd8a4ff5227108
Author: Shayenne Moura 
Date:   Wed Feb 6 20:08:13 2019 +

  drm/vkms: WARN when hrtimer_forward_now fails

The original thread for this bug received 1 reply, 112 days ago.

If you fix this bug, please add the following tag to the commit:
Reported-by: syzbot+0871b14ca2e2fb64f...@syzkaller.appspotmail.com

If you send any email or patch for this bug, please consider replying to the
original thread.  For the git send-email command to use, or tips on how to reply
if the thread isn't in your mailbox, see the "Reply instructions" at
https://lkml.kernel.org/r/11c9310581a57...@google.com

[PATCH] drm/vkms: fix use-after-free when drm_gem_handle_create() fails

[Eric Biggers]

From: Eric Biggers 

If drm_gem_handle_create() fails in vkms_gem_create(), then the
vkms_gem_object is freed twice: once when the reference is dropped by
drm_gem_object_put_unlocked(), and again by the extra calls to
drm_gem_object_release() and kfree().

Fix it by skipping the second release and free.

This bug was originally found in the vgem driver by syzkaller using
fault injection, but I noticed it's also present in the vkms driver.

Fixes: 559e50fd34d1 ("drm/vkms: Add dumb operations")
Cc: Rodrigo Siqueira 
Cc: Haneen Mohammed 
Cc: Daniel Vetter 
Cc: Chris Wilson 
Cc: sta...@vger.kernel.org
Signed-off-by: Eric Biggers 
---
 drivers/gpu/drm/vkms/vkms_gem.c | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_gem.c b/drivers/gpu/drm/vkms/vkms_gem.c
index 138b0bb325cf9..69048e73377dc 100644
--- a/drivers/gpu/drm/vkms/vkms_gem.c
+++ b/drivers/gpu/drm/vkms/vkms_gem.c
@@ -111,11 +111,8 @@ struct drm_gem_object *vkms_gem_create(struct drm_device 
*dev,
 
ret = drm_gem_handle_create(file, >gem, handle);
drm_gem_object_put_unlocked(>gem);
-   if (ret) {
-   drm_gem_object_release(>gem);
-   kfree(obj);
+   if (ret)
return ERR_PTR(ret);
-   }
 
return >gem;
 }
-- 
2.21.0.rc2.261.ga7da99ff1b-goog

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

[PATCH v2] drm/vgem: fix use-after-free when drm_gem_handle_create() fails

[Eric Biggers]

From: Eric Biggers 

If drm_gem_handle_create() fails in vgem_gem_create(), then the
drm_vgem_gem_object is freed twice: once when the reference is dropped
by drm_gem_object_put_unlocked(), and again by __vgem_gem_destroy().

This was hit by syzkaller using fault injection.

Fix it by skipping the second free.

Reported-by: syzbot+e73f2fb5ed5a5df36...@syzkaller.appspotmail.com
Fixes: af33a9190d02 ("drm/vgem: Enable dmabuf import interfaces")
Reviewed-by: Chris Wilson 
Cc: Laura Abbott 
Cc: Daniel Vetter 
Cc: sta...@vger.kernel.org
Signed-off-by: Eric Biggers 
---
 drivers/gpu/drm/vgem/vgem_drv.c | 6 +-
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/vgem/vgem_drv.c b/drivers/gpu/drm/vgem/vgem_drv.c
index 5930facd6d2d8..11a8f99ba18c5 100644
--- a/drivers/gpu/drm/vgem/vgem_drv.c
+++ b/drivers/gpu/drm/vgem/vgem_drv.c
@@ -191,13 +191,9 @@ static struct drm_gem_object *vgem_gem_create(struct 
drm_device *dev,
ret = drm_gem_handle_create(file, >base, handle);
drm_gem_object_put_unlocked(>base);
if (ret)
-   goto err;
+   return ERR_PTR(ret);
 
return >base;
-
-err:
-   __vgem_gem_destroy(obj);
-   return ERR_PTR(ret);
 }
 
 static int vgem_gem_dumb_create(struct drm_file *file, struct drm_device *dev,
-- 
2.21.0.rc2.261.ga7da99ff1b-goog

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

Re: [PATCH] drm/vgem: fix use-after-free when drm_gem_handle_create() fails

[Eric Biggers]

On Tue, Feb 26, 2019 at 09:01:29PM +, Chris Wilson wrote:
> Quoting Eric Biggers (2019-02-26 20:47:26)
> > From: Eric Biggers 
> > 
> > If drm_gem_handle_create() fails in vgem_gem_create(), then the
> > drm_vgem_gem_object is freed twice: once when the reference is dropped
> > by drm_gem_object_put_unlocked(), and again by __vgem_gem_destroy().
> > 
> > This was hit by syzkaller using fault injection.
> > 
> > Fix it by skipping the second free.
> > 
> > Reported-by: syzbot+e73f2fb5ed5a5df36...@syzkaller.appspotmail.com
> > Fixes: 5ba6c9ff961a ("drm/vgem: Fix mmaping")
> 
> That's the wrong fixes line, it's
> Fixes: af33a9190d02 ("drm/vgem: Enable dmabuf import interfaces")
> Cc: Chris Wilson 
> Cc: Laura Abbott 
> Cc: Daniel Vetter 
> 
> Sadly I reviewed it so I'm still culpable, but the fix is correct as the
> put purposely frees it on error.
> 

You're right; I misread the code at that commit.
I'll resend with the correct tags.

> > Cc: sta...@vger.kernel.org
> > Signed-off-by: Eric Biggers 
> 
> > ---
> >  drivers/gpu/drm/vgem/vgem_drv.c | 8 +++-
> >  1 file changed, 3 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/vgem/vgem_drv.c 
> > b/drivers/gpu/drm/vgem/vgem_drv.c
> > index 5930facd6d2d8..70646d9da1596 100644
> > --- a/drivers/gpu/drm/vgem/vgem_drv.c
> > +++ b/drivers/gpu/drm/vgem/vgem_drv.c
> > @@ -189,15 +189,13 @@ static struct drm_gem_object *vgem_gem_create(struct 
> > drm_device *dev,
> > return ERR_CAST(obj);
> >  
> > ret = drm_gem_handle_create(file, >base, handle);
> > +
> > drm_gem_object_put_unlocked(>base);
> > +
> 
> The pattern in the other GEM drivers is not to have these extra
> newlines.
> 
> Reviewed-by: Chris Wilson 
> 
> > if (ret)
> > -   goto err;
> > +   return ERR_PTR(ret);
> >  
> > return >base;
> > -
> > -err:
> > -   __vgem_gem_destroy(obj);
> > -   return ERR_PTR(ret);
> >  }
> >  
> >  static int vgem_gem_dumb_create(struct drm_file *file, struct drm_device 
> > *dev,
> > -- 
> > 2.21.0.rc2.261.ga7da99ff1b-goog
> > 
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/vgem: fix use-after-free when drm_gem_handle_create() fails

[Eric Biggers]

From: Eric Biggers 

If drm_gem_handle_create() fails in vgem_gem_create(), then the
drm_vgem_gem_object is freed twice: once when the reference is dropped
by drm_gem_object_put_unlocked(), and again by __vgem_gem_destroy().

This was hit by syzkaller using fault injection.

Fix it by skipping the second free.

Reported-by: syzbot+e73f2fb5ed5a5df36...@syzkaller.appspotmail.com
Fixes: 5ba6c9ff961a ("drm/vgem: Fix mmaping")
Cc: sta...@vger.kernel.org
Signed-off-by: Eric Biggers 
---
 drivers/gpu/drm/vgem/vgem_drv.c | 8 +++-
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/vgem/vgem_drv.c b/drivers/gpu/drm/vgem/vgem_drv.c
index 5930facd6d2d8..70646d9da1596 100644
--- a/drivers/gpu/drm/vgem/vgem_drv.c
+++ b/drivers/gpu/drm/vgem/vgem_drv.c
@@ -189,15 +189,13 @@ static struct drm_gem_object *vgem_gem_create(struct 
drm_device *dev,
return ERR_CAST(obj);
 
ret = drm_gem_handle_create(file, >base, handle);
+
drm_gem_object_put_unlocked(>base);
+
if (ret)
-   goto err;
+   return ERR_PTR(ret);
 
return >base;
-
-err:
-   __vgem_gem_destroy(obj);
-   return ERR_PTR(ret);
 }
 
 static int vgem_gem_dumb_create(struct drm_file *file, struct drm_device *dev,
-- 
2.21.0.rc2.261.ga7da99ff1b-goog

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

[PATCH] idr: remove WARN_ON_ONCE() when trying to replace negative ID

[Eric Biggers]

From: Eric Biggers <ebigg...@google.com>

IDR only supports non-negative IDs.  There used to be a
'WARN_ON_ONCE(id < 0)' in idr_replace(), but it was intentionally
removed by commit 2e1c9b286765 ("idr: remove WARN_ON_ONCE() on negative
IDs").  Then it was added back by commit 0a835c4f090a ("Reimplement IDR
and IDA using the radix tree").  However it seems that adding it back
was a mistake, given that some users such as drm_gem_handle_delete()
(DRM_IOCTL_GEM_CLOSE) pass in a value from userspace to idr_replace(),
allowing the WARN_ON_ONCE to be triggered.  drm_gem_handle_delete()
actually just wants idr_replace() to return an error code if the ID is
not allocated, including in the case where the ID is invalid (negative).

So once again remove the bogus WARN_ON_ONCE().

This bug was found by syzkaller, which encountered the following
warning:

WARNING: CPU: 3 PID: 3008 at lib/idr.c:157 idr_replace+0x1d8/0x240 
lib/idr.c:157
Kernel panic - not syncing: panic_on_warn set ...

CPU: 3 PID: 3008 Comm: syzkaller218828 Not tainted 4.13.0-rc4-next-20170811 
#2
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
Call Trace:
 __dump_stack lib/dump_stack.c:16 [inline]
 dump_stack+0x194/0x257 lib/dump_stack.c:52
 panic+0x1e4/0x417 kernel/panic.c:180
 __warn+0x1c4/0x1d9 kernel/panic.c:541
 report_bug+0x211/0x2d0 lib/bug.c:183
 fixup_bug+0x40/0x90 arch/x86/kernel/traps.c:190
 do_trap_no_signal arch/x86/kernel/traps.c:224 [inline]
 do_trap+0x260/0x390 arch/x86/kernel/traps.c:273
 do_error_trap+0x120/0x390 arch/x86/kernel/traps.c:310
 do_invalid_op+0x1b/0x20 arch/x86/kernel/traps.c:323
 invalid_op+0x1e/0x30 arch/x86/entry/entry_64.S:930
RIP: 0010:idr_replace+0x1d8/0x240 lib/idr.c:157
RSP: 0018:8800394bf9f8 EFLAGS: 00010297
RAX: 88003c6c60c0 RBX: 110007297f43 RCX: 
RDX:  RSI:  RDI: 8800394bfa78
RBP: 8800394bfae0 R08: 82856487 R09: 
R10: 8800394bf9a8 R11: 88006c8bae28 R12: 
R13: 8800394bfab8 R14: dc00 R15: 8800394bfbc8
 drm_gem_handle_delete+0x33/0xa0 drivers/gpu/drm/drm_gem.c:297
 drm_gem_close_ioctl+0xa1/0xe0 drivers/gpu/drm/drm_gem.c:671
 drm_ioctl_kernel+0x1e7/0x2e0 drivers/gpu/drm/drm_ioctl.c:729
 drm_ioctl+0x72e/0xa50 drivers/gpu/drm/drm_ioctl.c:825
 vfs_ioctl fs/ioctl.c:45 [inline]
 do_vfs_ioctl+0x1b1/0x1520 fs/ioctl.c:685
 SYSC_ioctl fs/ioctl.c:700 [inline]
 SyS_ioctl+0x8f/0xc0 fs/ioctl.c:691
 entry_SYSCALL_64_fastpath+0x1f/0xbe

Here is a C reproducer:

#include 
#include 
#include 
#include 
#include 

int main(void)
{
int cardfd = open("/dev/dri/card0", O_RDONLY);

ioctl(cardfd, DRM_IOCTL_GEM_CLOSE,
  &(struct drm_gem_close) { .handle = -1 } );
}

Fixes: 0a835c4f090a ("Reimplement IDR and IDA using the radix tree")
Cc: Andrew Morton <a...@linux-foundation.org>
Cc: Chris Wilson <ch...@chris-wilson.co.uk>
Cc: Dmitry Vyukov <dvyu...@google.com>
Cc: Matthew Wilcox <mawil...@microsoft.com>
Cc: Tejun Heo <t...@kernel.org>
Cc: dri-devel@lists.freedesktop.org
Cc: <sta...@vger.kernel.org> [v4.11+]
Signed-off-by: Eric Biggers <ebigg...@google.com>
---
 lib/idr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/idr.c b/lib/idr.c
index 082778cf883e..f9adf4805fd7 100644
--- a/lib/idr.c
+++ b/lib/idr.c
@@ -151,7 +151,7 @@ EXPORT_SYMBOL(idr_get_next_ext);
  */
 void *idr_replace(struct idr *idr, void *ptr, int id)
 {
-   if (WARN_ON_ONCE(id < 0))
+   if (id < 0)
return ERR_PTR(-EINVAL);
 
return idr_replace_ext(idr, ptr, id);
-- 
2.14.1.581.gf28d330327-goog

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

[Linux v4.3-rc6] i915: issues with Skylake integrated graphics

[Eric Biggers]

On Mon, Oct 26, 2015 at 03:28:37PM +0200, Jani Nikula wrote:
> Please file two separate bugs at [1], one for the above, and another for
> the below. Please add drm.debug=14 module parameter, and attach dmesg
> all the way from boot to the problem.

I have filed https://bugs.freedesktop.org/show_bug.cgi?id=92685 for the second
part.  I did not, however, re-experience the first warning (the
"WARN_ON(p->pixel_rate == 0)") after upgrading to Linux v4.3-rc7 and rebooting
several times.

Eric

[Linux v4.3-rc6] i915: issues with Skylake integrated graphics

[Eric Biggers]

Hi,

I am testing Linux v4.3-rc6 on a laptop with Intel 'Skylake' integrated
graphics.  The integrated display works, but I noticed several warnings in the
kernel log, and a VGA monitor attached to the HDMI input via a VGA->HDMI adapter
did not work.  Furthermore, the system hung for several seconds during boot.
The two WARNs were:

[1.078097] WARNING: CPU: 0 PID: 1 at drivers/gpu/drm/i915/intel_pm.c:3321 
skl_update_pipe_wm+0x7fa/0x810()
[1.078097] WARN_ON(p->pixel_rate == 0)
...
[1.353670] WARNING: CPU: 1 PID: 6 at drivers/gpu/drm/i915/intel_dp.c:854 
intel_dp_aux_ch+0x10f/0x6a0()
[1.353670] dp_aux_ch not started status 0xad40001f

The following error was reported later, when the HDMI input was connected but
the driver failed to report the input as connected:

[   52.553940] [drm:drm_edid_block_valid] *ERROR* EDID checksum is invalid, 
remainder is 130

I have attached the full 'dmesg' output for anyone who may find it useful.  

Eric
-- next part --
A non-text attachment was scrubbed...
Name: dmesg.gz
Type: application/octet-stream
Size: 18629 bytes
Desc: not available
URL: