<html> <head> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> <style type="text/css" style="display:none;"></style> </head> <body dir="ltr"> <div id="divtagdefaultwrapper" style="font-size:12pt;color:#000000;font-family:Calibri,Helvetica,sans-serif;" dir="ltr"> <p style="margin-top:0;margin-bottom:0">Did find way to reproduce issue constantly. After applying David's patch "<span>0001-drm-amdgpu-fix-signaled-fence-isn-t-handled" </span>with minor change</p> <p style="margin-top:0;margin-bottom:0"><br> </p> <p style="margin-top:0;margin-bottom:0"></p> <div>-static struct dma_fence *drm_syncobj_get_stub_fence(void)</div> <div>+struct dma_fence *drm_syncobj_get_stub_fence(void)</div> <br> <p></p> <p style="margin-top:0;margin-bottom:0">was able to avoid kernel panic due to NULL pointer dereference. So seems it is resolving the problem which I was trying to address. </p> <p style="margin-top:0;margin-bottom:0">Can you please put revision of this patch?</p> <p style="margin-top:0;margin-bottom:0"><br> </p> <p style="margin-top:0;margin-bottom:0"><br> </p> <p style="margin-top:0;margin-bottom:0">Though only one time I got reboot with dmesg..</p> <p style="margin-top:0;margin-bottom:0"><span style="font-size: 12pt;"><br> </span></p> <p style="margin-top:0;margin-bottom:0"><span style="font-size: 12pt;"></p> <div>[ 213.714324] RIP: 0010:reservation_object_add_shared_fence+0x22e/0x245</div> <div>[ 213.714331] RSP: 0018:ffffa03900b5ba80 EFLAGS: 00010246</div> <div>[ 213.714338] RAX: 0000000000000004 RBX: ffffa03900b5bba8 RCX: ffff969421139d00</div> <div>[ 213.714343] RDX: 0000000000000000 RSI: ffff9693639662e0 RDI: ffff9694203db230</div> <div>[ 213.714349] RBP: ffffa03900b5bad0 R08: 000000000000002a R09: ffff969363966280</div> <div>[ 213.714354] R10: 0000000180800079 R11: ffffffffa3637f66 R12: ffff96941ea5cf00</div> <div>[ 213.714360] R13: 0000000000000000 R14: ffff9693639662e0 R15: ffff9694203db230</div> <div>[ 213.714366] FS: 0000786009e5f700(0000) GS:ffff96942ec00000(0000) knlGS:0000000000000000</div> <div>[ 213.714372] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033</div> <div>[ 213.714377] CR2: 00007860023a4000 CR3: 000000011ea8a000 CR4: 00000000001406f0</div> <div>[ 213.714382] Call Trace:</div> <div>[ 213.714397] ttm_eu_fence_buffer_objects+0x53/0x89</div> <div>[ 213.714407] amdgpu_cs_ioctl+0x194f/0x196c</div> <div>[ 213.714420] ? amdgpu_cs_report_moved_bytes+0x5f/0x5f</div> <div>[ 213.714427] drm_ioctl_kernel+0x98/0xac</div> <div>[ 213.714435] drm_ioctl+0x235/0x357</div> <div>[ 213.714443] ? amdgpu_cs_report_moved_bytes+0x5f/0x5f</div> <div>[ 213.714450] ? __switch_to_asm+0x34/0x70</div> <div>[ 213.714456] ? __switch_to_asm+0x40/0x70</div> <div>[ 213.714462] ? __switch_to_asm+0x34/0x70</div> <div>[ 213.714468] ? __switch_to_asm+0x40/0x70</div> <div>[ 213.714476] amdgpu_drm_ioctl+0x46/0x78</div> <div>[ 213.714500] vfs_ioctl+0x1b/0x30</div> <div>[ 213.714507] do_vfs_ioctl+0x492/0x6c1</div> <div>[ 213.714530] SyS_ioctl+0x52/0x77</div> <div>[ 213.714537] do_syscall_64+0x6d/0x81</div> <div>[ 213.714544] entry_SYSCALL_64_after_hwframe+0x3d/0xa2</div> <div>[ 213.714551] RIP: 0033:0x78600c9c6967</div> <div>[ 213.714556] RSP: 002b:0000786009e5ec08 EFLAGS: 00000246 ORIG_RAX: 0000000000000010</div> <div>[ 213.714563] RAX: ffffffffffffffda RBX: 00000000c0186444 RCX: 000078600c9c6967</div> <div>[ 213.714568] RDX: 0000786009e5ec60 RSI: 00000000c0186444 RDI: 000000000000000d</div> <div>[ 213.714573] RBP: 0000786009e5ec40 R08: 0000786009e5ed00 R09: 0000786009e5ec50</div> <div>[ 213.714578] R10: 0000000000000000 R11: 0000000000000246 R12: 000000000000000d</div> <div>[ 213.714583] R13: 00003ba2573ff128 R14: 0000000000000000 R15: 0000786009e5ec60</div> <p style="margin-top:0;margin-bottom:0"><span style="font-size: 12pt;"><br> </span></p> <p style="margin-top:0;margin-bottom:0"><span style="font-size: 12pt;"><br> </span></p> Its worth to mention that I am not using latest kernel .relevant git log <span></span></span> <p></p> <p style="margin-top:0;margin-bottom:0"><span style="font-size: 12pt;"><br> </span></p> <p style="margin-top:0;margin-bottom:0"><span style="font-size: 12pt;"><span>71a0556255de125b7e3fc0dc6171fb31fab2b9ad drm/syncobj: Don't leak fences when WAIT_FOR_SUBMIT is set</span><br> </span></p> <p style="margin-top:0;margin-bottom:0"><span style="font-size: 12pt;"><span><span>4034318d2afac8f7f43457a4b480b877a94ec651 <span style="font-family: Calibri, Helvetica, sans-serif, EmojiFont, "Apple Color Emoji", "Segoe UI Emoji", NotoColorEmoji, "Segoe UI Symbol", "Android Emoji", EmojiSymbols; font-size: 16px;">drm/syncobj: Stop reusing the same struct file for all syncobj -> fd</span></span><br> </span></span></p> <p style="margin-top:0;margin-bottom:0"><span style="font-size: 12pt;"><span><br> </span></span></p> <p style="margin-top:0;margin-bottom:0"><span style="font-size: 12pt;">So have not taken any recent changes from <a href="https://patchwork.kernel.org/patch/10575275/" class="OWAAutoLink" id="LPlnk368333" previewremoved="true">https://patchwork.kernel.org/patch/10575275/</a> series. only </span>backported<span style="font-size: 12pt;"> attached </span><span style="font-size: 12pt;">two patches . I need to check if something is missing that can cause issue.</span></p> <p style="margin-top:0;margin-bottom:0"><span style="font-size: 12pt;"><br> </span></p> <p style="margin-top:0;margin-bottom:0"><span style="font-size: 12pt;">Thanks,</span></p> <p style="margin-top:0;margin-bottom:0"><span style="font-size: 12pt;">Deepak</span></p> <p style="margin-top:0;margin-bottom:0"><br> </p> <div style="color: rgb(0, 0, 0);"> <hr style="display:inline-block;width:98%" tabindex="-1"> <div id="divRplyFwdMsg" dir="ltr"><font face="Calibri, sans-serif" style="font-size:11pt" color="#000000"><b>From:</b> Sharma, Deepak<br> <b>Sent:</b> Tuesday, November 27, 2018 3:12 PM<br> <b>To:</b> Zhou, David(ChunMing); Koenig, Christian; amd-gfx@lists.freedesktop.org<br> <b>Subject:</b> Re: [PATCH] drm/amdgpu: add the checking to avoid NULL pointer dereference</font> <div> </div> </div> <div dir="ltr"> <div id="x_divtagdefaultwrapper" dir="ltr" style="font-size:12pt; color:rgb(0,0,0); font-family:Calibri,Helvetica,sans-serif,EmojiFont,"Apple Color Emoji","Segoe UI Emoji",NotoColorEmoji,"Segoe UI Symbol","Android Emoji",EmojiSymbols"> <p style="margin-top:0; margin-bottom:0">Thanks David , </p> <p style="margin-top:0; margin-bottom:0">I did backport the drm patch to my kernel after that I am not getting <span>-ENOMEM from <span>amdgpu_cs_ioctl </span>while running tests so have not been able to test patch to handle signaled fence. As this issue is hard to reproduce , will give some more try. </span></p> <p style="margin-top:0; margin-bottom:0"><span>But yes the problem is there and need to handle when fence is null that your patch seems to handle it correctly.</span></p> <br> -Deepak<br> <div style="color:rgb(0,0,0)"> <hr tabindex="-1" style="display:inline-block; width:98%"> <div id="x_divRplyFwdMsg" dir="ltr"><font face="Calibri, sans-serif" color="#000000" style="font-size:11pt"><b>From:</b> Zhou, David(ChunMing)<br> <b>Sent:</b> Monday, November 26, 2018 6:40 PM<br> <b>To:</b> Sharma, Deepak; Zhou, David(ChunMing); Koenig, Christian; amd-gfx@lists.freedesktop.org<br> <b>Subject:</b> Re: [PATCH] drm/amdgpu: add the checking to avoid NULL pointer dereference</font> <div> </div> </div> <div class="x_BodyFragment"><font size="2"><span style="font-size:11pt"> <div class="x_PlainText">Yeah, you need another drm patch as well when you apply my patch. Attached.<br> <br> -David<br> <br> <br> On 2018年11月27日 08:40, Sharma, Deepak wrote:<br> ><br> > On 11/26/18 1:57 AM, Zhou, David(ChunMing) wrote:<br> >><br> >>> -----Original Message-----<br> >>> From: Christian König <ckoenig.leichtzumerken@gmail.com><br> >>> Sent: Monday, November 26, 2018 5:23 PM<br> >>> To: Sharma, Deepak <Deepak.Sharma@amd.com>; Zhou, David(ChunMing)<br> >>> <David1.Zhou@amd.com>; Koenig, Christian <Christian.Koenig@amd.com>;<br> >>> amd-gfx@lists.freedesktop.org<br> >>> Subject: Re: [PATCH] drm/amdgpu: add the checking to avoid NULL pointer<br> >>> dereference<br> >>><br> >>> Am 26.11.18 um 02:59 schrieb Sharma, Deepak:<br> >>>> 在 2018/11/24 2:10, Koenig, Christian 写道:<br> >>>>> Am 23.11.18 um 15:10 schrieb Zhou, David(ChunMing):<br> >>>>>> 在 2018/11/23 21:30, Koenig, Christian 写道:<br> >>>>>>> Am 23.11.18 um 14:27 schrieb Zhou, David(ChunMing):<br> >>>>>>>> 在 2018/11/22 19:25, Christian König 写道:<br> >>>>>>>>> Am 22.11.18 um 07:56 schrieb Sharma, Deepak:<br> >>>>>>>>>> when returned fence is not valid mostly due to userspace ignored<br> >>>>>>>>>> previous error causes NULL pointer dereference.<br> >>>>>>>>> Again, this is clearly incorrect. The my other mails on the<br> >>>>>>>>> earlier patch.<br> >>>>>>>> Sorry for I didn't get your history, but looks from the patch<br> >>>>>>>> itself, it is still a valid patch, isn't it?<br> >>>>>>> No, the semantic of amdgpu_ctx_get_fence() is that we return NULL<br> >>>>>>> when the fence is already signaled.<br> >>>>>>><br> >>>>>>> So this patch could totally break userspace because it changes the<br> >>>>>>> behavior when we try to sync to an already signaled fence.<br> >>>>>> Ah, I got your meaning, how about attached patch?<br> >>>>> Yeah something like this, but I would just give the<br> >>>>> DRM_SYNCOBJ_CREATE_SIGNALED instead.<br> >>>>><br> >>>>> I mean that's what this flag is good for isn't it?<br> >>>> Yeah, I give a flag initally when creating patch, but as you know, there is a<br> >>> swich case not be able to use that flag:<br> >>>> case AMDGPU_FENCE_TO_HANDLE_GET_SYNC_FILE_FD:<br> >>>> fd = get_unused_fd_flags(O_CLOEXEC);<br> >>>> if (fd < 0) {<br> >>>> dma_fence_put(fence);<br> >>>> return fd;<br> >>>> }<br> >>>><br> >>>> sync_file = sync_file_create(fence);<br> >>>> dma_fence_put(fence);<br> >>>> if (!sync_file) {<br> >>>> put_unused_fd(fd);<br> >>>> return -ENOMEM;<br> >>>> }<br> >>>><br> >>>> fd_install(fd, sync_file->file);<br> >>>> info->out.handle = fd;<br> >>>> return 0;<br> >>>><br> >>>> So I change to stub fence instead.<br> >>> Yeah, I've missed that case. Not sure if the sync_file can deal with a NULL<br> >>> fence.<br> >>><br> >>> We should then probably move the stub fence function into<br> >>> dma_fence_stub.c under drivers/dma-buf to keep the stuff together.<br> >> Yes, you wrap it to review first with your stub fence, we can do it separately first.<br> >><br> >> -David<br> >>>> -David<br> >>>><br> >>>> I have not applied this patch.<br> >>>> The issue was trying to address is when amdgpu_cs_ioctl() failed due to<br> >>> low memory (ENOMEM) but userspace chose to proceed and called<br> >>> amdgpu_cs_fence_to_handle_ioctl().<br> >>>> In amdgpu_cs_fence_to_handle_ioctl(), fence is null and later causing<br> >>>> NULL pointer dereference, this patch was to avoid that and system panic<br> >>> But I understand now that its a valid case retuning NULL if fence was already<br> >>> signaled but need to handle case so avoid kernel panic. Seems David patch<br> >>> should fix this, I will test it tomorrow.<br> >>><br> >>> Mhm, but don't we bail out with an error if we ask for a failed command<br> >>> submission? If not that sounds like a bug as well.<br> >>><br> >>> Christian.<br> >>><br> > Where do we do that?<br> > I see error<br> > [drm:amdgpu_cs_ioctl] *ERROR* amdgpu_cs_list_validate(validated) failed.<br> > [drm:amdgpu_cs_ioctl] *ERROR* Not enough memory for command submission!<br> > BUG: unable to handle kernel NULL pointer dereference at 0000000000000008<br> > Did some more debugging,dma_fence_is_array() is causing NULL pointer<br> > dereference call through sync_file_ioctl.<br> ><br> > Also I think changes in David patch cant be applied on<br> > amd-staging-drm-next, which all patches I should take to get it correctly?<br> ><br> >>>> -Deepak<br> >>>>> Christian.<br> >>>>><br> >>>>>> -David<br> >>>>>>> If that patch was applied then please revert it immediately.<br> >>>>>>><br> >>>>>>> Christian.<br> >>>>>>><br> >>>>>>>> -David<br> >>>>>>>>> If you have already pushed the patch then please revert.<br> >>>>>>>>><br> >>>>>>>>> Christian.<br> >>>>>>>>><br> >>>>>>>>>> Signed-off-by: Deepak Sharma <Deepak.Sharma@amd.com><br> >>>>>>>>>> ---<br> >>>>>>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 2 ++<br> >>>>>>>>>> 1 file changed, 2 insertions(+)<br> >>>>>>>>>><br> >>>>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c<br> >>>>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c<br> >>>>>>>>>> index 024dfbd87f11..14166cd8a12f 100644<br> >>>>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c<br> >>>>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c<br> >>>>>>>>>> @@ -1403,6 +1403,8 @@ static struct dma_fence<br> >>>>>>>>>> *amdgpu_cs_get_fence(struct amdgpu_device *adev,<br> >>>>>>>>>> fence = amdgpu_ctx_get_fence(ctx, entity, user->seq_no);<br> >>>>>>>>>> amdgpu_ctx_put(ctx);<br> >>>>>>>>>> + if(!fence)<br> >>>>>>>>>> + return ERR_PTR(-EINVAL);<br> >>>>>>>>>> return fence;<br> >>>>>>>>>> }<br> >>>> _______________________________________________<br> >>>> amd-gfx mailing list<br> >>>> amd-gfx@lists.freedesktop.org<br> >>>> <a href="https://lists.freedesktop.org/mailman/listinfo/amd-gfx" id="LPlnk611918" class="x_OWAAutoLink" previewremoved="true"> https://lists.freedesktop.org/mailman/listinfo/amd-gfx</a><br> <br> </div> </span></font></div> </div> </div> </div> </div> </div> </body> </html>