Re: [PATCH v5] dma-buf: Fix silent overflow for phys vec to sgt

[Leon Romanovsky]

On Thu, Jun 04, 2026 at 03:36:48PM -0400, David Hu wrote:
> On Thu, Jun 4, 2026 at 5:43 AM Leon Romanovsky <leon@xxxxxxxxxx> wrote:
> >
> > On Mon, Jun 01, 2026 at 08:00:12PM +0000, David Hu wrote:
> > > @@ -36,7 +36,7 @@ static unsigned int calc_sg_nents(struct dma_iova_state *state,
> > >                                 struct phys_vec *phys_vec, size_t nr_ranges,
> > >                                 size_t size)
> > >  {
> > > -     unsigned int nents = 0;
> > > +     size_t nents = 0;
> > >       size_t i;
> > >
> > >       if (!state || !dma_use_iova(state)) {
> > > @@ -51,6 +51,9 @@ static unsigned int calc_sg_nents(struct dma_iova_state *state,
> > >               nents = DIV_ROUND_UP(size, UINT_MAX);
> > >       }
> > >
> > > +     if (nents > UINT_MAX)
> >
> > I would suggest to use check_add_overflow() while calculating nents
> > instead of this check.
> 
> Hi Leon,
> 
> Thank you for the review. Using `check_add_overflow()` is a great
> suggestion and definitely
> cleaner for the accumulation loop. I'll update this for v6.
> 
> > > @@ -133,6 +137,11 @@ struct sg_table *dma_buf_phys_vec_to_sgt(struct dma_buf_attachment *attach,
> > >       }
> > >
> > >       nents = calc_sg_nents(dma->state, phys_vec, nr_ranges, size);
> > > +     if (!nents) {
> > > +             ret = -EINVAL;
> > > +             goto err_free_state;
> > > +     }
> >
> > Technically, this hunk is not necessary, since sg_alloc_table() will
> > return -EINVAL when nents == 0. At least, that is the behavior I relied on.
> 
> I originally added this explicit check in v5 to address Jason's
> feedback, and to make the
> failure explicit rather than relying on `sg_alloc_table()` failing
> silently on `nents=0`.

I prefer explicit checks, but I am not in favor of duplicating them.
Since sg_alloc_table() already validates this condition, we do not need
to repeat the same check in dma-buf. A comment should be sufficient to
inform future reviewers that nents == 0 is already handled.

Thanks

> 
> Jason, do you have a strong preference here? I am happy to drop the
> hunk and rely on
> `sg_alloc_table()` returning `-EINVAL` if you are both comfortable with that.
> 
> Thanks,
> David