linux-kernel - RE: [PATCH][drm-next] drm/i915/gvt: ensure -ve return value is handled correctly

Open Source and information security mailing list archives

Message-ID: <F3B0350DF4CB6849A642218320DE483D7D64178C@SHSMSX101.ccr.corp.intel.com>
Date:   Thu, 21 Sep 2017 16:17:13 +0000
From:   "Wang, Zhi A" <zhi.a.wang@...el.com>
To:     Joonas Lahtinen <joonas.lahtinen@...ux.intel.com>,
        Zhenyu Wang <zhenyuw@...ux.intel.com>,
        Joe Perches <joe@...ches.com>
CC:     "Gao, Fred" <fred.gao@...el.com>, David Airlie <airlied@...ux.ie>,
        "intel-gfx@...ts.freedesktop.org" <intel-gfx@...ts.freedesktop.org>,
        "kernel-janitors@...r.kernel.org" <kernel-janitors@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Jani Nikula <jani.nikula@...ux.intel.com>,
        "dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
        "Vivi, Rodrigo" <rodrigo.vivi@...el.com>,
        Colin King <colin.king@...onical.com>,
        "intel-gvt-dev@...ts.freedesktop.org" 
        <intel-gvt-dev@...ts.freedesktop.org>
Subject: RE: [PATCH][drm-next] drm/i915/gvt: ensure -ve return value is
 handled correctly

Hi Joonas:

Thanks for the introduction. I have been thinking about the possibility of introducing GEM_BUG_ON into GVT-g recently and investigating on it. I'm just a bit confused about the usage between GEM_BUG_ON and WARN_ON.

GEM_BUG_ON is only enabled when kernel debug is enabled, which mostly is disabled in a production kernel. In the case of i915, I'm sure it will be enabled in CI test so that it can catch broken code path. Looking into GVT-g, the similar scenario is we enable it in QA test.

Let's say GEM_BUG_ON can do its work very well in QA test but QA test is not fully covered all the condition, then something might be still broken when it comes to the production kernel for user and GEM_BUG_ON will be disabled and will not catch that, I guess.

That's my confusion which scratched my mind during the investigation: If GEM_BUG_ON is not always working, then it looks WARN_ON should always be used.... Expected to learn more about the story behind. :)

Thanks,
Zhi.

-----Original Message-----
From: intel-gvt-dev [mailto:intel-gvt-dev-bounces@...ts.freedesktop.org] On Behalf Of Joonas Lahtinen
Sent: Thursday, September 21, 2017 5:32 PM
To: Zhenyu Wang <zhenyuw@...ux.intel.com>; Joe Perches <joe@...ches.com>
Cc: Gao, Fred <fred.gao@...el.com>; David Airlie <airlied@...ux.ie>; intel-gfx@...ts.freedesktop.org; kernel-janitors@...r.kernel.org; linux-kernel@...r.kernel.org; Jani Nikula <jani.nikula@...ux.intel.com>; dri-devel@...ts.freedesktop.org; Vivi, Rodrigo <rodrigo.vivi@...el.com>; Colin King <colin.king@...onical.com>; intel-gvt-dev@...ts.freedesktop.org; Wang, Zhi A <zhi.a.wang@...el.com>
Subject: Re: [PATCH][drm-next] drm/i915/gvt: ensure -ve return value is handled correctly

On Thu, 2017-09-21 at 06:44 +0800, Zhenyu Wang wrote:
> On 2017.09.19 19:35:23 -0700, Joe Perches wrote:
> > On Wed, 2017-09-20 at 05:46 +0800, Zhenyu Wang wrote:
> > > On 2017.09.19 16:55:34 +0100, Colin King wrote:
> > > > From: Colin Ian King <colin.king@...onical.com>
> > > > 
> > > > An earlier fix changed the return type from find_bb_size however 
> > > > the integer return is being assigned to a unsigned int so the 
> > > > -ve error check will never be detected. Make bb_size an int to fix this.
> > > > 
> > > > Detected by CoverityScan CID#1456886 ("Unsigned compared against 
> > > > 0")
> > > > 
> > > > Fixes: 1e3197d6ad73 ("drm/i915/gvt: Refine error handling for 
> > > > perform_bb_shadow")
> > > > Signed-off-by: Colin Ian King <colin.king@...onical.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/gvt/cmd_parser.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/gvt/cmd_parser.c 
> > > > b/drivers/gpu/drm/i915/gvt/cmd_parser.c
> > > > index 2c0ccbb817dc..f41cbf664b69 100644
> > > > --- a/drivers/gpu/drm/i915/gvt/cmd_parser.c
> > > > +++ b/drivers/gpu/drm/i915/gvt/cmd_parser.c
> > > > @@ -1628,7 +1628,7 @@ static int perform_bb_shadow(struct parser_exec_state *s)
> > > >  	struct intel_shadow_bb_entry *entry_obj;
> > > >  	struct intel_vgpu *vgpu = s->vgpu;
> > > >  	unsigned long gma = 0;
> > > > -	uint32_t bb_size;
> > > > +	int bb_size;
> > > >  	void *dst = NULL;
> > > >  	int ret = 0;
> > > >  
> > > 
> > > Applied this, thanks!
> > 
> > Is it possible for bb_size to be both >= 2g and valid?
> 
> Never be possible in practise and if really that big I think something 
> is already insane indeed.

It's good idea to document these assumptions as WARN_ON's. In i915, if the value is completely internal to kernel, we're using GEM_BUG_ON for these so that our CI will notice breakage. If it's not a driver internal value only, a WARN_ON is the appropriate action.

Otherwise the information is lost and the next person reading the code will have the same question in mind.

Regards, Joonas
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
intel-gvt-dev mailing list
intel-gvt-dev@...ts.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives