From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jani Nikula Subject: Re: [Intel-gfx] [PATCH 1/2] drm/i915: Don't clobber the addfb2 ioctl params Date: Tue, 17 Nov 2015 16:45:31 +0200 Message-ID: <87egfoelbo.fsf@intel.com> References: <1447261890-3960-1-git-send-email-ville.syrjala@linux.intel.com> <20151117094706.GH16848@phenom.ffwll.local> <20151117110421.GQ4437@intel.com> <20151117130045.GS4437@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: In-Reply-To: <20151117130045.GS4437@intel.com> Sender: stable-owner@vger.kernel.org To: Ville =?utf-8?B?U3lyasOkbMOk?= , Daniel Vetter Cc: Daniel Vetter , intel-gfx@lists.freedesktop.org, stable@vger.kernel.org, dri-devel@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Tue, 17 Nov 2015, Ville Syrj=C3=A4l=C3=A4 wrote: > On Tue, Nov 17, 2015 at 01:04:21PM +0200, Ville Syrj=C3=A4l=C3=A4 wro= te: >> On Tue, Nov 17, 2015 at 10:47:06AM +0100, Daniel Vetter wrote: >> > On Wed, Nov 11, 2015 at 07:11:28PM +0200, ville.syrjala@linux.inte= l.com wrote: >> > > From: Ville Syrj=C3=A4l=C3=A4 >> > >=20 >> > > We try to convert the old way of of specifying fb tiling (obj->t= iling) >> > > into the new fb modifiers. We store the result in the passed in = mode_cmd >> > > structure. But that structure comes directly from the addfb2 ioc= tl, and >> > > gets copied back out to userspace, which means we're clobbering = the >> > > modifiers that the user provided (all 0 since the DRM_MODE_FB_MO= DIFIERS >> > > flag wasn't even set by the user). Hence if the user reuses the = struct >> > > for another addfb2, the ioctl will be rejected since it's now as= king for >> > > some modifiers w/o the flag set. >> > >=20 >> > > Fix the problem by making a copy of the user provided structure.= We can >> > > play any games we want with the copy. >> > >=20 >> > > Cc: stable@vger.kernel.org >> > > Cc: Daniel Vetter >> > > Cc: Tvrtko Ursulin >> > > Fixes: 2a80eada326f ("drm/i915: Add fb format modifier support") >> > > Testcase: igt/kms_addfb_basic/clobbered-modifier >> > > Signed-off-by: Ville Syrj=C3=A4l=C3=A4 >> >=20 >> > Out of curiosity: Where does this blow up? That should be added to= the >> > commit message (so that people affected can match it up with this = fix). >>=20 >> I don't know that it affects any actual users. The way I caught this= was >> running kms_addfb_basic. One of the subtests failed when I ran the f= ull >> test, but if I ran only the specific subtest it was fine. So the >> modifiers got clobbered by the previous subtest. I already forgot wh= ich >> subtest it was, but it's easy enough to track it down again. > > # ./tests/kms_addfb_basic=20 > IGT-Version: 1.12-git (x86_64) (Linux: 4.4.0-rc1-stereo+ x86_64) > ... > Subtest basic-X-tiled: SUCCESS (0.001s) > Test assertion failure function pitch_tests, file kms_addfb_basic.c:1= 67: > Failed assertion: drmIoctl(fd, DRM_IOCTL_MODE_ADDFB2, &f) =3D=3D 0 > Last errno: 22, Invalid argument > Stack trace: > #0 [__igt_fail_assert+0x101] > #1 [pitch_tests+0x619] > #2 [__real_main426+0x2f] > #3 [main+0x23] > #4 [__libc_start_main+0xf0] > #5 [_start+0x29] > #6 [+0x29] > Subtest framebuffer-vs-set-tiling failed. > **** DEBUG **** > Test assertion failure function pitch_tests, file kms_addfb_basic.c:1= 67: > Failed assertion: drmIoctl(fd, DRM_IOCTL_MODE_ADDFB2, &f) =3D=3D 0 > Last errno: 22, Invalid argument > **** END **** > Subtest framebuffer-vs-set-tiling: FAIL (0.003s) > ... > > # ./tests/kms_addfb_basic --r framebuffer-vs-set-tiling > IGT-Version: 1.12-git (x86_64) (Linux: 4.4.0-rc1-stereo+ x86_64) > Subtest framebuffer-vs-set-tiling: SUCCESS (0.000s) > >>=20 >> > With that: >> >=20 >> > Reviewed-by: Daniel Vetter Pushed to drm-intel-fixes, thanks for the patch and review. I had to do a trivial rebase, and resolve conflicts for nightly. Ville, please check the commit in -fixes and yell if it's not right. BR, Jani. --=20 Jani Nikula, Intel Open Source Technology Center