On Tue, 2023-10-31 at 09:15 +0000, Manna, Animesh wrote:
>
> > -----Original Message-----
> > From: Luca Coelho <l...@coelho.fi>
> > Sent: Tuesday, October 31, 2023 1:14 PM
> > To: Manna, Animesh <animesh.ma...@intel.com>; intel-
> > g...@lists.freedesktop.org
> > Cc: Nikula, Jani <jani.nik...@intel.com>
> > Subject: Re: [Intel-gfx] [PATCH v4] drm/i915/dsb: DSB code refactoring
> >
> > On Fri, 2023-10-27 at 17:27 +0530, Animesh Manna wrote:
> > > Refactor DSB implementation to be compatible with Xe driver.
> > >
> > > v1: RFC version.
> > > v2: Make intel_dsb structure opaque from external usage. [Jani]
> > > v3: Rebased on latest.
> > > v4:
> > > - Add boundary check in dsb_buffer_memset(). [Luca]
> > > - Use size_t instead of u32. [Luca]
> > >
> > > Cc: Jani Nikula <jani.nik...@intel.com>
> > > Signed-off-by: Animesh Manna <animesh.ma...@intel.com>
> > > ---
> >
> > [...]
> > > +void intel_dsb_buffer_memset(struct intel_dsb_buffer *dsb_buf, u32
> > > +idx, u32 val, size_t size) {
> > > + if ((idx > dsb_buf->buf_size / 4) || (size > dsb_buf->buf_size - idx
> > > +* 4))
> >
> > You actually don't need the first expression. This expression should
> > enough:
> >
> > dsb_buf->buf_size <= (idx + size) * sizeof(*dsb_buf->cmd_buf)
>
> Here size is in bytes, but idx is index of 32 bytes array. So, the above
> expression would be,
>
> dsb_buf->buf_size <= (idx * sizeof(*dsb_buf->cmd_buf) + size)
Oh, you're right, of course.
> The same is done with 2nd expression but agree to use sizeof() instead of
> magic number 4.
>
> The first expression is added if idx is big number and due to overflow the
> above check can pass which is not correct. Please let me know your thoughts,
> if you are not ok will drop maybe.
If you're worried about overflow when you're multiplying by 4, then you
can just do it the opposite way, still with a single expression:
dsb_buf->buf_size / sizeof(*dsb_buf->cmd_buf) <= idx + size /
sizeof(*dsb_buf->cmd_buf)
Or, taking advantage of the fact that both buf_size and size need to be
divided by sizeof(), we could something like:
idx > (dsb_buf->buf_size - size) / sizeof(*dsb_buf->cmd_buf)
...but we're bike-shedding. I don't think the number of expressions or
the complexity of the expressions matter much here, unless you're
really in a hotpath, in which case you should add an unlikely() or so.
I'll leave it to you.
> >
> > > + return;
> >
> > Blindly returning here doesn't solve the problem, it just hides it. I
> > think the
> > best would be to use WARN_ON() instead of if.
> >
> > So:
> > WARN_ON(dsb_buf->buf_size <= (idx + size) * sizeof(*dsb_buf-
> > > cmd_buf));
>
> I will add the WARN_ON().
This is the part that I actually think is important. ;)
--
Cheers,
Luca.
