Error in inner loop in validate_cmds_sorted / out of bounds issue
diff mbox

Message ID 20150728111419.38b36358@pc1
State New
Headers show

Commit Message

Hanno Böck July 28, 2015, 6:14 p.m. UTC
Hi,

On Tue, 28 Jul 2015 10:14:51 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> > Indeed, nice catch. Could you please read
> > Documentation/SubmittingPatches and apply your Signed-off-by and
> > then we can accept this patch under your authorship.
> > 
> > Preferrably this is two patches, (a) fix the tables, (b) fix the
> > validator. That way we can delay enabling the validator if we need
> > to fix the tables for others.

I think I have checked all tables, not just the ones used on my gpu,
they should be fine. But I've splittet the patch.

> Also can you please add signed-off-by lines to your patch when
> resubmitting? See Documentation/SubmittingPatches for all the details.

The patch already had a "Signed-off-by" line.

The checkpatch script complains that it doesn't like the formatting of
the CMD command. However I won't change that in this patch, as this is
how the whole file is formatted. If this is wanted I can submit a patch
changing the formatting afterwards, but I think this is an unrelated
change.

Please apply.

Comments

Chris Wilson July 28, 2015, 7:36 p.m. UTC | #1
On Tue, Jul 28, 2015 at 11:14:19AM -0700, Hanno Böck wrote:
> Hi,
> 
> On Tue, 28 Jul 2015 10:14:51 +0200
> Daniel Vetter <daniel@ffwll.ch> wrote:
> 
> > > Indeed, nice catch. Could you please read
> > > Documentation/SubmittingPatches and apply your Signed-off-by and
> > > then we can accept this patch under your authorship.
> > > 
> > > Preferrably this is two patches, (a) fix the tables, (b) fix the
> > > validator. That way we can delay enabling the validator if we need
> > > to fix the tables for others.
> 
> I think I have checked all tables, not just the ones used on my gpu,
> they should be fine. But I've splittet the patch.
> 
> > Also can you please add signed-off-by lines to your patch when
> > resubmitting? See Documentation/SubmittingPatches for all the details.
> 
> The patch already had a "Signed-off-by" line.
> 
> The checkpatch script complains that it doesn't like the formatting of
> the CMD command. However I won't change that in this patch, as this is
> how the whole file is formatted. If this is wanted I can submit a patch
> changing the formatting afterwards, but I think this is an unrelated
> change.
> 
> Please apply.
> 
> -- 
> Hanno Böck
> http://hboeck.de/
> 
> mail/jabber: hanno@hboeck.de
> GPG: BBB51E42

> Properly sort cmd tables.

drm/i915: Properly sort MI coomand table

In the future, we may want to speed up command/register searching using
a bisection and so we require them to be in ascending order respectively
by command value or register address. However, this was not true for one
pair in the MI table; make it so.
 
> Signed-off-by: Hanno Boeck <hanno@hboeck.de>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

> diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c
> index 306d9e4..5fdd8c8 100644
> --- a/drivers/gpu/drm/i915/i915_cmd_parser.c
> +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
> @@ -151,8 +151,8 @@ static const struct drm_i915_cmd_descriptor render_cmds[] = {
>  	CMD(  MI_ARB_ON_OFF,                    SMI,    F,  1,      R  ),
>  	CMD(  MI_PREDICATE,                     SMI,    F,  1,      S  ),
>  	CMD(  MI_TOPOLOGY_FILTER,               SMI,    F,  1,      S  ),
> -	CMD(  MI_DISPLAY_FLIP,                  SMI,   !F,  0xFF,   R  ),
>  	CMD(  MI_SET_APPID,                     SMI,    F,  1,      S  ),
> +	CMD(  MI_DISPLAY_FLIP,                  SMI,   !F,  0xFF,   R  ),
>  	CMD(  MI_SET_CONTEXT,                   SMI,   !F,  0xFF,   R  ),
>  	CMD(  MI_URB_CLEAR,                     SMI,   !F,  0xFF,   S  ),
>  	CMD(  MI_STORE_DWORD_IMM,               SMI,   !F,  0x3F,   B,

> Fix loop checking cmd tables.

drm/i915: Fix command parser table validator

As we may like to use a bisection search on the tables in future, we
need them to be ordered. For convenience we expect the compiled tables
to be order and check on initialisation. However, the validator used the
wrong iterators failed to spot the misordered MI tables and instead
walked off into the unknown (as spotted by kasan).

> Signed-off-by: Hanno Boeck <hanno@hboeck.de>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

> diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c
> index 306d9e4..3a53bf3 100644
> --- a/drivers/gpu/drm/i915/i915_cmd_parser.c
> +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
> @@ -564,7 +564,7 @@ static bool validate_cmds_sorted(struct intel_engine_cs *ring,
>  
>  		for (j = 0; j < table->count; j++) {
>  			const struct drm_i915_cmd_descriptor *desc =
> -				&table->table[i];
> +				&table->table[j];
>  			u32 curr = desc->cmd.value & desc->cmd.mask;
>  
>  			if (curr < previous) {
Daniel Vetter July 29, 2015, 8:33 a.m. UTC | #2
On Tue, Jul 28, 2015 at 08:36:18PM +0100, Chris Wilson wrote:
> On Tue, Jul 28, 2015 at 11:14:19AM -0700, Hanno Böck wrote:
> > Hi,
> > 
> > On Tue, 28 Jul 2015 10:14:51 +0200
> > Daniel Vetter <daniel@ffwll.ch> wrote:
> > 
> > > > Indeed, nice catch. Could you please read
> > > > Documentation/SubmittingPatches and apply your Signed-off-by and
> > > > then we can accept this patch under your authorship.
> > > > 
> > > > Preferrably this is two patches, (a) fix the tables, (b) fix the
> > > > validator. That way we can delay enabling the validator if we need
> > > > to fix the tables for others.
> > 
> > I think I have checked all tables, not just the ones used on my gpu,
> > they should be fine. But I've splittet the patch.
> > 
> > > Also can you please add signed-off-by lines to your patch when
> > > resubmitting? See Documentation/SubmittingPatches for all the details.
> > 
> > The patch already had a "Signed-off-by" line.
> > 
> > The checkpatch script complains that it doesn't like the formatting of
> > the CMD command. However I won't change that in this patch, as this is
> > how the whole file is formatted. If this is wanted I can submit a patch
> > changing the formatting afterwards, but I think this is an unrelated
> > change.
> > 
> > Please apply.
> > 
> > -- 
> > Hanno Böck
> > http://hboeck.de/
> > 
> > mail/jabber: hanno@hboeck.de
> > GPG: BBB51E42
> 
> > Properly sort cmd tables.
> 
> drm/i915: Properly sort MI coomand table
> 
> In the future, we may want to speed up command/register searching using
> a bisection and so we require them to be in ascending order respectively
> by command value or register address. However, this was not true for one
> pair in the MI table; make it so.
>  
> > Signed-off-by: Hanno Boeck <hanno@hboeck.de>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> > diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c
> > index 306d9e4..5fdd8c8 100644
> > --- a/drivers/gpu/drm/i915/i915_cmd_parser.c
> > +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
> > @@ -151,8 +151,8 @@ static const struct drm_i915_cmd_descriptor render_cmds[] = {
> >  	CMD(  MI_ARB_ON_OFF,                    SMI,    F,  1,      R  ),
> >  	CMD(  MI_PREDICATE,                     SMI,    F,  1,      S  ),
> >  	CMD(  MI_TOPOLOGY_FILTER,               SMI,    F,  1,      S  ),
> > -	CMD(  MI_DISPLAY_FLIP,                  SMI,   !F,  0xFF,   R  ),
> >  	CMD(  MI_SET_APPID,                     SMI,    F,  1,      S  ),
> > +	CMD(  MI_DISPLAY_FLIP,                  SMI,   !F,  0xFF,   R  ),
> >  	CMD(  MI_SET_CONTEXT,                   SMI,   !F,  0xFF,   R  ),
> >  	CMD(  MI_URB_CLEAR,                     SMI,   !F,  0xFF,   S  ),
> >  	CMD(  MI_STORE_DWORD_IMM,               SMI,   !F,  0x3F,   B,
> 
> > Fix loop checking cmd tables.
> 
> drm/i915: Fix command parser table validator
> 
> As we may like to use a bisection search on the tables in future, we
> need them to be ordered. For convenience we expect the compiled tables
> to be order and check on initialisation. However, the validator used the
> wrong iterators failed to spot the misordered MI tables and instead
> walked off into the unknown (as spotted by kasan).
> 
> > Signed-off-by: Hanno Boeck <hanno@hboeck.de>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Ok I hand-assembled the patches since it's your first, but please followo
Documentation/Submitting patches next time around, i.e. 1 mail per patch,
no attachments, diff included at the bottom. Otherwise the tools can't
just pick up patches on my side. git send-email will do all that
formatting for you correctly, so that's what I recommend to use.

Anyway pachtes applied to drm-intel-next-queued, thanks a lot for doing
them.
-Daniel

> 
> > diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c
> > index 306d9e4..3a53bf3 100644
> > --- a/drivers/gpu/drm/i915/i915_cmd_parser.c
> > +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
> > @@ -564,7 +564,7 @@ static bool validate_cmds_sorted(struct intel_engine_cs *ring,
> >  
> >  		for (j = 0; j < table->count; j++) {
> >  			const struct drm_i915_cmd_descriptor *desc =
> > -				&table->table[i];
> > +				&table->table[j];
> >  			u32 curr = desc->cmd.value & desc->cmd.mask;
> >  
> >  			if (curr < previous) {
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre

Patch
diff mbox

Fix loop checking cmd tables.

Signed-off-by: Hanno Boeck <hanno@hboeck.de>
diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c
index 306d9e4..3a53bf3 100644
--- a/drivers/gpu/drm/i915/i915_cmd_parser.c
+++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
@@ -564,7 +564,7 @@  static bool validate_cmds_sorted(struct intel_engine_cs *ring,
 
 		for (j = 0; j < table->count; j++) {
 			const struct drm_i915_cmd_descriptor *desc =
-				&table->table[i];
+				&table->table[j];
 			u32 curr = desc->cmd.value & desc->cmd.mask;
 
 			if (curr < previous) {