linux-kernel - Re: [PATCH v1 22/24] media: hantro: h264: Make dpb entry management more robust

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 30 Mar 2022 11:15:58 -0400
From:   Nicolas Dufresne <nicolas.dufresne@...labora.com>
To:     Sebastian Fricke <sebastian.fricke@...labora.com>
Cc:     Ezequiel Garcia <ezequiel@...guardiasur.com.ar>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        kernel@...labora.com, Jonas Karlman <jonas@...boo.se>,
        linux-media@...r.kernel.org, linux-rockchip@...ts.infradead.org,
        linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 22/24] media: hantro: h264: Make dpb entry management
 more robust

Le mercredi 30 mars 2022 à 09:59 +0200, Sebastian Fricke a écrit :
> Hey Nicolas,
> 
> On 28.03.2022 15:59, Nicolas Dufresne wrote:
> > From: Jonas Karlman <jonas@...boo.se>
> > 
> > The driver maintains stable slot location for reference pictures. This
> 
> s/slot location/slot locations/
> 
> > change makes the code more robust by using the reference_ts as key and
> > by marking all entries invalid right from the start.
> > 
> > Signed-off-by: Jonas Karlman <jonas@...boo.se>
> > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@...labora.com>
> > ---
> > drivers/staging/media/hantro/hantro_h264.c | 10 ++++------
> > 1 file changed, 4 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/staging/media/hantro/hantro_h264.c b/drivers/staging/media/hantro/hantro_h264.c
> > index 228629fb3cdf..7377fc26f780 100644
> > --- a/drivers/staging/media/hantro/hantro_h264.c
> > +++ b/drivers/staging/media/hantro/hantro_h264.c
> > @@ -258,8 +258,7 @@ static void prepare_table(struct hantro_ctx *ctx)
> > static bool dpb_entry_match(const struct v4l2_h264_dpb_entry *a,
> > 			    const struct v4l2_h264_dpb_entry *b)
> > {
> > -	return a->top_field_order_cnt == b->top_field_order_cnt &&
> > -	       a->bottom_field_order_cnt == b->bottom_field_order_cnt;
> > +	return a->reference_ts == b->reference_ts;
> > }
> > 
> > static void update_dpb(struct hantro_ctx *ctx)
> > @@ -273,13 +272,13 @@ static void update_dpb(struct hantro_ctx *ctx)
> > 
> > 	/* Disable all entries by default. */
> > 	for (i = 0; i < ARRAY_SIZE(ctx->h264_dec.dpb); i++)
> > -		ctx->h264_dec.dpb[i].flags &= ~V4L2_H264_DPB_ENTRY_FLAG_ACTIVE;
> > +		ctx->h264_dec.dpb[i].flags = 0;
> 
> Ehm ... we just remove all flags? Can't this have any unwanted side
> effects like removing a flag that we actually wanted to keep?
> (Like long term or the field flags?)

This is a local copy of the dpb, the DPB is fully passed for every decode. So
these flags will be fully restored lower when we copy the found entry. In fact,
holding a state here would not represent well the userland intention and can
have negative side effect on the decoding. Flags are not immutable between
decode and can change. This simplify the code, and make things less error prone.
This part of the code is already a bit complex, no need for an extra layer.

> If we just want to set the DPB entry inactive, then removing the ACTIVE
> flag seems like the correct approach to me.
> If we want to get rid of the VALID flag as well, then we could just do:
> 		ctx->h264_dec.dpb[i].flags &=
>        ~(V4L2_H264_DPB_ENTRY_FLAG_ACTIVE | V4L2_H264_DPB_ENTRY_FLAG_VALID);
> 
> In case we really want to reset all flags, I'd say adjust the comment
> above it:
> ```
> - 	/* Disable all entries by default. */
> + 	/* Reset the flags for all entries by default. */
> ```

This reads the same to me, but I can do that yes. understand that VALID means
the reference exist and the TS should point to some existing past reference
(unless there was some decode error, which the userland may not be aware yet as
this is asynchronous). While ACTIVE means that it is used as a reference. FFMPEG
is known not to filter inactive references. ACTIVE is just a flag without bunch
of other flags that can change for every decode. So none of this make sense
between 2 decodes.

> 
> Greetings,
> Sebastian
> 
> > 
> > 	/* Try to match new DPB entries with existing ones by their POCs. */
> > 	for (i = 0; i < ARRAY_SIZE(dec_param->dpb); i++) {
> > 		const struct v4l2_h264_dpb_entry *ndpb = &dec_param->dpb[i];
> > 
> > -		if (!(ndpb->flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE))
> > +		if (!(ndpb->flags & V4L2_H264_DPB_ENTRY_FLAG_VALID))
> > 			continue;
> > 
> > 		/*
> > @@ -290,8 +289,7 @@ static void update_dpb(struct hantro_ctx *ctx)
> > 			struct v4l2_h264_dpb_entry *cdpb;
> > 
> > 			cdpb = &ctx->h264_dec.dpb[j];
> > -			if (cdpb->flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE ||
> > -			    !dpb_entry_match(cdpb, ndpb))
> > +			if (!dpb_entry_match(cdpb, ndpb))
> > 				continue;
> > 
> > 			*cdpb = *ndpb;
> > -- 
> > 2.34.1
> >