Re: [PATCH for-next] RDMA/core: Fix best page size finding when it can cross SG entries

[Leon Romanovsky <leon@kernel.org>]

On Thu, Feb 13, 2025 at 10:42:19AM -0400, Jason Gunthorpe wrote:
> On Thu, Feb 13, 2025 at 04:30:11PM +0200, Margolin, Michael wrote:
> > 
> > On 2/13/2025 4:04 PM, Jason Gunthorpe wrote:
> > > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> > > 
> > > 
> > > 
> > > On Thu, Feb 13, 2025 at 02:51:26PM +0200, Leon Romanovsky wrote:
> > > > diff --git a/drivers/infiniband/core/umem.c b/drivers/infiniband/core/umem.c
> > > > index e7e428369159..63a92d6cfbc2 100644
> > > > --- a/drivers/infiniband/core/umem.c
> > > > +++ b/drivers/infiniband/core/umem.c
> > > > @@ -112,8 +112,7 @@ unsigned long ib_umem_find_best_pgsz(struct ib_umem *umem,
> > > >                  /* If the current entry is physically contiguous with the previous
> > > >                   * one, no need to take its start addresses into consideration.
> > > >                   */
> > > > -               if (curr_base + curr_len != sg_dma_address(sg)) {
> > > > -
> > > > +               if (curr_base != sg_dma_address(sg) - curr_len) {
> > > >                          curr_base = sg_dma_address(sg);
> > > >                          curr_len = 0;
> > > I'm not sure about this, what ensures sg_dma_address() > curr_len?
> > > 
> > > curr_base + curr_len could also overflow, we've seen that AMD IOMMU
> > > sometimes uses the very high addresess already
> > 
> > I think the only case we care about where curr_base + curr_len can overflow
> > is when next sg_dma_address() == 0.
> > 
> > But maybe we should just add an explicit check:
> > 
> > -               if (curr_base + curr_len != sg_dma_address(sg)) {
> > +               if (curr_base + curr_len < curr_base ||
> > +                   curr_base + curr_len != sg_dma_address(sg)) {
> >                         curr_base = sg_dma_address(sg);
> >                         curr_len = 0;
> 
> Ugh
> 
> I wonder if we should try to make a overflow.h helper for these kinds
> of problems.
> 
> /* Check if a + n == b, failing if a+n overflows */
> check_consecutive(a, n, b)
> 
> ?
> 
> It is a fairly common problem
> 
> I suggest to take the patch as it originally was and try to propose
> the above helper?

My concern was with this line:
if (curr_base + curr_len != sg_dma_address(sg)) {

Initially curr_base is 0xFF.....FF and curr_len is 0.
So if this "if ..." is skipped (not possible but static checkers don't know),
we will advance curr_len and curr_base + curr_len will overflow.

I don't want to take original patch.

Thanks

> 
> Jason