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
